[Pki-devel] CLI for editing profiles

[Endi Sukma Dewata]

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_utilities
>>
>> 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