2:26 a.m.
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