[Pki-devel] CLI for editing profiles

[Fraser Tweedale]

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