Improvements to LdapPublisher #940
Conversation
Several improvements
Some additional testcases added
Extra check included into createIntermidiateNodes()
1. createIntermediateNodes() 2. revokecerificate()
|
Thanks. A question, are these improvements in use by you? I consider LDAP a quite legacy feature and want to spend as little time as possible on it. Great that you added JUnit tests, that helps a lot. The good point list makes it possible to break it down into more unique parts. |
| return new LDAPAttribute("objectclass", new String[] { "top", "domain" }); | ||
| } else if(lowCaseField.equals("c")) { | ||
| return new LDAPAttribute("objectclass", new String[] { "top", "country" }); | ||
| } else if(lowCaseField.equals("l") || lowCaseField.equals("st")) { |
There was a problem hiding this comment.
locality and state is not the same, you can have both, which is not uncommon in the US (but should really be avoided as best practice)
There was a problem hiding this comment.
In LDAP, there is an objectClass=locality, but there is not one for State/Province. Hence this code should return objectClass=locality for both "l" and "st." Refer: https://www.rfc-editor.org/rfc/rfc4519#section-3.7
| } catch (CertificateEncodingException e) { | ||
| // Exception should not happen! | ||
| String msg = "Unexpected certificate encoding issue. Cannot remove the certificate from LDAP."; | ||
| log.error(msg); |
There was a problem hiding this comment.
In storeCertificate the same encoding error throws a PublisherException. It would be consistent to do that here as well I think.
There was a problem hiding this comment.
I had originally done just that, but then the I realised throwing an exception would leave the cert on the publishing queue. As the cert is 'dodgy' (it had an encoding issue) then we should remove it from the publishing queue, or it will just get in an endless retry loop. This was my logic in not throwing some publisher exceptions.
| } | ||
| if (removeuser) { | ||
| lc.delete(oldEntry.getDN(), ldapStoreConstraints); | ||
| lc.delete(oldEntry.getDN(), ldapStoreConstraints); |
There was a problem hiding this comment.
I can see that this old code, doesn't it have a strange order? I.e. if you "removeuser", what's the point in first going through all the work of deleting certificates if you are to remove the whole entry? That could be at the top could it not?
There was a problem hiding this comment.
Yes, I thought the same. Although it was useful to re-work the code so it didn't remove all the Uer's certificates.
LDAP is well used in PKI solutions. I would not have thought of it as "legacy". I guess that explains why the LDAP Publishing code didn't quite seem to be at the same high standard as the EJBCA code. |
| if (removeuser) { | ||
| lc.delete(oldEntry.getDN(), ldapStoreConstraints); | ||
| lc.delete(oldEntry.getDN(), ldapStoreConstraints); | ||
| if (log.isDebugEnabled()) { |
There was a problem hiding this comment.
This debug logs the same this as is info logged on the follow row.
There was a problem hiding this comment.
Not quite the same. The Info logs (in english) "LDAP REVOKE: Removed certificate from object: successfully.". This is technicaly true if the certificate is simply removed, or if the User DN is deleted. There was not an existing localized message to cover the removal of the User entry, so I've kept the existing Info log, but added more debug logs to fully explain the actions.
| // Default Values | ||
|
|
||
| protected static final String HOSTNAMES = "hostname"; | ||
| protected static final String HOSTNAMES = "hostnames"; |
There was a problem hiding this comment.
imho this and baswdn are not worth an upgrade. I'd rather leave them as is.
There was a problem hiding this comment.
Sure, although the fix is now done along with an 'upgrade' from any previous settings.
2 participants
What is changed?
In testing with LdapPublisher, I found a few issues and provide some fixes as described below:
The static strings for HOSTNAMES and BASEDN had typos. I fixed this and provided a change to upgrade() method to rectify any previous configurations using the incorrect spellings.
The method constructLDAPDN() appends the BaseDN after working out the LDAP DN. This is normally OK, but not if the BaseDN is 'blank'. Some situations may require a BaseDN set to 'blank', so have included a fix for this. Note that you could set a 'blank' BaseDN in the GUI by entering a space character.
The method getObjectClassAttribute() was limitted to creation of O and OU object classes. Generally, other object classes should be supported, such as DC, L, ST, and C. This method has been updated for this.
The method createIntermediateNodes() changed to use a recursive process of checking the existence of parent nodes, and then creating the missing nodes as the recursion unwinds. I suggest this code variation is a bit simpler and better handles a BaseDN that is blank..
The method revokeCertificate() has the option to delete the revoked certificate from LDAP. However, if the user has multiple certificates in the LDAP, then all certificates will get deleted. This is mentioned in the documentation, but a better approach would be to keep the remaining certificates in the LDAP. A fix for this has been included.
The method revokeCertificate() can also delete the whole user entry in the LDAP when their certificate is revoked. A better approach would be to only delete an entry if there are no other certificates remaining once the revoked certificate is removed. A fix for this has been included.
Some changes to debug log messages included.
Some methods changed from 'private' to 'protected' to help further subclassing.
How has this been tested?
Added extra testcase into LdapPublisherUnitTest.
Other testing performed manually using an LDAP service (OpenLdap and ViewDS). Tested publishing with CA and EE certs, included the creation of parent nodes. Tested revocation of EE cert using LDAP entries having one certificate, and others having two certificates.
Checklist before requesting a review
See also Contributing Guidelines.