Enabling certificate verification using CRL-DP extension #1003

fmarco76 · 2024-04-10T09:58:33Z

Currently certificates can be validated only using OCSP with a configured responder or using the AIA certificate extension. If the responder cannot be used verification is not possible. This is the case for the startup certificates of the responder.

The new policy add a verification using the CRL-DP extension defined in the certificate. If this extension is defined it has precedence over the OCSP and if it pass no other check are performed. If CRL cannot be retrieved then the OCSP responder is used. This new method takes place when OCSP is configured without a default responder and the PKIX verification method is adopted (with the policy OCSP_LEAF_AND_CHAIN_POLICY).

At least a verification method has to return success to accept the certificate.

Additionally, the connector parameter enableOCSP is renamed to enableRevocationCheck since this is used to enable CRL-DP and AIA based validation

ladycfu

looks good. my only comment is,, as stated in slack, to keep enableOCSP working as before just for backward compatibility purpose. Thanks

edewata

I have some comments about backward compatibility, but everything else looks good. Feel free to update/merge.

As discussed in Slack, in the future we might want to add more params to configure the validation policy (e.g. OCSP first then CRL).

edewata · 2024-04-10T17:57:39Z

native/src/main/native/org/mozilla/jss/ssl/common.c

@@ -945,22 +945,22 @@ JSSL_verifyCertPKIXInternal(CERTCertificate *cert,
     * we construct the policy ourselves. */
    PRUint64 ocsp_Enabled_Hard_Policy_LeafFlags[2] = {
        /* crl */
-        CERT_REV_M_DO_NOT_TEST_USING_THIS_METHOD,
+        CERT_REV_M_TEST_USING_THIS_METHOD,


We probably should update the comment above this code since it's still saying Note that we disable CRL fetching as Dogtag doesn't support it.

edewata · 2024-04-10T17:58:09Z

native/src/main/native/org/mozilla/jss/ssl/common.c

        /* ocsp */
        CERT_REV_M_TEST_USING_THIS_METHOD |
            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
    };

    CERTRevocationMethodIndex ocsp_Enabled_Hard_Policy_Method_Preference[1] = {
-        cert_revocation_method_ocsp
+      cert_revocation_method_crl


This line is under indented.

edewata · 2024-04-10T18:00:44Z

tomcat/src/main/java/org/dogtagpki/jss/tomcat/Http11NioProtocol.java

+    public boolean getEnableRevocationCheck() {
+        return tomcatjss.getEnableRevocationCheck();
    }

-    public void setEnableOCSP(boolean enableOCSP) {
-        tomcatjss.setEnableOCSP(enableOCSP);
+    public void setEnableRevocationCheck(boolean enableRevocationCheck) {
+        tomcatjss.setEnableRevocationCheck(enableRevocationCheck);


As discussed on Slack, we should keep the enabledOCSP param for backward compatibility, but now the original getter/setter for that param should call TomcatJSS.enableRevocationCheck's getter/setter instead.

edewata · 2024-04-10T18:03:46Z

tomcat/src/main/java/org/dogtagpki/jss/tomcat/TomcatJSS.java

+        String enableRevocationCheckProp = config.getProperty("enableRevocationCheck");
+        if (enableRevocationCheckProp != null)
+            setEnableRevocationCheck(Boolean.parseBoolean(enableRevocationCheckProp));


Similarly, we should also check enableOCSP param for backward compatibility.

edewata · 2024-04-10T18:03:55Z

tomcat/src/main/java/org/dogtagpki/jss/tomcat/TomcatJSS.java

+        String enableRevocationCheckProp = connector.getAttribute("enableRevocationCheck");
+        if (enableRevocationCheckProp != null)
+            setEnableRevocationCheck(Boolean.parseBoolean(enableRevocationCheckProp));


Similarly, we should also check enableOCSP param for backward compatibility.

edewata · 2024-04-10T18:16:15Z

docs/changes/v5.6.0/Parameter-Changes.adoc

+== enableOCSP become enableRevocationCheck ==
+
+Since verification with CRL-DP is introduced and it is enabled using
+this parameter in `server.xml` connector, the name is modified to be not related only to OCSP.


Since we're going to keep enableOCSP, I'd suggest to modify this to be something like:

Rename enableOCSP to enableRevocationCheck

The enableOCSP param has been deprecated. Use enableRevocationCheck instead.

Currently certificates can be validated only using OCSP with a configured responder or using the AIA certificate extension. If the responder cannot be used verification is not possible. This is the case for the startup certificates of the responder. The new policy add a verification using the CRL-DP extension defined in the certificate. If AIA extension is defined it has precedence over the CRLs and if no other check are performed. This new method takes place when OCSP is configured without a default responder and the PKIX verification method is adopted (with the policy OCSP_LEAF_AND_CHAIN_POLICY). At least a verification method, defined in the certificate, has to return success to accept the certificate.

Since verification with CRL-DP is introduced and it is enabled using this parameter the name is modified to be not related only to OCSP.

edewata · 2024-04-11T13:43:24Z

native/src/main/native/org/mozilla/jss/ssl/common.c

+     * - if AIA and CRL-DP are both presents only AIA is used and in case
+     *   freshin formation cannot be retrieved it fails the validation;


I think there's a typo: fresh information.

With this PR what would be the verification order, CRL-DP first or AIA first?

If the AIA check is done first but the OCSP responder is not available, can we fallback to CRL-DP?

JFTR, @fmarco76 explained that the order is defined in ocsp_Enabled_Hard_Policy_Method_Preference, and currently it's configured to do OCSP first (so CRL is implicitly the second).

Also, it seems like it's not possible to define a single validation policy that can fallback from OCSP to CRL without affecting individual OCSP or CRL validations. A possible workaround is to define separate policies based on which extensions are present in the cert itself, but this likely requires more significant changes.

Regardless, the description above correctly describes the policy based on the changes in this PR, so feel free to update/merge. Thanks!

ladycfu · 2024-04-11T16:09:49Z

native/src/main/native/org/mozilla/jss/ssl/common.c

+     *
+     * When enabled the checking on the chained CA certificates.
+     * With this policy the verification process does:
+     * - if one between AIA and CRL-DP is present then it will be used;


suggestion to rephrase : "- if only an AIA or an CRL-DP is present, the respective validation method is used;"

ladycfu · 2024-04-12T16:39:41Z

native/src/main/native/org/mozilla/jss/ssl/common.c

        /* ocsp */
        CERT_REV_M_TEST_USING_THIS_METHOD |
            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
    };

    PRUint64 ocsp_Enabled_Hard_Policy_ChainFlags[2] = {
        /* crl */
-        CERT_REV_M_DO_NOT_TEST_USING_THIS_METHOD,
+        CERT_REV_M_TEST_USING_THIS_METHOD |
+            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO,
        /* ocsp */
        CERT_REV_M_TEST_USING_THIS_METHOD |
            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
    };



for the following setting, it might help the reader to add a comment that says something like "/* if CRL-dp is present in the cert, disable CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO for ocsp */

Comment added to the if where the flag is modified

ladycfu · 2024-04-12T16:48:22Z

native/src/main/native/org/mozilla/jss/ssl/common.c

+     * - if only an AIA or an CRL-DP is present, the respective validation method is used;
+     * - if AIA and CRL-DP are both presents the execution order is: AIA first followed by
+     *   CRL only in case OCSP endpoint does not provide fresh information;
+     * - it no AIA and CRL-DP are present no revocation check is performed.*/


typo. "if" instead of "it"

ladycfu

looks good. Just a couple comments-related suggestions.

edewata · 2024-04-12T18:28:28Z

I don't have any additional comments. Thanks for the updates!

In case of CRLDP and AIA extensions available in the certificate the PKIX verification flags do not allow to implement a fallback mechanism because the fallback remain active also for certificate with only one extension with unexpected behaviour. A more dynamic approach is introduced verifying the presence of the CRLDP extension and setting the flags accordingly.

sonarqubecloud · 2024-04-15T08:09:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fmarco76 requested review from edewata and ladycfu April 10, 2024 09:58

fmarco76 force-pushed the CRL-DP_Enabled branch from 573ba47 to ea808b8 Compare April 10, 2024 10:09

ladycfu reviewed Apr 10, 2024

View reviewed changes

edewata approved these changes Apr 10, 2024

View reviewed changes

fmarco76 added 2 commits April 11, 2024 14:56

Rename enableOCSP to enableRevocationCheck

332041b

Since verification with CRL-DP is introduced and it is enabled using this parameter the name is modified to be not related only to OCSP.

fmarco76 force-pushed the CRL-DP_Enabled branch from ea808b8 to 332041b Compare April 11, 2024 13:01

edewata reviewed Apr 11, 2024

View reviewed changes

ladycfu reviewed Apr 11, 2024

View reviewed changes

fmarco76 force-pushed the CRL-DP_Enabled branch 2 times, most recently from 7d90b9f to bf32a6f Compare April 12, 2024 14:42

ladycfu reviewed Apr 12, 2024

View reviewed changes

fmarco76 force-pushed the CRL-DP_Enabled branch from bf32a6f to ff5589a Compare April 15, 2024 08:07

fmarco76 merged commit 242024a into dogtagpki:master Apr 15, 2024
34 of 35 checks passed

fmarco76 deleted the CRL-DP_Enabled branch May 17, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling certificate verification using CRL-DP extension #1003

Enabling certificate verification using CRL-DP extension #1003

fmarco76 commented Apr 10, 2024

ladycfu left a comment

edewata left a comment

edewata Apr 10, 2024

edewata Apr 10, 2024

edewata Apr 10, 2024 •

edited

Loading

edewata Apr 10, 2024

edewata Apr 10, 2024

edewata Apr 10, 2024

edewata Apr 11, 2024 •

edited

Loading

edewata Apr 11, 2024

ladycfu Apr 11, 2024

ladycfu Apr 12, 2024

fmarco76 Apr 15, 2024

ladycfu Apr 12, 2024

ladycfu left a comment

edewata commented Apr 12, 2024

sonarqubecloud bot commented Apr 15, 2024

		* - if AIA and CRL-DP are both presents only AIA is used and in case
		* freshin formation cannot be retrieved it fails the validation;

Enabling certificate verification using CRL-DP extension #1003

Enabling certificate verification using CRL-DP extension #1003

Conversation

fmarco76 commented Apr 10, 2024

ladycfu left a comment

Choose a reason for hiding this comment

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

edewata Apr 10, 2024 • 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

edewata Apr 11, 2024 • 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

ladycfu left a comment

Choose a reason for hiding this comment

edewata commented Apr 12, 2024

sonarqubecloud bot commented Apr 15, 2024

Quality Gate passed

edewata Apr 10, 2024 •

edited

Loading

edewata Apr 11, 2024 •

edited

Loading