On 7/23/2014 2:09 AM, Fraser Tweedale wrote:

On Wed, Jul 23, 2014 at 11:27:11AM +1000, Fraser Tweedale wrote:

...

Along with LDAP profiles, we will be adding modules to the CLI for adding and editing profiles in the ConfigStore format that was used for file-based profiles. For more info, see:

http://pki.fedoraproject.org/wiki/LDAP_Profile_Storage#Command-line_utilitie...

There is an existing CLI for adding and modifying profiles, in the XML format, e.g. ``pki ca profile add caCustomProfile.xml``. The XML format carries information including the profile ID and class_id, but these data must be supplied out-of-band when dealing with the ConfigStore format.

Because of this, I intend to:

- add new commands to the existing profile CLI for working with the "raw" (i.e., ConfigStore) format, e.g. "edit-raw", "add-raw". Where necessary, these commands will take compulsory ``--profile-id`` and/or ``--class-id`` arguments, to account for the absense of such information in the profile ConfigStore format;

and

- transport this information in the XML format - not in the "raw" format - so that it will be unnecessary to make changes to ProfileClient or the ProfileService API.

As usual, I welcome feedback - especially if you feel I am going the wrong way ^_^

An update: due to the various Profile and ProfileInput/Output/Policy classes not being distributed in the pki-tools package (or dependencies thereof), I /did/ end up adding methods to the REST API.

The `pki ca profile show-raw <profileId>` command is implemented in the attached patch 0009. I expected I will complete the edit-raw and add-raw commands tomorrow, and after that will move on to testing replication and polling/monitoring for changes to profiles.

Cheers,

Fraser

Some comments/questions: 1. About the the new ou=certProfiles entry, We probably should move it into the ou=ca subtree because that subtree already contains other CA-specific LDAP entries such as ou=certificateRepository. 2. Is certProfilesLastModified really necessary? Each LDAP entry already has a modifyTimestamp attributes. To be accurate, the certProfilesLastModified has to be updated whenever the profiles are imported or updated. There seems to be no code for that yet, and there's no guarantee it won't get out of sync (e.g. server crash, direct LDAP modification). 3. If certProfilesLastModified is really necessary, instead of adding a separate cn=certProfilesInfo entry just to hold this attribute, we probably can move it into ou=certProfiles either by extending the organizationalUnit object class or adding an auxiliary object class. We probably should make it an optional attribute (i.e. no attribute means never updated). 4. I'm not sure if we really need the certProfileIsDefault attribute. There's no guarantee that the value will be accurate because someone could modify the certProfileConfig directly and forget to update certProfileIsDefault. Also, if we need to do any profile upgrade, it should be based on the actual configuration parameters in certProfileConfig, not based on the certProfileIsDefault. 5. In LDAPConfigStore let's not use the 'm' prefix for the field names. It's a legacy convention and we should not use it for newer code. 6. The mInDatabase in LDAPConfigStore doesn't seem to be used anywhere. 7. In LDAPConfigStore the certProfileConfig attribute is read as binary using getByteValues(). Is there a specific reason? Can we use getStringValues() since the content is a string (i.e. property file)? 8. The ProfileService constructs the profile DN. I think this level of details should be hidden in the ProfileSubsystem layer. The ProfileService shouldn't need to know where/how the profiles are stored. 9. The ProfileAdminServlet calls ProfileSubsystem.createProfile() but it supplies a config path instead of a DN. As in #8, I think the caller should only specify the profile ID, it's up to the ProfileSubsystem to determine the actual location (i.e. profile DN). 10. The mProfileDNs in ProfileSubsystem shouldn't be necessary because we can generate the DN from the profileId since it's a flat tree. 11. The profile subsystem index is hard-coded in the following line: cs.remove("subsystem.1.enabled"); Although this code is modifying the standard CS.cfg, it would be better to do a search the index, then make the modification with the index. int index = ... find subsystem index where id=profile ... cs.remove("subsystem." + index + ".enabled"); This will avoid problems if we ever add a default subsystem in the future. 12. The profile-show-raw is essentially the same as profile-show, except the output format is different. I'd suggest merging them into profile-show and use an option like --raw to show the raw profile. About the out-of-band class ID, I think we should just map it into a 'virtual' property in the raw profile (e.g. class_id=...). The property will only exist if the client pulls the raw profile. As already in done the patch, in the database it will be stored in classId attribute instead of certProfileConfig. In the long term all properties will be converted into separate attributes like this, and the 'raw' format should only exist as a legacy interface. 13. The ProfileResource.retrieveProfileRaw() returns a byte[]. Although it works just fine for this purpose, I think returning a Properties would make it more developer-friendly. That way the output is readily usable, and it takes care of any CR/LF issue across different platforms. Maybe call it getProfileProperties()? 14. The CAInstallerService.importProfiles() and importProfile() don't need to be static. As virtual methods it will have access to other member attributes/methods already defined in the class and its super classes (e.g. config store). A long term goal is to eliminate the use of static methods in CMS (it's not very modular). 15. Minor. I see two styles of writing the try-catch block in the patch: try { ... } catch (...) { ... } and try { ... } catch (...) { ... } I think the first one is more common. 16. Minor. Instead of adding the enabled parameter to the existing SubsystemInfo constructor (and having to modify all invocations), it's also possible to create a new constructor with the enabled parameter and call it from the old constructor with enabled set to true. -- Endi S. Dewata

On Thu, 2014-08-07 at 17:15 +1000, Fraser Tweedale wrote:

On Wed, Jul 30, 2014 at 04:13:33PM -0500, Endi Sukma Dewata wrote:

...

On 7/23/2014 2:09 AM, Fraser Tweedale wrote:

...

On Wed, Jul 23, 2014 at 11:27:11AM +1000, Fraser Tweedale wrote:

...

Along with LDAP profiles, we will be adding modules to the CLI for adding and editing profiles in the ConfigStore format that was used for file-based profiles. For more info, see:

http://pki.fedoraproject.org/wiki/LDAP_Profile_Storage#Command-line_utilitie...

There is an existing CLI for adding and modifying profiles, in the XML format, e.g. ``pki ca profile add caCustomProfile.xml``. The XML format carries information including the profile ID and class_id, but these data must be supplied out-of-band when dealing with the ConfigStore format.

Because of this, I intend to:

- add new commands to the existing profile CLI for working with the "raw" (i.e., ConfigStore) format, e.g. "edit-raw", "add-raw". Where necessary, these commands will take compulsory ``--profile-id`` and/or ``--class-id`` arguments, to account for the absense of such information in the profile ConfigStore format;

and

- transport this information in the XML format - not in the "raw" format - so that it will be unnecessary to make changes to ProfileClient or the ProfileService API.

As usual, I welcome feedback - especially if you feel I am going the wrong way ^_^

An update: due to the various Profile and ProfileInput/Output/Policy classes not being distributed in the pki-tools package (or dependencies thereof), I /did/ end up adding methods to the REST API.

The `pki ca profile show-raw <profileId>` command is implemented in the attached patch 0009. I expected I will complete the edit-raw and add-raw commands tomorrow, and after that will move on to testing replication and polling/monitoring for changes to profiles.

Cheers,

Fraser

Some comments/questions:

Thanks for the review/feedback Endi. Responses inline.

...

1. About the the new ou=certProfiles entry, We probably should move it into the ou=ca subtree because that subtree already contains other CA-specific LDAP entries such as ou=certificateRepository.

This makes sense; shall do.

Lets also use the full name - ou=certificateProfiles, ou=ca, $baseDN There are a few properties which should be specified, either as config parameters in CS.cfg or as constants in ProfileSubsystem.java. The DN above for example, should probably be a config parameter in CS.cfg - something like profile.baseDN or better yet, a property of the ProfileSubsystem CS.cfg entries. Things like the attribute used to store the data, the createAttrs etc. could be made constants in ProfileSubsystem.java.

...

2. Is certProfilesLastModified really necessary? Each LDAP entry already has a modifyTimestamp attributes. To be accurate, the certProfilesLastModified has to be updated whenever the profiles are imported or updated. There seems to be no code for that yet, and there's no guarantee it won't get out of sync (e.g. server crash, direct LDAP modification).

This attribute is intended for use by the mechanism that monitors/polls LDAP for changes and reloads profiles as necessary. There is no code for it yet; if I don't end up using it I will remove it.

Yeah - I think we should use modifyTimeStamp attributes if we can, rather than relying on this extra attribute.

...

3. If certProfilesLastModified is really necessary, instead of adding a separate cn=certProfilesInfo entry just to hold this attribute, we probably can move it into ou=certProfiles either by extending the organizationalUnit object class or adding an auxiliary object class. We probably should make it an optional attribute (i.e. no attribute means never updated).

This is a nice idea. If we keep it, I will do as you suggest.

Agreed.

...

4. I'm not sure if we really need the certProfileIsDefault attribute. There's no guarantee that the value will be accurate because someone could modify the certProfileConfig directly and forget to update certProfileIsDefault. Also, if we need to do any profile upgrade, it should be based on the actual configuration parameters in certProfileConfig, not based on the certProfileIsDefault.

This attribute was intended for use by upgrade scripts. I'm not precious about keeping it, but it may be useful. OTOH, default profile updates are rare enough that all the information for a three-way merge could be included in the upgrade helper. So it can probably be removed as you suggest.

I agree on removing this attribute.

...

5. In LDAPConfigStore let's not use the 'm' prefix for the field names. It's a legacy convention and we should not use it for newer code.

OK yes - no Hungarian notation please!

Also, perhaps rename attrs to createAttrs in LDAPConfigStore The comment is also wrong in line 61 -- not a File Config store. Line 65 decuments an argument cn which is not in the method signature.

...

6. The mInDatabase in LDAPConfigStore doesn't seem to be used anywhere.

Yep, I didn't end up using it. Good catch.

Actually, I caught this too, by looking at the code using eclipse. In general, you should make sure that any code that you are adding does not add any additional warnings.

...

7. In LDAPConfigStore the certProfileConfig attribute is read as binary using getByteValues(). Is there a specific reason? Can we use getStringValues() since the content is a string (i.e. property file)?

The load() function takes in InputStream, which is a byte stream abstraction. getByteValues() result in the least work. If the bytes/chars discrepancy is a cause for concern, I can change the LDAP syntax to Octet String.

+1 for using getStringValues(). Do any of these InputStreams and OutputStreams require close()? I think we need to think about refactoring the DBManager/ LDAPConnection code so that the LDAPConnection implements Closeable. Then we can manage all of these resources using try-with-resources constructions, and not have to worry about finally and nested finally blocks. This should probably be a separate patch, but it would be nice to do that refactoring first, so that we could use the new cleaner code structure here. Is there any synchronization required for the commit() for LDAPConfigStore()?

...

8. The ProfileService constructs the profile DN. I think this level of details should be hidden in the ProfileSubsystem layer. The ProfileService shouldn't need to know where/how the profiles are stored.

This was the simply implementation based on existing usage of ProfileService, i.e. the existing implementation told ProfileSubsystem about file locations. I will do this refactor.

Agreed.

...

9. The ProfileAdminServlet calls ProfileSubsystem.createProfile() but it supplies a config path instead of a DN. As in #8, I think the caller should only specify the profile ID, it's up to the ProfileSubsystem to determine the actual location (i.e. profile DN).

Whups, I missed this class. Will update.

OK

...

10. The mProfileDNs in ProfileSubsystem shouldn't be necessary because we can generate the DN from the profileId since it's a flat tree.

This was motivated by the fact that the caller of ProfileSubsystem.createProfile would specify the DN (see #8). Will remove in the refactor. +1

ProfileSubsystem.createProfileConfig() does nothing now. Lets remove it and its references.

...

11. The profile subsystem index is hard-coded in the following line:

cs.remove("subsystem.1.enabled");

Although this code is modifying the standard CS.cfg, it would be better to do a search the index, then make the modification with the index.

int index = ... find subsystem index where id=profile ... cs.remove("subsystem." + index + ".enabled");

This will avoid problems if we ever add a default subsystem in the future.

Fair enough; will update.

Better still would be some methods to either get/enable/disable specific subsystems. Presumably, these methods would live in CMSEngine.java or some such.

...

12. The profile-show-raw is essentially the same as profile-show, except the output format is different. I'd suggest merging them into profile-show and use an option like --raw to show the raw profile.

About the out-of-band class ID, I think we should just map it into a 'virtual' property in the raw profile (e.g. class_id=...). The property will only exist if the client pulls the raw profile. As already in done the patch, in the database it will be stored in classId attribute instead of certProfileConfig. In the long term all properties will be converted into separate attributes like this, and the 'raw' format should only exist as a legacy interface.

Will assess impact of this change on the API (e.g., it may be acceptable to have `null' classId, in which case it is looked for in the config). It will certainly simplify the "unified" CLI command implementation you suggest.

Agreed - I prefer not to add another CLI command for a different output format. We will already have two formats for profile-show -- xml or json. raw is just another.

...

13. The ProfileResource.retrieveProfileRaw() returns a byte[]. Although it works just fine for this purpose, I think returning a Properties would make it more developer-friendly. That way the output is readily usable, and it takes care of any CR/LF issue across different platforms. Maybe call it getProfileProperties()?

I'll look into this implications and benefits of doing this... once I've done all the other things :)

+1 Lets make it more developer friendly.

...

14. The CAInstallerService.importProfiles() and importProfile() don't need to be static. As virtual methods it will have access to other member attributes/methods already defined in the class and its super classes (e.g. config store). A long term goal is to eliminate the use of static methods in CMS (it's not very modular).

Yep, there's a ConfigStore already available. Will change to non-static.

...

15. Minor. I see two styles of writing the try-catch block in the patch:

try { ... } catch (...) { ... }

and

try { ... } catch (...) { ... }

I think the first one is more common.

Yes - please use first version.

...

16. Minor. Instead of adding the enabled parameter to the existing SubsystemInfo constructor (and having to modify all invocations), it's also possible to create a new constructor with the enabled parameter and call it from the old constructor with enabled set to true.

Will do #15 and #16.

Otherwise, this looks like a great start (and a lot fewer changes than I would have expected). Nice. Ade

...

-- Endi S. Dewata

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

On 8/28/2014 12:30 AM, Fraser Tweedale wrote:

...

Do any of these InputStreams and OutputStreams require close()? I think we need to think about refactoring the DBManager/ LDAPConnection code so that the LDAPConnection implements Closeable. Then we can manage all of these resources using try-with-resources constructions, and not have to worry about finally and nested finally blocks.

This should probably be a separate patch, but it would be nice to do that refactoring first, so that we could use the new cleaner code structure here.

LDAPConnection is provided by ldapjdk so I don't think we can modify that class.

We could define a "wrapper" class that implements the desired behaviour. It could contain an LDAPConnection and the factory and in its close() method return the connection to the factory. The code that uses it could then just get the LDAPConnection out of the wrapper, resulting in few changes, and ILdapConnFactory (and its implementations) would learn a new method that returns one of these "wrapped" connections instead of a "bare" LDAPConnection.

If this sounds like a reasonable approach, I will prototype this design.

I think to be a proper wrapper it should wrap the LDAP operations too (e.g. search(), add()) so we don't have to unwrap the original LDAPConnection. I'm not sure if we should implement this in Dogtag though. This change really belongs in the LDAPJDK itself.

...

Is there any synchronization required for the commit() for LDAPConfigStore()?

The PropConfigStore load() and save() methods are synchronised. I hope that my expectation that the LDAP server will serialize concurrent modifications to an entry is reasonably held. Let me know if I am wrong about that, and suggested approach(es) to addressing it.

The code is probably fine for now. Later we probably want to improve it. Given that the certProfileConfig attribute contains the entire profile properties, the chance that people overwrite each other's modification can be higher. A better solution is to separate the properties into separate attributes and only update the attributes that have changed. We can also use REST's ETag to make sure we're not making modifications based on outdated data. This will only work if all changes are coming through REST though. Some more comments: 17. In LDAPConfigStore.save() the writer.close() might not be called if there's an exception. I don't know how likely this may happen, but to be safe we should put close() in a finally block, or use try-with-resources. 18. In LDAPConfigStore.commit() the nested try-catch block might not be necessary. If the LDAPException is something other than NO_SUCH_OBJECT it could throw an EBaseException at that point. 19. In ProfileSubsystem.init() while loading the profiles if one of the class ID's is invalid (e.g. due to typo) the loop will break and the subsystem initialization will fail. I'm not sure about the implication if that happens or how likely, but we probably should just disable the failed profiles and continue loading the rest. People will find the error when they try to use the profile but it's not available. 20. Similar issue with CAInstallerService.importProfiles(), what happens if one profile fails to import? This is probably less likely to happen since it's loading the default profiles. 21. The ProfileSubsystem.profileDN() probably should be called getProfileDN() or createProfileDN() to follow Java conventions. 22. In new code like CMSEngine.getDynSubsystemNames() we should use the light-weight ArrayList or LinkedList instead of Vector. 23. Since WarningListener.mEnabled is new, it shouldn't use the Hungarian notation. -- Endi S. Dewata

On Fri, Aug 08, 2014 at 02:14:21PM -0400, Ade Lee wrote:

On Thu, 2014-08-07 at 17:15 +1000, Fraser Tweedale wrote:

...

On Wed, Jul 30, 2014 at 04:13:33PM -0500, Endi Sukma Dewata wrote:

...

On 7/23/2014 2:09 AM, Fraser Tweedale wrote:

...

On Wed, Jul 23, 2014 at 11:27:11AM +1000, Fraser Tweedale wrote:

...

Along with LDAP profiles, we will be adding modules to the CLI for adding and editing profiles in the ConfigStore format that was used for file-based profiles. For more info, see:

http://pki.fedoraproject.org/wiki/LDAP_Profile_Storage#Command-line_utilitie...

There is an existing CLI for adding and modifying profiles, in the XML format, e.g. ``pki ca profile add caCustomProfile.xml``. The XML format carries information including the profile ID and class_id, but these data must be supplied out-of-band when dealing with the ConfigStore format.

Because of this, I intend to:

- add new commands to the existing profile CLI for working with the "raw" (i.e., ConfigStore) format, e.g. "edit-raw", "add-raw". Where necessary, these commands will take compulsory ``--profile-id`` and/or ``--class-id`` arguments, to account for the absense of such information in the profile ConfigStore format;

and

- transport this information in the XML format - not in the "raw" format - so that it will be unnecessary to make changes to ProfileClient or the ProfileService API.

As usual, I welcome feedback - especially if you feel I am going the wrong way ^_^

An update: due to the various Profile and ProfileInput/Output/Policy classes not being distributed in the pki-tools package (or dependencies thereof), I /did/ end up adding methods to the REST API.

The `pki ca profile show-raw <profileId>` command is implemented in the attached patch 0009. I expected I will complete the edit-raw and add-raw commands tomorrow, and after that will move on to testing replication and polling/monitoring for changes to profiles.

Cheers,

Fraser

Some comments/questions:

Thanks for the review/feedback Endi. Responses inline.

...

1. About the the new ou=certProfiles entry, We probably should move it into the ou=ca subtree because that subtree already contains other CA-specific LDAP entries such as ou=certificateRepository.

This makes sense; shall do.

Lets also use the full name - ou=certificateProfiles, ou=ca, $baseDN

There are a few properties which should be specified, either as config parameters in CS.cfg or as constants in ProfileSubsystem.java.

The DN above for example, should probably be a config parameter in CS.cfg - something like profile.baseDN or better yet, a property of the ProfileSubsystem CS.cfg entries.

Things like the attribute used to store the data, the createAttrs etc. could be made constants in ProfileSubsystem.java.

...

...

2. Is certProfilesLastModified really necessary? Each LDAP entry already has a modifyTimestamp attributes. To be accurate, the certProfilesLastModified has to be updated whenever the profiles are imported or updated. There seems to be no code for that yet, and there's no guarantee it won't get out of sync (e.g. server crash, direct LDAP modification).

This attribute is intended for use by the mechanism that monitors/polls LDAP for changes and reloads profiles as necessary. There is no code for it yet; if I don't end up using it I will remove it.

Yeah - I think we should use modifyTimeStamp attributes if we can, rather than relying on this extra attribute.

...

...

3. If certProfilesLastModified is really necessary, instead of adding a separate cn=certProfilesInfo entry just to hold this attribute, we probably can move it into ou=certProfiles either by extending the organizationalUnit object class or adding an auxiliary object class. We probably should make it an optional attribute (i.e. no attribute means never updated).

This is a nice idea. If we keep it, I will do as you suggest.

Agreed.

...

...

4. I'm not sure if we really need the certProfileIsDefault attribute. There's no guarantee that the value will be accurate because someone could modify the certProfileConfig directly and forget to update certProfileIsDefault. Also, if we need to do any profile upgrade, it should be based on the actual configuration parameters in certProfileConfig, not based on the certProfileIsDefault.

This attribute was intended for use by upgrade scripts. I'm not precious about keeping it, but it may be useful. OTOH, default profile updates are rare enough that all the information for a three-way merge could be included in the upgrade helper. So it can probably be removed as you suggest.

I agree on removing this attribute.

...

...

5. In LDAPConfigStore let's not use the 'm' prefix for the field names. It's a legacy convention and we should not use it for newer code.

OK yes - no Hungarian notation please!

Also, perhaps rename attrs to createAttrs in LDAPConfigStore The comment is also wrong in line 61 -- not a File Config store. Line 65 decuments an argument cn which is not in the method signature.

...

...

6. The mInDatabase in LDAPConfigStore doesn't seem to be used anywhere.

Yep, I didn't end up using it. Good catch.

Actually, I caught this too, by looking at the code using eclipse. In general, you should make sure that any code that you are adding does not add any additional warnings.

...

...

7. In LDAPConfigStore the certProfileConfig attribute is read as binary using getByteValues(). Is there a specific reason? Can we use getStringValues() since the content is a string (i.e. property file)?

The load() function takes in InputStream, which is a byte stream abstraction. getByteValues() result in the least work. If the bytes/chars discrepancy is a cause for concern, I can change the LDAP syntax to Octet String.

+1 for using getStringValues().

Do any of these InputStreams and OutputStreams require close()? I think we need to think about refactoring the DBManager/ LDAPConnection code so that the LDAPConnection implements Closeable. Then we can manage all of these resources using try-with-resources constructions, and not have to worry about finally and nested finally blocks.

This should probably be a separate patch, but it would be nice to do that refactoring first, so that we could use the new cleaner code structure here.

Is there any synchronization required for the commit() for LDAPConfigStore()?

...

...

8. The ProfileService constructs the profile DN. I think this level of details should be hidden in the ProfileSubsystem layer. The ProfileService shouldn't need to know where/how the profiles are stored.

This was the simply implementation based on existing usage of ProfileService, i.e. the existing implementation told ProfileSubsystem about file locations. I will do this refactor.

Agreed.

...

...

9. The ProfileAdminServlet calls ProfileSubsystem.createProfile() but it supplies a config path instead of a DN. As in #8, I think the caller should only specify the profile ID, it's up to the ProfileSubsystem to determine the actual location (i.e. profile DN).

Whups, I missed this class. Will update.

OK

...

...

10. The mProfileDNs in ProfileSubsystem shouldn't be necessary because we can generate the DN from the profileId since it's a flat tree.

This was motivated by the fact that the caller of ProfileSubsystem.createProfile would specify the DN (see #8). Will remove in the refactor. +1

ProfileSubsystem.createProfileConfig() does nothing now. Lets remove it and its references.

...

...

11. The profile subsystem index is hard-coded in the following line:

cs.remove("subsystem.1.enabled");

Although this code is modifying the standard CS.cfg, it would be better to do a search the index, then make the modification with the index.

int index = ... find subsystem index where id=profile ... cs.remove("subsystem." + index + ".enabled");

This will avoid problems if we ever add a default subsystem in the future.

Fair enough; will update.

Better still would be some methods to either get/enable/disable specific subsystems. Presumably, these methods would live in CMSEngine.java or some such.

...

...

12. The profile-show-raw is essentially the same as profile-show, except the output format is different. I'd suggest merging them into profile-show and use an option like --raw to show the raw profile.

About the out-of-band class ID, I think we should just map it into a 'virtual' property in the raw profile (e.g. class_id=...). The property will only exist if the client pulls the raw profile. As already in done the patch, in the database it will be stored in classId attribute instead of certProfileConfig. In the long term all properties will be converted into separate attributes like this, and the 'raw' format should only exist as a legacy interface.

Will assess impact of this change on the API (e.g., it may be acceptable to have `null' classId, in which case it is looked for in the config). It will certainly simplify the "unified" CLI command implementation you suggest.

Agreed - I prefer not to add another CLI command for a different output format. We will already have two formats for profile-show -- xml or json. raw is just another.

...

...

13. The ProfileResource.retrieveProfileRaw() returns a byte[]. Although it works just fine for this purpose, I think returning a Properties would make it more developer-friendly. That way the output is readily usable, and it takes care of any CR/LF issue across different platforms. Maybe call it getProfileProperties()?

I'll look into this implications and benefits of doing this... once I've done all the other things :)

+1 Lets make it more developer friendly.

SimpleProperties and PropConfigStore currently live in com.netscape.cmscore.base. Is there any objection to moving them to com.netscape.certsrv.base so that they are available to the CLI code? This item is the last of the review items (including Endi's subsequent review) blocking the next cut of the patchset. Cheers, Fraser

...

...

14. The CAInstallerService.importProfiles() and importProfile() don't need to be static. As virtual methods it will have access to other member attributes/methods already defined in the class and its super classes (e.g. config store). A long term goal is to eliminate the use of static methods in CMS (it's not very modular).

Yep, there's a ConfigStore already available. Will change to non-static.

...

15. Minor. I see two styles of writing the try-catch block in the patch:

try { ... } catch (...) { ... }

and

try { ... } catch (...) { ... }

I think the first one is more common.

Yes - please use first version.

...

...

16. Minor. Instead of adding the enabled parameter to the existing SubsystemInfo constructor (and having to modify all invocations), it's also possible to create a new constructor with the enabled parameter and call it from the old constructor with enabled set to true.

Will do #15 and #16.

Otherwise, this looks like a great start (and a lot fewer changes than I would have expected). Nice.

Ade

...

...

-- Endi S. Dewata

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

On Tue, Sep 02, 2014 at 11:38:42PM -0500, Endi Sukma Dewata wrote:

On 9/2/2014 9:16 PM, Fraser Tweedale wrote:

...

...

...

...

13. The ProfileResource.retrieveProfileRaw() returns a byte[]. Although it works just fine for this purpose, I think returning a Properties would make it more developer-friendly. That way the output is readily usable, and it takes care of any CR/LF issue across different platforms. Maybe call it getProfileProperties()?

I'll look into this implications and benefits of doing this... once I've done all the other things :)

+1 Lets make it more developer friendly.

SimpleProperties and PropConfigStore currently live in com.netscape.cmscore.base. Is there any objection to moving them to com.netscape.certsrv.base so that they are available to the CLI code?

This item is the last of the review items (including Endi's subsequent review) blocking the next cut of the patchset.

Do those classes have dependencies on other server-side classes? I was thinking to simply use java.util.Properties. Or maybe just a String then the ProfileClient parses it into a Properties.

Aha, I was not aware of java.util.Properties. My immediate use case doesn't require use of the sub-configs, so Properties would suffice. Due to the heirarchical nature of profile config I suspect we might *eventually* want PropConfigStore, but YAGNI for now, I guess. In regards to moving SimpleProperties and PropConfigStore, there is one other class - SourceConfigStore - that would have to have been moved. AFAICT there are no other dependencies. On the subject of SimpleProperties: how does our SimpleProperties class differ from java.util.Properties? A cursory examination reveals only two differences: SimpleProperties' hashCode() and equals() methods take the `defaults' into account, whereas Properties inherits this methods from java.util.HashTable. The only place SimpleProperties is used is in SourceConfigStore (which extends it), and the only place SourceConfigStore is used is in PropConfigStore (protected SourceConfigStore mSource). Neither PropConfigStore nor its subclasses invoke mSource.equals() or .hashCode() so it seems - from this brief examination and without the knowledge of history - that we could possibly get rid of SimpleProperties? If there is an important reason why SimpleProperties exists, I look forward to learning about it :) Regards, Fraser

-- Endi S. Dewata

On Wed, Sep 03, 2014 at 10:36:21AM -0500, Endi Sukma Dewata wrote:

On 9/3/2014 12:25 AM, Fraser Tweedale wrote:

...

...

...

SimpleProperties and PropConfigStore currently live in com.netscape.cmscore.base. Is there any objection to moving them to com.netscape.certsrv.base so that they are available to the CLI code?

This item is the last of the review items (including Endi's subsequent review) blocking the next cut of the patchset.

Do those classes have dependencies on other server-side classes? I was thinking to simply use java.util.Properties. Or maybe just a String then the ProfileClient parses it into a Properties.

Aha, I was not aware of java.util.Properties. My immediate use case doesn't require use of the sub-configs, so Properties would suffice. Due to the heirarchical nature of profile config I suspect we might *eventually* want PropConfigStore, but YAGNI for now, I guess.

In regards to moving SimpleProperties and PropConfigStore, there is one other class - SourceConfigStore - that would have to have been moved. AFAICT there are no other dependencies.

On the subject of SimpleProperties: how does our SimpleProperties class differ from java.util.Properties? A cursory examination reveals only two differences: SimpleProperties' hashCode() and equals() methods take the `defaults' into account, whereas Properties inherits this methods from java.util.HashTable.

The only place SimpleProperties is used is in SourceConfigStore (which extends it), and the only place SourceConfigStore is used is in PropConfigStore (protected SourceConfigStore mSource). Neither PropConfigStore nor its subclasses invoke mSource.equals() or .hashCode() so it seems - from this brief examination and without the knowledge of history - that we could possibly get rid of SimpleProperties?

If there is an important reason why SimpleProperties exists, I look forward to learning about it :)

I don't know the history either, but the SimpleProperties allows you to specify the default values of some properties so what you get is the effective values. This might be useful for the server because the server works using the effective property values, but I'm not sure if the current code is actually using it. On the client side we should be dealing with the configured properties only.

There are probably some other differences such as adding a header on the top of the file and the property value encoding. I don't think we need it on the client side though. Also note that some of these codes are quite old, and newer/better utility classes might have been added into the standard/common Java libraries since then, so in some cases it might be better to replace it if possible. Usually the hard part is testing it to make sure we're not breaking the existing functionality.

`Properties' has both default values and header comments, FWIW. Anyhow I'll leave the refactor for now and file a ticket, and proceed with Properties for the client-side.

-- Endi S. Dewata

Third cut of the LDAP profiles feature. All comments from previous reviews have been addressed in this patchset. The switch from byte[] to Properties in the ProfileClient API (item #13 from Endi's first review) is currently in a separte patch (#0014) to make it clear what had to happen for this change, and to make it easy to change or remove if there are problems. If it's preferred to squash it when acked, let me know (or it can stay separate - I don't mind either way). Cheers, Fraser

On Thu, Oct 09, 2014 at 09:47:24AM -0500, Endi Sukma Dewata wrote:

On 9/19/2014 2:46 AM, Fraser Tweedale wrote:

...

Third cut of the LDAP profiles feature. All comments from previous reviews have been addressed in this patchset.

The switch from byte[] to Properties in the ProfileClient API (item #13 from Endi's first review) is currently in a separte patch (#0014) to make it clear what had to happen for this change, and to make it easy to change or remove if there are problems. If it's preferred to squash it when acked, let me know (or it can stay separate - I don't mind either way).

Cheers,

Fraser

Sorry for the delay. I have some more comments below.

Patch #4-3:

1. What do you think about renaming classId into certProfileClassID? Is there a plan to use the classId for something other than profile?

The classId represents a Java class. It is not yet used elsewhere, but if we move to finer-grained LDAP profile config schema it will be used for the inputs/outputs/defaults also, therefore I will leave it as-is.

2. Let's use real descriptions instead of generic 'CMS defined attribute/class', for example: * classId: Certificate profile class ID * certProfileConfig: Certificate profile configuration * certProfile: Certificate profile

Will be adjusted in next patch revision.

No major issue, but see comments about database upgrade below. ACK.

Patch #5-3:

1. The commit() parameter "createBackup" doesn't match the @param backup.

Fixed the javadoc.

Nothing major. ACK.

Patch #6-3:

1. The LDAP-based profile subsystem is fine, but it might not run on existing installation that still uses file-based profile. Are we going to (a) support both types and provide a migration tool that can be run at a later time, or (b) require the people to migrate to LDAP before they can use the new code?

Option (a) is more flexible, but we would need to keep both the old ProfileSubsystem and the new LDAPProfileSubsystem, and then switch the class when we migrate to LDAP. With (b) once the RPM is upgraded, the database has to be upgraded and the profiles have to be migrated before we can restart the server, otherwise the profile subsystem will not work. See also comments about profile migration tool below.

Need to have a deeper discussion of the implications. Will create a new thread for this topic, or maybe we can discuss in next ipa-cs team meeting.

2. If the default profile subsystem is changed to LDAP, we shouldn't need to copy the profile files anymore to the instance. Look for "pki_subsystem_profiles_path" in subsystem_layout.py.

3. Are the profile.* properties in CS.cfg still used by any code (other than importProfiles()) if we use LDAP-based profile? If not, we probably should remove them from CS.cfg and move them into the corresponding profile files (just like the raw format), and use file list instead of profile.list during import.

If we go with option b) above, this will clean things up a bit. If we go with option a) we either keep it or make more changes to move the classid into the file-based representation.

I'd suggest we use option (a), change the default to LDAP, and remove profile.* properties after we have the database upgrade and profile migration tool ready.

Patch #7-3:

1. By default the subsystems are enabled, so in setSubsystemEnabled() if enabled=true we can actually remove the PROP_ENABLED property to keep it consistent with other subsystems that are enabled by default.

Nothing major. ACK.

Patch #8-3: 1. There's an unused variable:

String className = info.getClassName();

Removed it.

Nothing major. ACK.

Patch #9-3:

1. The ca-profile-add --raw works, but the regular ca-profile-add throws an error. If this is an existing problem please open a ticket.

This is a regression. Will have it fixed in next patch revision.

2. The ca-profile-show --raw prepends a message to the raw output, for example:

--------------------- Profile "caAdminCert" --------------------- ...

I think the message should not be included in the raw output so it can be redirected into a file, for example:

pki ca-profile-show caAdminCert --raw > caAdminCert.cfg

Was included it for consistency with non-raw output. Happy to see it go.

3. On the server side we should use CMS.debug(e) to log exceptions instead of e.printStackTrace() so that the amount of logging can be controlled.

OK

4. On the CLI side it would be better to throw an exception on execution error (not option error), or just don't catch the original exception, rather than printing an error message. The exception will be caught and displayed properly by the MainCLI, and we can see the full stack trace in verbose mode.

5. The code that reads the raw profile data in ProfileAddCLI and ProfileModifyCLI probably can be refactored into ProfileData.loadRawProfile(). The ProfileCLI.readProfileFromFile() probably can also be moved into ProfileData.loadXMLProfile() for consistency.

OK

6. The ca-profile-edit can also support both formats: XML and raw.

I think #1 and #2 should be fixed. The others can be addressed later.

Patch #14-3:

1. The ProfileResource interface still uses byte[]. I originally thought it will use Properties as well. Is that possible?

2. Related to #1, the response content-type is application/xml, but we should be returning text/plain or application/octet-stream. I need to take a look at the REST framework to see how we can support it.

These issues require further investigation. The code itself is fine. ACK.

I have found RESTeasy to be anything but. Thanks for ACK; happy to revisit later with someone who groks RESTeasy more than I.

Other comments:

1. For database upgrade, we need to add the profile schema and the profile base entry into the database. We don't have a database upgrade framework yet, but I suppose we can prepare a script that will connect to the database and make the modifications. Later we can convert that script into an proper database upgrade scriptlet.

2. For profile migration, we need to read profile files in the existing instance, add it to LDAP, and remove the profile files and the profile properties in CS.cfg. If it's a one-way migration the migration tool can be implemented as database upgrade scriptlet too. If it's two-way, it would have to be implemented as a stand-alone tool.

3. Since these patches are targeted for 10.3, check with Matt/Ade to make sure the repo is branched properly before you push.

4. The patches can be pushed separately or squashed, it doesn't really matter. Just make sure that each patch pushed contains a complete set of changes so the build doesn't break. This would be important if we need to cherry-pick or revert some patches later.

-- Endi S. Dewata

Whups, it helps to attach the patches! On Thu, Nov 20, 2014 at 04:27:48PM +1000, Fraser Tweedale wrote:

Fourth cut of the feature.

Please review w.r.t. the issues below.

The big issue mentioned in the review but *not* dealt with in this patchset is support for using file-based profiles OR LDAP-based profiles. This will involve copying the new ProfileSystem (as it appears in the patches) to a new class ("LDAPProfileSubsystem" I suppose), restoring the original ProfileSubsystem with the file-based behaviour, providing config and mechanism for loading the preferred variant of the subsystem, and reconciling any conflicts. But the implementation of the LDAP-based version of ProfileSubsystem won't be changed once these patches are ACKed.

(See also patch 0015-2 in the other thread)

Cheers,

Fraser

On Wed, Nov 12, 2014 at 03:52:47PM +1000, Fraser Tweedale wrote:

...

On Thu, Oct 09, 2014 at 09:47:24AM -0500, Endi Sukma Dewata wrote:

...

On 9/19/2014 2:46 AM, Fraser Tweedale wrote:

...

Third cut of the LDAP profiles feature. All comments from previous reviews have been addressed in this patchset.

The switch from byte[] to Properties in the ProfileClient API (item #13 from Endi's first review) is currently in a separte patch (#0014) to make it clear what had to happen for this change, and to make it easy to change or remove if there are problems. If it's preferred to squash it when acked, let me know (or it can stay separate - I don't mind either way).

Cheers,

Fraser

Sorry for the delay. I have some more comments below.

Patch #4-3:

1. What do you think about renaming classId into certProfileClassID? Is there a plan to use the classId for something other than profile?

The classId represents a Java class. It is not yet used elsewhere, but if we move to finer-grained LDAP profile config schema it will be used for the inputs/outputs/defaults also, therefore I will leave it as-is.

...

2. Let's use real descriptions instead of generic 'CMS defined attribute/class', for example: * classId: Certificate profile class ID * certProfileConfig: Certificate profile configuration * certProfile: Certificate profile

Will be adjusted in next patch revision.

...

No major issue, but see comments about database upgrade below. ACK.

Patch #5-3:

1. The commit() parameter "createBackup" doesn't match the @param backup.

Fixed the javadoc.

...

Nothing major. ACK.

Patch #6-3:

1. The LDAP-based profile subsystem is fine, but it might not run on existing installation that still uses file-based profile. Are we going to (a) support both types and provide a migration tool that can be run at a later time, or (b) require the people to migrate to LDAP before they can use the new code?

Option (a) is more flexible, but we would need to keep both the old ProfileSubsystem and the new LDAPProfileSubsystem, and then switch the class when we migrate to LDAP. With (b) once the RPM is upgraded, the database has to be upgraded and the profiles have to be migrated before we can restart the server, otherwise the profile subsystem will not work. See also comments about profile migration tool below.

Need to have a deeper discussion of the implications. Will create a new thread for this topic, or maybe we can discuss in next ipa-cs team meeting.

...

2. If the default profile subsystem is changed to LDAP, we shouldn't need to copy the profile files anymore to the instance. Look for "pki_subsystem_profiles_path" in subsystem_layout.py.

3. Are the profile.* properties in CS.cfg still used by any code (other than importProfiles()) if we use LDAP-based profile? If not, we probably should remove them from CS.cfg and move them into the corresponding profile files (just like the raw format), and use file list instead of profile.list during import.

If we go with option b) above, this will clean things up a bit. If we go with option a) we either keep it or make more changes to move the classid into the file-based representation.

...

I'd suggest we use option (a), change the default to LDAP, and remove profile.* properties after we have the database upgrade and profile migration tool ready.

Patch #7-3:

1. By default the subsystems are enabled, so in setSubsystemEnabled() if enabled=true we can actually remove the PROP_ENABLED property to keep it consistent with other subsystems that are enabled by default.

Nothing major. ACK.

Patch #8-3: 1. There's an unused variable:

String className = info.getClassName();

Removed it.

...

Nothing major. ACK.

Patch #9-3:

1. The ca-profile-add --raw works, but the regular ca-profile-add throws an error. If this is an existing problem please open a ticket.

This is a regression. Will have it fixed in next patch revision.

...

2. The ca-profile-show --raw prepends a message to the raw output, for example:

--------------------- Profile "caAdminCert" --------------------- ...

I think the message should not be included in the raw output so it can be redirected into a file, for example:

pki ca-profile-show caAdminCert --raw > caAdminCert.cfg

Was included it for consistency with non-raw output. Happy to see it go.

...

3. On the server side we should use CMS.debug(e) to log exceptions instead of e.printStackTrace() so that the amount of logging can be controlled.

OK

...

4. On the CLI side it would be better to throw an exception on execution error (not option error), or just don't catch the original exception, rather than printing an error message. The exception will be caught and displayed properly by the MainCLI, and we can see the full stack trace in verbose mode.

5. The code that reads the raw profile data in ProfileAddCLI and ProfileModifyCLI probably can be refactored into ProfileData.loadRawProfile(). The ProfileCLI.readProfileFromFile() probably can also be moved into ProfileData.loadXMLProfile() for consistency.

OK

...

6. The ca-profile-edit can also support both formats: XML and raw.

I think #1 and #2 should be fixed. The others can be addressed later.

Patch #14-3:

1. The ProfileResource interface still uses byte[]. I originally thought it will use Properties as well. Is that possible?

2. Related to #1, the response content-type is application/xml, but we should be returning text/plain or application/octet-stream. I need to take a look at the REST framework to see how we can support it.

These issues require further investigation. The code itself is fine. ACK.

I have found RESTeasy to be anything but. Thanks for ACK; happy to revisit later with someone who groks RESTeasy more than I.

...

Other comments:

1. For database upgrade, we need to add the profile schema and the profile base entry into the database. We don't have a database upgrade framework yet, but I suppose we can prepare a script that will connect to the database and make the modifications. Later we can convert that script into an proper database upgrade scriptlet.

2. For profile migration, we need to read profile files in the existing instance, add it to LDAP, and remove the profile files and the profile properties in CS.cfg. If it's a one-way migration the migration tool can be implemented as database upgrade scriptlet too. If it's two-way, it would have to be implemented as a stand-alone tool.

3. Since these patches are targeted for 10.3, check with Matt/Ade to make sure the repo is branched properly before you push.

4. The patches can be pushed separately or squashed, it doesn't really matter. Just make sure that each patch pushed contains a complete set of changes so the build doesn't break. This would be important if we need to cherry-pick or revert some patches later.

-- Endi S. Dewata

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

New patchset for ldap profiles after a long time working on sub-CAs! 4, 5, 6, 7, 8 have been ACKed previously. #6 deserves renewed attention as the original (file-based) ProfileSubsystem has been restored, and the new subsystem now appears as LDAPProfileSubsystem. A new patch (separate email) includes the pkispawn logic to specify whether to use file or ldap profiles. Comments on patch 9 inline below. Cheers, Fraser On Wed, Dec 03, 2014 at 01:59:24PM +0700, Endi Sukma Dewata wrote:

On 11/20/2014 1:30 PM, Fraser Tweedale wrote:

...

...

Fourth cut of the feature.

Please review w.r.t. the issues below.

The big issue mentioned in the review but *not* dealt with in this patchset is support for using file-based profiles OR LDAP-based profiles. This will involve copying the new ProfileSystem (as it appears in the patches) to a new class ("LDAPProfileSubsystem" I suppose), restoring the original ProfileSubsystem with the file-based behaviour, providing config and mechanism for loading the preferred variant of the subsystem, and reconciling any conflicts. But the implementation of the LDAP-based version of ProfileSubsystem won't be changed once these patches are ACKed.

(See also patch 0015-2 in the other thread)

Patches #4-4, #5-4, #7-4, #8-4 are ACKed.

Patch #6-4 is ACKed assuming the file-based profiles will be restored later.

Patch #9-4 has some issues:

1. Since the profileId and classId are virtual properties and have their own LDAP attributes, they should not be stored in certProfileConfig attribute when a profile is added/modified.

Done. Because the profile configStore is written and read directly by the ProfileService, the solution to achieve this is also in that class.

2. Try exporting a profile, change the ID, then add it back to the server. If you use the XML format, the new profile will not have an enable and enableBy properties. If you use the RAW format, the new profile will have enable=false and retain the old enableBy. To be more consistent these properties should also be removed during add/modify. Probably the ProfileSubsystem.disableProfile() can be changed to remove PROP_ENABLE (and PROP_ENABLE_BY too) instead of setting it to "false".

Deferred. Will file ticket.

Similarly, in CMSEngine.setSubsystemEnabled() you could also remove PROP_ENABLED instead of setting it to "false". That way the CS.cfg will be cleaner and more consistent.

3. In ProfileEditCLI a missing "enable" property is incorrectly interpreted as enabled.

String enabled = orig.getProperty("enable"); if (enabled == null || !enabled.equalsIgnoreCase("false")) { System.err.println("Error: Cannot edit profile. Profile must be disabled."); System.exit(-1); }

As a comparison, in ProfileSubsystem.isProfileEnable() a missing "enable" is considered disabled.

if (enable == null || enable.equals("false")) return false; else return true;

See also the ticket I opened below.

Done. Also fixed the ticket (#1220) while I was at it.

4. If an error happens in ProfileEditCLI, the temporary file may not be removed. To make sure it's removed we should use a finally block, or use File.deleteOnExit(). See https://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit().

Done.

5. The Properties object used in ProfileClient doesn't order the properties since it's based on Hashtable so it will be hard to compare different outputs. Maybe we should use a Map<String, String> in the method signature, and use a TreeMap object to store the properties.

Deferred. Will file ticket.

6. The ProfileAddCLI swallows the exception so if there's an error (e.g. wrong file format) it will only print a message, it won't display the stack trace in verbose mode. It should let the exception be handled by the main program.

Deferred. Will address before merge.

I think #1, #3, and #4 should be fixed. The others can be addressed later. I also opened some tickets for existing issues found during testing:

https://fedorahosted.org/pki/ticket/1218 https://fedorahosted.org/pki/ticket/1220

-- Endi S. Dewata

On Mon, Apr 06, 2015 at 08:26:57PM -0500, Endi Sukma Dewata wrote:

On 3/30/2015 2:26 AM, Fraser Tweedale wrote:

...

New patchset for ldap profiles after a long time working on sub-CAs!

4, 5, 6, 7, 8 have been ACKed previously. #6 deserves renewed attention as the original (file-based) ProfileSubsystem has been restored, and the new subsystem now appears as LDAPProfileSubsystem.

ACK for patch #6, just minor things:

Thanks; comments inline.

1. The CMS_PROFILE_DELETE_DATABASEERROR probably can be renamed to a more generic CMS_PROFILE_DELETE_ERROR because based on the message itself (i.e. "Failed to delete profile") the database is irrelevant.

2. The CMS_PROFILE_DELETE_UNKNOWNPROFILE probably can be replaced with CMS_PROFILE_DELETE_ERROR as well since the message is actually used to describe unknown (i.e. generic) server problem, not unknown profile.

3. Please chain the exceptions, for example:

} catch (EBaseException e) { throw new EProfileException("CMS_PROFILE_DELETE_ERROR", e); }

1, 2 and 3 have been addressed (trivial, so I'll spare you yet another patch to review).

...

Comments on patch 9 inline below.

...

1. Since the profileId and classId are virtual properties and have their own LDAP attributes, they should not be stored in certProfileConfig attribute when a profile is added/modified.

Done. Because the profile configStore is written and read directly by the ProfileService, the solution to achieve this is also in that class.

I think the classId provided during createProfileRaw() and modifyProfileRaw() should be stored in both mProfileClassIds Hashtable and in classId LDAP attribute.

The classId gets stored in mProfileClassIds, and in the LDAP attribute, by the createProfile() method.

...

...

2. Try exporting a profile, change the ID, then add it back to the server. If you use the XML format, the new profile will not have an enable and enableBy properties. If you use the RAW format, the new profile will have enable=false and retain the old enableBy. To be more consistent these properties should also be removed during add/modify. Probably the ProfileSubsystem.disableProfile() can be changed to remove PROP_ENABLE (and PROP_ENABLE_BY too) instead of setting it to "false".

Deferred. Will file ticket.

Filed https://fedorahosted.org/pki/ticket/1335

...

...

Similarly, in CMSEngine.setSubsystemEnabled() you could also remove PROP_ENABLED instead of setting it to "false". That way the CS.cfg will be cleaner and more consistent.

3. In ProfileEditCLI a missing "enable" property is incorrectly interpreted as enabled.

String enabled = orig.getProperty("enable"); if (enabled == null || !enabled.equalsIgnoreCase("false")) { System.err.println("Error: Cannot edit profile. Profile must be disabled."); System.exit(-1); }

As a comparison, in ProfileSubsystem.isProfileEnable() a missing "enable" is considered disabled.

if (enable == null || enable.equals("false")) return false; else return true;

See also the ticket I opened below.

Done. Also fixed the ticket (#1220) while I was at it.

...

4. If an error happens in ProfileEditCLI, the temporary file may not be removed. To make sure it's removed we should use a finally block, or use File.deleteOnExit(). See https://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit().

Done.

...

5. The Properties object used in ProfileClient doesn't order the properties since it's based on Hashtable so it will be hard to compare different outputs. Maybe we should use a Map<String, String> in the method signature, and use a TreeMap object to store the properties.

Deferred. Will file ticket.

Filed https://fedorahosted.org/pki/ticket/1336

...

...

6. The ProfileAddCLI swallows the exception so if there's an error (e.g. wrong file format) it will only print a message, it won't display the stack trace in verbose mode. It should let the exception be handled by the main program.

Deferred. Will address before merge.

Please check this issue in LDAPProfileSubsystem.createProfile() as well.

Have un-swallowed these exceptions.

Once #1 is fixed, it's ACKed. I think it's safe to push these patches to the master branch (10.3) since it doesn't break the current code.

I also created this ticket to enhance the profile CLI in the future: https://fedorahosted.org/pki/ticket/1331

-- Endi S. Dewata

Cheers! Fraser

On Tue, Apr 07, 2015 at 09:23:57PM -0500, Endi Sukma Dewata wrote:

On 4/7/2015 7:30 PM, Fraser Tweedale wrote:

...

...

...

...

1. Since the profileId and classId are virtual properties and have their own LDAP attributes, they should not be stored in certProfileConfig attribute when a profile is added/modified.

Done. Because the profile configStore is written and read directly by the ProfileService, the solution to achieve this is also in that class.

I think the classId provided during createProfileRaw() and modifyProfileRaw() should be stored in both mProfileClassIds Hashtable and in classId LDAP attribute.

The classId gets stored in mProfileClassIds, and in the LDAP attribute, by the createProfile() method.

I think I tried changing the classId using the ca-profile-edit command and it didn't get changed in the LDAP entry. Could you verify this? If it works that's good. If it doesn't, let's fix this in another patch. Thanks.

Ah, yes. The classId is never changed, I this this was the existing behaviour. I'll confirm and if so, I'll file a ticket. Cheers, Fraser

-- Endi S. Dewata

On Wed, Apr 08, 2015 at 10:30:37AM +1000, Fraser Tweedale wrote:

On Mon, Apr 06, 2015 at 08:26:57PM -0500, Endi Sukma Dewata wrote:

...

On 3/30/2015 2:26 AM, Fraser Tweedale wrote:

...

New patchset for ldap profiles after a long time working on sub-CAs!

4, 5, 6, 7, 8 have been ACKed previously. #6 deserves renewed attention as the original (file-based) ProfileSubsystem has been restored, and the new subsystem now appears as LDAPProfileSubsystem.

ACK for patch #6, just minor things:

Thanks; comments inline.

...

1. The CMS_PROFILE_DELETE_DATABASEERROR probably can be renamed to a more generic CMS_PROFILE_DELETE_ERROR because based on the message itself (i.e. "Failed to delete profile") the database is irrelevant.

2. The CMS_PROFILE_DELETE_UNKNOWNPROFILE probably can be replaced with CMS_PROFILE_DELETE_ERROR as well since the message is actually used to describe unknown (i.e. generic) server problem, not unknown profile.

3. Please chain the exceptions, for example:

} catch (EBaseException e) { throw new EProfileException("CMS_PROFILE_DELETE_ERROR", e); }

1, 2 and 3 have been addressed (trivial, so I'll spare you yet another patch to review).

...

...

Comments on patch 9 inline below.

...

1. Since the profileId and classId are virtual properties and have their own LDAP attributes, they should not be stored in certProfileConfig attribute when a profile is added/modified.

Done. Because the profile configStore is written and read directly by the ProfileService, the solution to achieve this is also in that class.

I think the classId provided during createProfileRaw() and modifyProfileRaw() should be stored in both mProfileClassIds Hashtable and in classId LDAP attribute.

The classId gets stored in mProfileClassIds, and in the LDAP attribute, by the createProfile() method.

...

...

...

2. Try exporting a profile, change the ID, then add it back to the server. If you use the XML format, the new profile will not have an enable and enableBy properties. If you use the RAW format, the new profile will have enable=false and retain the old enableBy. To be more consistent these properties should also be removed during add/modify. Probably the ProfileSubsystem.disableProfile() can be changed to remove PROP_ENABLE (and PROP_ENABLE_BY too) instead of setting it to "false".

Deferred. Will file ticket.

Filed https://fedorahosted.org/pki/ticket/1335

...

...

...

Similarly, in CMSEngine.setSubsystemEnabled() you could also remove PROP_ENABLED instead of setting it to "false". That way the CS.cfg will be cleaner and more consistent.

3. In ProfileEditCLI a missing "enable" property is incorrectly interpreted as enabled.

String enabled = orig.getProperty("enable"); if (enabled == null || !enabled.equalsIgnoreCase("false")) { System.err.println("Error: Cannot edit profile. Profile must be disabled."); System.exit(-1); }

As a comparison, in ProfileSubsystem.isProfileEnable() a missing "enable" is considered disabled.

if (enable == null || enable.equals("false")) return false; else return true;

See also the ticket I opened below.

Done. Also fixed the ticket (#1220) while I was at it.

...

4. If an error happens in ProfileEditCLI, the temporary file may not be removed. To make sure it's removed we should use a finally block, or use File.deleteOnExit(). See https://docs.oracle.com/javase/7/docs/api/java/io/File.html#deleteOnExit().

Done.

...

5. The Properties object used in ProfileClient doesn't order the properties since it's based on Hashtable so it will be hard to compare different outputs. Maybe we should use a Map<String, String> in the method signature, and use a TreeMap object to store the properties.

Deferred. Will file ticket.

Filed https://fedorahosted.org/pki/ticket/1336

...

...

...

6. The ProfileAddCLI swallows the exception so if there's an error (e.g. wrong file format) it will only print a message, it won't display the stack trace in verbose mode. It should let the exception be handled by the main program.

Deferred. Will address before merge.

Please check this issue in LDAPProfileSubsystem.createProfile() as well.

Have un-swallowed these exceptions.

...

Once #1 is fixed, it's ACKed. I think it's safe to push these patches to the master branch (10.3) since it doesn't break the current code.

I also created this ticket to enhance the profile CLI in the future: https://fedorahosted.org/pki/ticket/1331

-- Endi S. Dewata

Cheers! Fraser

Pushed to master 9d1d02c Add schema for LDAP-based profiles 1b44dcb Add LDAPConfigStore class 4785f08 Add LDAPProfileSubsystem to store profiles in LDAP 2af78ce Add ability to enable/disable dynamic subsystems e4869e6 Import profiles when spawning CA instance c4ee90c Update pki-profile CLI commands to work with "raw" format