OCSP responder to serve status check for itself using latest CRL #4504

fmarco76 · 2023-07-19T13:20:13Z

Modify OCSP responder to perform peer certificate checks with the CRL instead of using OCSP, avoiding the chicken and egg issue.

Solve RHCS-4262

fmarco76 · 2023-07-19T13:24:32Z

The first step of inverting the init order to load the store first is done.

The init order for OCSP is modified to allow CRL retrieval before creating connection with DS or other services. Secure`connections will be verified against the CRL. Solve RHCS-4262

Add new field in CMS for a callback validation of certificate instantiated by PKISocketFactory. This is useful for OCSP where the OCSP protocol cannot be enabled and the verification is done on CRLs. Solve RHCS-4262

Add a new parameter to enable the crl check for OCSP connection when acting as client. The new parameter is `ocsp.store.ldapStore.checkSubsystemConnection` and its default value is `false`. When set to `true` connection certificate are verified using the crl stored in the LDAP.

When OCSP is acting as server certificate can be verified using CRL internally stored. To verify the certificates the `LDAPStore` has to be enabled with the variable `ocsp.store.ldapStore.checkSubsystemConnection` and the variable `auths.revocationChecking.enabled` both set to true. Solve RHCS-4262

edewata

Please see my comments below. I have one question about the loop, and the rest are just minor suggestions.

edewata · 2023-07-31T15:16:53Z

base/server/src/main/java/com/netscape/cmscore/apps/CMS.java

@@ -53,6 +54,8 @@ public final class CMS {

    private static CMSEngine engine;

+    private static SSLCertificateApprovalCallback approvalCallback;


In the newer branches we've removed the dependency on global variables in CMS class. I'd suggest moving this variable into CMSEngine class.

edewata · 2023-07-31T15:20:21Z

base/ocsp/src/main/java/com/netscape/cms/ocsp/CRLLdapValidator.java

+
+    @Override
+    public boolean approve(X509Certificate certificate, ValidityStatus currentStatus) {
+        logger.info("CRLLdapValidator: validate of peer's certificate for the connection " + certificate.getSubjectDN().toString());


The .toString() can be removed since it's implicitly called by string concatenation.

edewata · 2023-07-31T15:23:56Z

base/ocsp/src/main/java/com/netscape/cms/ocsp/CRLLdapValidator.java

+                }
+            }
+        } catch (EBaseException e) {
+            logger.error("CRLLdapValidator: problem find CRL issuing point for " + certificate.getIssuerDN().toString());


To simplify troubleshooting I'd suggest displaying the exception message and optionally attach the exception (to show the stack trace):

logger.error("...<message>...: " + e.getMessage(), e);

edewata · 2023-07-31T15:34:46Z

base/ocsp/src/main/java/org/dogtagpki/server/ocsp/OCSPEngine.java

+        for (X509Certificate cert: certificates) {
+            if(crlCertValid(crlStore, cert, null)) {
+                return false;
+            }
+        }


IIUC this loop will return false as soon as it finds the first valid cert. Should we check all certs in the chain instead?

Socket callback moved to CMSEngine to avoid dependencies on global variables.

edewata

Thanks for the update! Looks good to me, but I'll defer to others for the final approval.

ladycfu

I have conducted a few tests with the PR on revoked DS server-cert, agent cert at webUI, and admin cert with pkiconsole and I think they work as expected. Of course more testing could be used for other scenarios but I'll leave that to QE. With regard to this PR, please see my comments. Thanks.

ladycfu · 2023-08-02T17:47:43Z

base/ocsp/src/main/java/com/netscape/cms/ocsp/LDAPStore.java

@@ -137,6 +139,7 @@ public void init(IConfigStore config, DBSubsystem dbSubsystem) throws EBaseExcep
                    DEF_CA_CERT_ATTR);
        mByName = mConfig.getBoolean(PROP_BY_NAME, true);

+        mCheckConnection = mConfig.getBoolean(PROP_CHECK_SUBSYSTEM_CONNECTION, false);


I'm debating whether this should be true for default. What's your thoughts on this @fmarco76 @edewata ?

Or, simply put ocsp.store.ldapStore.checkSubsystemConnection=true as default in OCSP's CS.cfg. Also, based on my latest experiment, how about adding the following to the OCSP's CS.cfg: auths.revocationChecking.enabled=true . This is so that role access at the WebUI and pkiconsole can do that by default. CA already has that by default.

This code is only used by the OCSP subsystem. Is there any case where we want this functionality disabled? If it's always going to be enabled maybe we don't need this param.

I agree with @edewata, I do not see a reason why this should be disabled so I think we could make permanent this change and not triggered by an option. However, we could just makes it the default behaviour just in case the scenario where disabling the check arise. @ladycfu I am converting to true.

I have some concerns with auths.revocationChecking.enabled=true. For the OCSP subsystem this work only if the CA sends CRLs through an LDAP and not with direct publishing. Should we stopped, or at least log an error, if it is true but LDAP publishing is not configured?

The reason why I propose having a "switch" to go back to the previous behavior was because we want to be sure that there is no gotcha's in the new mechanism. For now, I think this switch serves its purpose, as a back up . I think it's fine to have it enabled by default. Now, wrt auths.revocationChecking.enabled=true, this "Method C" precludes the direct CA->OCSP CRL publishing because the ocsp's ssl port is not able to receive CRL from the CA due to chick-n-egg issue. The CA depends on the ocsp to start up, and the ocsp depends on the CA to start up, if replying on ca->ocsp direct publishing. The CA is to have enableOCSP=true and its internaldb's server-cert is to bear an AIA pointing to the ocsp. @fmarco76 I think you have a good suggestion there on checking whether it's ldapStore that's being set for crl update if ocsp.store.ldapStore.checkSubsystemConnection=true. btw, what is the concern you wrt auths.revocationChecking.enabled=true ?

@fmarco76 btw, why do you call this flag "checkSubsystemConnection"? To me it seems something like "ocsp.store.ldapStore.validateConnCertWithCRL" would be more telling. Actually, could you add an explanation in comment where this flag is defined what exactly it does? We need to add something in the doc. Thanks.

I think you have a good suggestion there on checking whether it's ldapStore that's being set for crl update if ocsp.store.ldapStore.checkSubsystemConnection=true.

This setting is done on ldapStore of the OCSP so we do not need to verify if it is used because if it is not then this setting does not apply.

what is the concern you wrt auths.revocationChecking.enabled=true ?

In the OCSP, if this is true but the ldapStore is not used the revocation cannot be verified. I have not tested if this generate errors. Therefore I was wondering if the user should be notified that revocation will not work even it is enabled.

why do you call this flag "checkSubsystemConnection"? To me it seems something like "ocsp.store.ldapStore.validateConnCertWithCRL" would be more telling. Actually, could you add an explanation in comment where this flag is defined what exactly it does? We need to add something in the doc.

Yes, sound better, I will change the name. Where exactly should I write an explanation? A comment in github where the code is provided or a comment inside the code?

wrt auths.revocationChecking.enabled=true, I'm still not sure why it would require ldapStore. I will take a closer look later.

Because the method OCSPEngine.isRevoked() is invoked when a new connection to OCSP is created. This method uses the LDAPStore to verify the certificate. If LDAPStore is not used, or the CRL verification is disabled I have not verified what happen.

ok I just dug slightly deeper and saw that you had overwritten the isRevoked in the OCSPEngine.

ladycfu · 2023-08-02T18:48:06Z

base/ocsp/src/main/java/com/netscape/cms/ocsp/CRLLdapValidator.java

+            while (eCRL.hasMoreElements() && pt == null) {
+                ICRLIssuingPointRecord tPt = eCRL.nextElement();
+                logger.debug("CRLLdapValidator: CRL check issuer  " + tPt.getId());
+                if(tPt.getId().equals(certificate.getIssuerDN().toString())) {


I'm not certain about issuerDN string comparison here. If you look at the processRequest(...) method in ocsp/src/main/java/com/netscape/cms/ocsp/LDAPStore.java, it uses the CA's public key hash as key to locate the right CRL, which is more assuring, isn't it?

I have implemented this using the CheckCertServlet as reference where the ICRLIssuingPointRecord is identified using the common name. Nevertheless, using the key hash is definitely a stronger mechanism. I will try to use the processRequest method or extract some code in a separate method.

@ladycfu I did not find an easy way to get the issuer key hash from the certificate but to be more confident I have verified that the CA cert is the one signing the certificate. The test is done only if the DNs match to avoid the overhead of encoding/decoding certificates. I have performed the operation considering Mozilla-JSS provider is available. If this is not always the case some extra step are needed. Is this approach viable for the job?

Hi @fmarco76 , I think in general, it's a better practice to avoid string comparison when it comes to DN names, due to encoding differences. Looking closely, the processRequest was for processing an ocsp request which contains the CertID, so it's not exactly what we have here. We have the peer cert, X509Certificate certificate, and we have the ca cert, X509CertImpl ca_certImpl = new X509CertImpl(tPt.getCACert()); and we can get the ca's public key PublicKey ca_pubkey = ca_certImpl.getPublicKey(); where PublicKey is java.security.PublicKey. Meanwhile, the peer cert X509Certificate certificate can have it's public key gotten with PublicKey peerCert_pubkey = certificate.getPublicKey(); So now you can compare. I'm not sure where you said you did signature verification. I could be wrong, but that sounds more expensive than the PublicKey comparison. Anyway, I think this patch is as good as it gets for the time we have, so you could leave this as an improvement for later if you want if you donn't have time for it now. Thanks.

Hi @fmarco76 , I think in general, it's a better practice to avoid string comparison when it comes to DN names, due to encoding differences.

IIRC String in java have the same encoding independently of how they are stored in the certificate. The encoding is converted when they are read/write. The format could be different but this is related to the used classes so it is known (although it could change moving to different versions of Java).

Looking closely, the processRequest was for processing an ocsp request which contains the CertID, so it's not exactly what we have here. We have the peer cert, X509Certificate certificate, and we have the ca cert, X509CertImpl ca_certImpl = new X509CertImpl(tPt.getCACert()); and we can get the ca's public key PublicKey ca_pubkey = ca_certImpl.getPublicKey(); where PublicKey is java.security.PublicKey. Meanwhile, the peer cert X509Certificate certificate can have it's public key gotten with PublicKey peerCert_pubkey = certificate.getPublicKey(); So now you can compare. I'm not sure where you said you did signature verification. I could be wrong, but that sounds more expensive than the PublicKey comparison.

The problem here is that you get the PublicKey of the CA and the PublicKey of the peer certificate. They are different so there is no way to compare. The OCSP request contain the hash of the issuer PublicKey so it is possible to get the hash of the CA PublicKey and compare.
From the peer certificate I found no way to get the issuer PublicKey to compare with the CA.
The approach I implemented is in line 61-63 of CRLLdapValidator.

It is definitely more expensive of the PublicKey comparison and this is the reason I do only if the DN match. However, considering a low number of CA for an OCSP we could skip the DN match and do always the signature check. Is this assumption correct?

Anyway, I think this patch is as good as it gets for the time we have, so you could leave this as an improvement for later if you want if you don't have time for it now. Thanks.

I can work on this until the end of the sprint so we still have several days before merge.

ladycfu · 2023-08-02T18:53:14Z

base/ocsp/src/main/java/org/dogtagpki/server/ocsp/OCSPEngine.java

+            while (eCRL.hasMoreElements() && pt == null) {
+                ICRLIssuingPointRecord tPt = eCRL.nextElement();
+                logger.debug("OCSPEngine: CRL check issuer  " + tPt.getId());
+                if(certificate.getIssuerX500Principal().equals(new X500Principal(tPt.getId()))) {


same comment as above regarding using CA's public key hash.

Same approach as above.

The parameter `ocsp.store.ldapStore.checkSubsystemConnection` default value has been modified to `true` so when LDAPStore is used certificates are verified against the CRL. Additionally, during the certificate verification the certificate signer is verified with the CA certificate providing the CRL to be sure it is the real issuer.

fmarco76 · 2023-08-03T21:41:40Z

With last commit there is an error on TPS tests but I do not see changes in this PR that could generate the error

The option `ocsp.store.ldapStore.validateConnCertWithCRL` enables the revocation verification of peer certificates using the CRL stored in the LDAP shared with the CA. When it is set to `true` (default value), the peer certificate of all the outcome connections from the OCSP subsystem are verified with the CRL. If the option `auths.revocationChecking.enabled` is also set to `true` the peer certificate ot all the income connections to the OCSP subsystem are verified with the CRL.

fmarco76 · 2023-08-04T11:01:24Z

base/ocsp/src/main/java/com/netscape/cms/ocsp/LDAPStore.java

@@ -82,6 +81,7 @@ public class LDAPStore implements IDefStore, IExtendedPluginInfo {
    private static final String DEF_CA_CERT_ATTR = "cACertificate;binary";
    private static final String PROP_HOST = "host";
    private static final String PROP_PORT = "port";
+    private static final String PROP_VALIDATE_CONNECTION_WITH_CRL = "validateConnCertWithCRL";


The option ocsp.store.ldapStore.validateConnCertWithCRL enables the revocation verification of peer certificates using the CRL stored in the LDAP shared with the CA.

When it is set to true (default value), the peer certificate of all the outcome connections from the OCSP subsystem are verified with the CRL.

If the option auths.revocationChecking.enabled is also set to true the peer certificate ot all the income connections to the OCSP subsystem are verified with the CRL.

How about put the info right here in the code where PROP_VALIDATE_CONNECTION_WITH_CRL = "validateConnCertWithCRL" is assigned? Comments in code are longer lasting and provide info others later. I will also be using it for the doc changes.

Identification of CRL issuing point done by matching Authority Key Identifier with Subject Key Identifier instead of DN matching. This should make more reliable the check because not affected of encoding or format changes in the DN.

ladycfu · 2023-08-04T17:56:55Z

base/ocsp/src/main/java/org/dogtagpki/server/ocsp/OCSPEngine.java

+        try {
+            X509CertImpl peerCert = new X509CertImpl(certificate.getEncoded());
+            Enumeration<ICRLIssuingPointRecord> eCRL = crlStore.searchAllCRLIssuingPointRecord(-1);
+            AuthorityKeyIdentifierExtension aPeeExt = (AuthorityKeyIdentifierExtension) peerCert.getExtension(PKIXExtensions.AuthorityKey_Id.toString());


May I suggest that you rename the variable "ePeeExt" to something else? lol. How about "peerAKIExt"?

ladycfu

Hi Marco, thanks for the update. I think the only things I have for you now is just to address the couple minor requests I have regarding adding a comment explaining the new param and changing one variable name.

ladycfu

Thanks for the update. LGTM

fmarco76 · 2023-08-08T10:37:24Z

@edewata @ladycfu Thanks!

fmarco76 added the WIP Work In Progress label Jul 19, 2023

fmarco76 marked this pull request as draft July 19, 2023 13:20

fmarco76 changed the title ~~Modify init order for OCSP subsystem~~ OCSP responder to serve status check for itself using latest CRL Jul 19, 2023

fmarco76 requested a review from ladycfu July 19, 2023 13:25

Modify init order for OCSP subsystem

b9a9eb0

The init order for OCSP is modified to allow CRL retrieval before creating connection with DS or other services. Secure`connections will be verified against the CRL. Solve RHCS-4262

fmarco76 force-pushed the OCSP_CRL branch 2 times, most recently from 4db78c4 to ecc6b01 Compare July 21, 2023 16:20

fmarco76 added 2 commits July 27, 2023 20:38

Add callback for CRL validation at application level

57df77e

Add new field in CMS for a callback validation of certificate instantiated by PKISocketFactory. This is useful for OCSP where the OCSP protocol cannot be enabled and the verification is done on CRLs. Solve RHCS-4262

fmarco76 force-pushed the OCSP_CRL branch 2 times, most recently from e8ed05b to e1e85e5 Compare July 28, 2023 14:43

fmarco76 force-pushed the OCSP_CRL branch from e1e85e5 to 7ad255e Compare July 31, 2023 10:00

fmarco76 removed the WIP Work In Progress label Jul 31, 2023

fmarco76 requested review from edewata, jmagne and ckelleyRH July 31, 2023 10:06

fmarco76 marked this pull request as ready for review July 31, 2023 10:07

edewata reviewed Jul 31, 2023

View reviewed changes

fmarco76 force-pushed the OCSP_CRL branch from b90b9b7 to 59b9851 Compare July 31, 2023 16:38

Move callback reference from CMS to CMSEngine

d54adc9

Socket callback moved to CMSEngine to avoid dependencies on global variables.

fmarco76 force-pushed the OCSP_CRL branch from 59b9851 to d54adc9 Compare August 1, 2023 10:02

edewata approved these changes Aug 1, 2023

View reviewed changes

ladycfu reviewed Aug 2, 2023

View reviewed changes

fmarco76 commented Aug 4, 2023

View reviewed changes

Use AKI/SKI to match peer certificate with CA CRL

4366f72

Identification of CRL issuing point done by matching Authority Key Identifier with Subject Key Identifier instead of DN matching. This should make more reliable the check because not affected of encoding or format changes in the DN.

ladycfu reviewed Aug 4, 2023

View reviewed changes

fmarco76 added 2 commits August 4, 2023 19:57

Add comment for the option ocsp.store.ldapStore.validateConnCertWithCRL

584cc11

Modify local variable names

3513512

ladycfu reviewed Aug 5, 2023

View reviewed changes

ladycfu approved these changes Aug 7, 2023

View reviewed changes

Update log message for revoked certificate

47f268c

fmarco76 merged commit cb2ea11 into dogtagpki:v10.13 Aug 8, 2023
56 of 57 checks passed

fmarco76 deleted the OCSP_CRL branch August 8, 2023 10:46

fmarco76 mentioned this pull request Aug 16, 2023

Implement ocsp crl check method c #4545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCSP responder to serve status check for itself using latest CRL #4504

OCSP responder to serve status check for itself using latest CRL #4504

fmarco76 commented Jul 19, 2023 •

edited

Loading

fmarco76 commented Jul 19, 2023

edewata left a comment

edewata Jul 31, 2023

edewata Jul 31, 2023

edewata Jul 31, 2023

edewata Jul 31, 2023

edewata left a comment

ladycfu left a comment •

edited

Loading

ladycfu Aug 2, 2023

ladycfu Aug 2, 2023

edewata Aug 2, 2023

fmarco76 Aug 3, 2023

ladycfu Aug 3, 2023

ladycfu Aug 3, 2023

fmarco76 Aug 4, 2023

ladycfu Aug 4, 2023

fmarco76 Aug 4, 2023

ladycfu Aug 5, 2023

ladycfu Aug 2, 2023

fmarco76 Aug 3, 2023

fmarco76 Aug 3, 2023

ladycfu Aug 3, 2023

fmarco76 Aug 4, 2023 •

edited

Loading

ladycfu Aug 2, 2023

fmarco76 Aug 3, 2023

fmarco76 commented Aug 3, 2023

fmarco76 Aug 4, 2023

ladycfu Aug 4, 2023

ladycfu Aug 4, 2023

ladycfu left a comment

ladycfu left a comment

fmarco76 commented Aug 8, 2023

		@@ -53,6 +54,8 @@ public final class CMS {

		private static CMSEngine engine;

		private static SSLCertificateApprovalCallback approvalCallback;

OCSP responder to serve status check for itself using latest CRL #4504

OCSP responder to serve status check for itself using latest CRL #4504

Conversation

fmarco76 commented Jul 19, 2023 • edited Loading

fmarco76 commented Jul 19, 2023

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edewata left a comment

Choose a reason for hiding this comment

ladycfu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco76 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco76 commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu left a comment

Choose a reason for hiding this comment

ladycfu left a comment

Choose a reason for hiding this comment

fmarco76 commented Aug 8, 2023

fmarco76 commented Jul 19, 2023 •

edited

Loading

ladycfu left a comment •

edited

Loading

fmarco76 Aug 4, 2023 •

edited

Loading