Add support for enabling and disabling CRLs. #6329

tmael · 2023-04-21T17:47:04Z

Description

This PR adds the ability to turn CRL checks on or off from X509_STORE during runtime. This functionality would be consistent with OpenSSL. By default, the CRL check is enabled when wolfSSL is configured with HAVE_CRL. wolfSSL_DisableCRL() can be used to disable CRL check on SSL object but we are missing an ability to turn it off for a WOLFSSL_X509_STORE object.

Fixes zd#15424

Testing

How did you test?

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

tmael · 2023-05-25T19:21:19Z

retest this please!

src/internal.c

src/ssl.c

tmael · 2023-06-02T14:48:32Z

retest this please!

kareem-wolfssl

Looks good Tesfa, just a few questions and potential changes.

src/ssl.c

src/x509_str.c

kareem-wolfssl · 2023-06-07T19:48:08Z

src/x509_str.c

-    if ((flag & WOLFSSL_CRL_CHECKALL) || (flag & WOLFSSL_CRL_CHECK)) {
-        ret = wolfSSL_CertManagerEnableCRL(store->cm, (int)flag);
-    }
+    ret = wolfSSL_CertManagerEnableCRL(store->cm, (int)flag);


Is it safe to make this change, considering the new behavior only applies when OPENSSL_COMPATIBLE_DEFAULTS is defined? Shouldn't this also be gated behind OPENSSL_COMPATIBLE_DEFAULTS?

The OPENSSL_COMPATIBLE_DEFAULTS flag is unnecessary here because wolfSSL_CertManagerEnableCRL() does not utilize the provided flag when OPENSSL_COMPATIBLE_DEFAULTS is not defined.

Before this change, wolfSSL_CertManagerEnableCRL is only being called if either WOLFSSL_CRL_CHECKALL or WOLFSSL_CRL_CHECK is set. But after this change, wolfSSL_CertManagerEnableCRL is always being called. In the case where OPENSSL_COMPATIBLE_DEFAULTS is not defined, calling wolfSSL_CertManagerEnableCRL will enable the CRL, so this is leading to a behavior change for that case. The function X509_STORE_set_flags is used for other X509 flags unrelated to CRL, and it is not expected to enable the CRL in this case.

Is there a reason you need to remove the flag check? If I'm understanding right, this should work fine in both cases with this check kept.

tmael · 2023-06-08T18:51:28Z

retest this please!

kareem-wolfssl · 2023-06-08T22:40:53Z

src/ssl.c

+        /* Turn off all checks */
+        cm->crlCheckAll = 0;
+    }
+#endif
    #ifdef HAVE_CRL
        if (cm->crl == NULL) {
            cm->crl = (WOLFSSL_CRL*)XMALLOC(sizeof(WOLFSSL_CRL), cm->heap,


If OPENSSL_COMPATIBLE_DEFAULTS is defined, and we don't end up enabling CRL, is there any point in allocating and initializing the cert manager's CRL? Or should the entire function be skipped in this case? I'm not seeing anywhere else in the code where we set crlEnabled.

@kareem-wolfssl. Excellent observation! Your feedback has been implemented in the update.

kareem-wolfssl · 2023-06-08T22:44:03Z

src/x509_str.c

-    if ((flag & WOLFSSL_CRL_CHECKALL) || (flag & WOLFSSL_CRL_CHECK)) {
-        ret = wolfSSL_CertManagerEnableCRL(store->cm, (int)flag);
-    }
+    ret = wolfSSL_CertManagerEnableCRL(store->cm, (int)flag);


Before this change, wolfSSL_CertManagerEnableCRL is only being called if either WOLFSSL_CRL_CHECKALL or WOLFSSL_CRL_CHECK is set. But after this change, wolfSSL_CertManagerEnableCRL is always being called. In the case where OPENSSL_COMPATIBLE_DEFAULTS is not defined, calling wolfSSL_CertManagerEnableCRL will enable the CRL, so this is leading to a behavior change for that case. The function X509_STORE_set_flags is used for other X509 flags unrelated to CRL, and it is not expected to enable the CRL in this case.

Is there a reason you need to remove the flag check? If I'm understanding right, this should work fine in both cases with this check kept.

src/internal.c

src/ssl.c

src/x509.c

src/x509_str.c

julek-wolfssl · 2023-06-23T14:21:17Z

src/internal.c

+    else {
+    #ifdef WOLFSSL_REFCNT_ERROR_RETURN
+        return ret;
+    #else
+        (void)ret;
+    #endif
+    }


I'm not sure why the return value logic needs to be gated on WOLFSSL_REFCNT_ERROR_RETURN. I can see that this follows a similar pattern that happens in many other places. I'm not a fan of this but I'll allow it for now.

julek-wolfssl

Not all of my previous reviews have been addressed yet.

Do leaf CRL check by default Correct wolfSSL_sk_X509_NAME_push return check Update OpenSSL compatibility errors for HAProxy Change X509_V to literal constant values Fix the compat layer with TLS session ticket reuse Fix for tls1_2 session resume and cache miss Save intitial wolfSSL ctx Check for OpenSSL CRL error code 23

dgarske · 2023-06-28T16:35:04Z

src/internal.c

-#ifndef NO_WOLFSSL_SERVER
-    #if defined(HAVE_SESSION_TICKET) && !defined(WOLFSSL_NO_DEF_TICKET_ENC_CB)
+#if !defined(NO_WOLFSSL_SERVER) && defined(HAVE_SESSION_TICKET)
+    #if defined(WOLFSSL_HAPROXY)


Should we support a way to use initial_ctx with a macro besides WOLFSSL_HAPROXY?

@dgarske, not in this PR. More comfortable with the current changes since it has been tested against Haproxy.

Seems shortsighted to limit that area of code to HAPROXY only. Could you instead create a new macro like WOLFSSL_TICKET_USE_INITIAL_CTX and set this macro in settings.h for WOLFSSL_HAPROXY? At least giving other OSP projects the opportunity to use it?

I believe the use case for initial_ctx is quite rare, and I'm skeptical about its use in other OSP projects.
Currently, #define SSL_TICKET_CTX(ssl) ssl->initial_ctx->ticketEncCtx , initial_ctx, and ssl->initial_ctx are gated by WOLFSSL_HAPROXY. Are you suggesting alterations to the gating for all of these?
If that's the case, I can replace the WOLFSSL_HAPROXY gating with WOLFSSL_USE_INITIAL_CTX, which would provide a bit more flexibility.

Correct, looking for flexibility around the use of the initial_ctx feature. Its a fairly easy refactor. If you think HAPROXY is the only project that will ever use this feature then we can take the PR as-is.

Thanks for clarifying. Currently, I'm inclined to leave it as-is and consider the easy refactor only when necessary. None of our supported OSP projects require it presently, and if they had, we would have run into issues earlier. I can't predict the needs of future OSP projects. Although introducing a new setting is straightforward, it implies the added responsibility of maintenance and instructing non-configure autotool users to add one more setting to their user_settings.h file.

julek-wolfssl · 2023-06-28T17:05:57Z

@dgarske
Reassigning to @tmael as this is his PR.

tmael self-assigned this Apr 21, 2023

julek-wolfssl self-requested a review May 8, 2023 17:26

tmael assigned julek-wolfssl May 25, 2023

julek-wolfssl requested changes May 26, 2023

View reviewed changes

src/internal.c Outdated Show resolved Hide resolved

src/internal.c Outdated Show resolved Hide resolved

src/ssl.c Outdated Show resolved Hide resolved

src/ssl.c Outdated Show resolved Hide resolved

src/ssl.c Outdated Show resolved Hide resolved

tmael requested review from julek-wolfssl and kareem-wolfssl June 2, 2023 16:10

kareem-wolfssl requested changes Jun 7, 2023

View reviewed changes

tmael force-pushed the crl_off branch from df92cc5 to 0285eb5 Compare June 8, 2023 17:16

kareem-wolfssl requested changes Jun 8, 2023

View reviewed changes

tmael added the For This Release label Jun 8, 2023

tmael requested a review from kareem-wolfssl June 9, 2023 17:18

tmael removed the For This Release label Jun 9, 2023

tmael removed their assignment Jun 13, 2023

julek-wolfssl requested changes Jun 16, 2023

View reviewed changes

src/internal.c Show resolved Hide resolved

src/ssl.c Outdated Show resolved Hide resolved

src/ssl.c Show resolved Hide resolved

src/x509.c Show resolved Hide resolved

src/x509_str.c Outdated Show resolved Hide resolved

julek-wolfssl assigned tmael and unassigned julek-wolfssl Jun 16, 2023

tmael assigned julek-wolfssl and unassigned tmael Jun 16, 2023

tmael requested a review from julek-wolfssl June 16, 2023 23:44

kareem-wolfssl requested changes Jun 21, 2023

View reviewed changes

src/x509_str.c Show resolved Hide resolved

julek-wolfssl reviewed Jun 23, 2023

View reviewed changes

julek-wolfssl requested changes Jun 23, 2023

View reviewed changes

julek-wolfssl assigned tmael and unassigned julek-wolfssl Jun 23, 2023

tmael force-pushed the crl_off branch from 84f7062 to 44195d9 Compare June 23, 2023 15:37

tmael assigned julek-wolfssl and unassigned tmael Jun 23, 2023

tmael added 5 commits June 27, 2023 08:26

Maintain consistent behavior when activating CRL

481ae20

Add test_wolfSSL_dup_CA_list

03e5d10

Maintain existing behavor when not using OPENSSL_COMPATIBLE_DEFAULTS

cb0e2c8

update wolfSSL_X509_STORE_set_flags

26e6fd9

tmael force-pushed the crl_off branch from 44195d9 to 26e6fd9 Compare June 27, 2023 15:29

kareem-wolfssl approved these changes Jun 27, 2023

View reviewed changes

dgarske requested a review from julek-wolfssl June 27, 2023 20:31

julek-wolfssl approved these changes Jun 28, 2023

View reviewed changes

julek-wolfssl assigned dgarske and unassigned julek-wolfssl Jun 28, 2023

julek-wolfssl requested a review from dgarske June 28, 2023 14:20

dgarske requested changes Jun 28, 2023

View reviewed changes

dgarske assigned julek-wolfssl and unassigned dgarske Jun 28, 2023

julek-wolfssl assigned tmael and unassigned julek-wolfssl Jun 28, 2023

tmael assigned dgarske and unassigned tmael Jun 28, 2023

dgarske assigned tmael and unassigned dgarske Jun 28, 2023

dgarske approved these changes Jun 28, 2023

View reviewed changes

dgarske merged commit 048083c into wolfSSL:master Jun 28, 2023

wlallemand mentioned this pull request Aug 25, 2023

SSL session cache with wolfSSL haproxy/haproxy#2263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for enabling and disabling CRLs. #6329

Add support for enabling and disabling CRLs. #6329

tmael commented Apr 21, 2023 •

edited

Loading

tmael commented May 25, 2023 •

edited

Loading

tmael commented Jun 2, 2023

kareem-wolfssl left a comment

kareem-wolfssl Jun 7, 2023

tmael Jun 8, 2023 •

edited

Loading

kareem-wolfssl Jun 8, 2023

tmael commented Jun 8, 2023

kareem-wolfssl Jun 8, 2023

tmael Jun 9, 2023

kareem-wolfssl Jun 8, 2023

julek-wolfssl Jun 23, 2023

julek-wolfssl left a comment

dgarske Jun 28, 2023

tmael Jun 28, 2023

dgarske Jun 28, 2023 •

edited

Loading

tmael Jun 28, 2023 •

edited

Loading

dgarske Jun 28, 2023

tmael Jun 28, 2023

julek-wolfssl commented Jun 28, 2023

Add support for enabling and disabling CRLs. #6329

Add support for enabling and disabling CRLs. #6329

Conversation

tmael commented Apr 21, 2023 • edited Loading

Description

Testing

Checklist

tmael commented May 25, 2023 • edited Loading

tmael commented Jun 2, 2023

kareem-wolfssl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmael Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmael commented Jun 8, 2023

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

julek-wolfssl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarske Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

tmael Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julek-wolfssl commented Jun 28, 2023

tmael commented Apr 21, 2023 •

edited

Loading

tmael commented May 25, 2023 •

edited

Loading

tmael Jun 8, 2023 •

edited

Loading

dgarske Jun 28, 2023 •

edited

Loading

tmael Jun 28, 2023 •

edited

Loading