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