Improvements to LdapPublisher #940
Conversation
# Conflicts: # .classpath
verify command was missing the dependency.
Remove dom4j library. Closes EJBCAINTER-816 See merge request ejbca/ejbca!1548
ECA-12639: Fix environment variable expansion for ConfigDump import Closes ECA-12639 See merge request ejbca/ejbca!1744
The flags for DNSSEC validation and DNAME lookups in the CAA CLI are inverted Closes ECA-12675 See merge request ejbca/ejbca!1765
Ref: ECA-12676
…gdump-import-ca-with-initialize' into 'main' ECA-12487: fix KeyPairInfo NPE when initializing imported CA Closes ECA-12487 See merge request ejbca/ejbca!1762
Resolve ECA-12524 "Msae ldap connection through ra" Closes ECA-12524 See merge request ejbca/ejbca!1674
Fix: adapt drop index query for postgresql Closes ECA-12323 See merge request ejbca/ejbca!1507
EJBCAINTER-1089 fix swagger-ui Closes EJBCAINTER-1089 See merge request ejbca/ejbca!1751
ECA-12674: Fix verify command Closes ECA-12674 See merge request ejbca/ejbca!1764
…main' Resolve EJBCAINTER-1004 "Cannot use plugins with java 17" Closes EJBCAINTER-1004 See merge request ejbca/ejbca!1711
ECA-12510: helm chart https only and added notes.txt Closes ECA-12510 See merge request ejbca/ejbca!1755
ECA-12675: Add tests for --nodnssec and -nodname for caacli See merge request ejbca/ejbca!1767
fix: deprecate debianfingerprint format Closes ECA-12676 See merge request ejbca/ejbca!1768
EJBCAINTER-1103: fix caa cli unit tests Closes EJBCAINTER-1103 See merge request ejbca/ejbca!1769
…11947-Jakarta10' Resolve EJBCAINTER-1000 "Fix/fb configdump import" See merge request ejbca/ejbca!1682
…oudshm-v5-hsm' into 'main' ECA-12626 Upgrade p11ng to prevent CKO_DATA deletion Closes ECA-12626 See merge request ejbca/ejbca!1772
Resolve ECA-11947 "Eb jakarta10" Closes ECA-11947 and EJBCAINTER-1000 See merge request ejbca/ejbca!1773
…-fixes/9.1.1 L10n: Admin GUI English message fixes (OCSP Responders)
…full-update/9.1.1 L10n: Admin GUI French update (based on 9.1.1) Fully translated
…l-update/9.1.1 L10n: RA GUI French update (based on 9.1.1) Fully translated
…lsender Log recipients for MailSender
…-algorithm/9.0.0' onto 'main' Noticket: Improve GUI messages for signing algorithm Noticket: Improve GUI messages for signing algorithm (red color) Noticket: Improve GUI messages for signing algorithm (the best)
…-algorithm/9.0.0' into improve/gui-messages-for-signing-algorithm/9.0.0
…or-signing-algorithm/9.0.0 Improve: GUI messages for signing algorithm in CA creation form
…is not used to "uniquely identify a key" as stated in RFC 4210 5.1.1. Contributed under SPDX "LGPL-2.1-or-later".
…eq-for-pbmac1 Make PKIMessage.PKIHeader.senderKID optional for PBE/PBMAC1
…is not used to "uniquely identify a key" as stated in RFC 4210 5.1.1. Contributed under SPDX "LGPL-2.1-or-later".
…eq-for-pbmac1-part2 Make PKIMessage.PKIHeader.senderKID optional for PBE/PBMAC1 since it …
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.
15 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.