Re: [Pki-devel] CLI for editing profiles

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_utili... > > >> > > >>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(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel