added support for OCSP multistapling by mikrue · Pull Request #19183 · openssl/openssl

mikrue · 2022-09-09T13:50:19Z

OCSP multistapling for TLSv1.3:

changed resp variable in the the ssl_struct from one DER encoded OCSP response to a stack of responses
server side: added function in s_server code for retrieving the OCSP responses from all certificates in the server cert chain
server side: added function in statem_srvr code to retrieve and return the response for the requested certificate
client side: added verify function for multiple OCSP responses

Checklist

documentation is added or updated
tests are added or updated

FdaSilvaYY

nice feature !

FdaSilvaYY · 2022-09-09T15:17:13Z

crypto/x509/x509_vfy.c

    int i = 0, last = 0, ok = 0;

-    if ((ctx->param->flags & X509_V_FLAG_CRL_CHECK) == 0)
+    if (!(ctx->param->flags & X509_V_FLAG_CRL_CHECK)


nit: explicit test against 0 : if ((ctx->param->flags & X509_V_FLAG_CRL_CHECK) == 0)

FdaSilvaYY · 2022-09-09T15:18:39Z

crypto/x509/x509_vfy.c

+            && !(ctx->param->flags & X509_V_FLAG_OCSP_CHECK))
        return 1;
-    if ((ctx->param->flags & X509_V_FLAG_CRL_CHECK_ALL) != 0) {
+    if (ctx->param->flags & X509_V_FLAG_CRL_CHECK_ALL


You may combine the flags test : if (ctx->param->flags & (X509_V_FLAG_CRL_CHECK_ALL|X509_V_FLAG_OCSP_CHECK_ALL)) != 0)

FdaSilvaYY · 2022-09-09T15:20:19Z

crypto/x509/x509_vfy.c

+            if (ok == V_OCSP_CERTSTATUS_GOOD)
+                continue;
+
+            if (!(ctx->param->flags & X509_V_FLAG_CRL_CHECK)) {


nit: explicit flag test against 0

FdaSilvaYY · 2022-09-09T15:20:44Z

crypto/x509/x509_vfy.c

+                                                X509_get_issuer_name(x));
+
+
+    if(ctx->ocsp_resp == NULL || sk_OCSP_RESPONSE_num(ctx->ocsp_resp) == 0) {


style nit: if (...

FdaSilvaYY · 2022-09-09T15:21:37Z

crypto/x509/x509_vfy.c

+
+    cert_id = OCSP_cert_to_id(NULL, ctx->current_cert, ctx->current_issuer);
+
+    for (i=0; i<sk_OCSP_RESPONSE_num(ctx->ocsp_resp); i++) {


style nit : spaces around operators : for ( i = 0; i < sk_...

FdaSilvaYY · 2022-09-09T15:40:23Z

ssl/statem/statem_clnt.c

-        SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_R_LENGTH_MISMATCH);
-        return 0;
+    if (SSL_CONNECTION_IS_TLS13(s) || type == TLSEXT_STATUSTYPE_ocsp) {
+        if(PACKET_remaining(pkt) > 0) {


nit : if ( ...

FdaSilvaYY · 2022-09-09T15:41:52Z

ssl/statem/statem_clnt.c

+            if (resplen > 0) {
+                respder = OPENSSL_malloc(resplen);
+                if (!PACKET_copy_bytes(pkt, respder, resplen)) {
+                SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_R_LENGTH_MISMATCH);


indentation is off by 4 space here

you are leaking respder on both return 0

FdaSilvaYY · 2022-09-09T15:42:08Z

ssl/statem/statem_clnt.c

+                p = respder;
+                resp = d2i_OCSP_RESPONSE(NULL, &p, resplen);
+                if (resp == NULL) {
+


useless blank line

FdaSilvaYY · 2022-09-09T15:44:45Z

ssl/statem/statem_srvr.c

+
+    /* In TLSv1.3 the caller gives the index of the certificate for which the
+     * status message should be created.
+     * Prior to TLSv1.3 the chain index is 0 and the body should only the status


and the body should only the
Missing word ?

FdaSilvaYY · 2022-09-09T15:45:32Z

ssl/statem/statem_srvr.c

+         * if chainidx > 0 an intermediate certificate is request
+         */
+        if((int)chainidx < sk_X509_num(server_certs)+1 && chainidx > 0) {
+            x = sk_X509_value(server_certs, chainidx-1);


nit: space around operators : chainidx - 1

FdaSilvaYY · 2022-09-09T17:47:14Z

Need a bit of tidy up.
A few memleaks in current code changes.
Can you add some tests ?

FdaSilvaYY

Reading a bit more, I see that theses changes need more work to be finished.

FdaSilvaYY · 2022-09-09T17:53:37Z

apps/s_client.c

+    STACK_OF(OCSP_RESPONSE) *sk_resp = NULL;
    OCSP_RESPONSE *rsp;
-    len = SSL_get_tlsext_status_ocsp_resp(s, &p);
+    SSL_get_tlsext_status_ocsp_resp(s, &sk_resp);


Where have you change the SSL_get_tlsext_status_ocsp_resp method signature ?
SSL_get_tlsext_status_ocsp_resp is a macro around a SSL_CRTL command.

You can't change existing API .

You should keep it untouched, and add new methods for your purpose.

FdaSilvaYY · 2022-09-09T17:56:13Z

ssl/s3_lib.c

-        OPENSSL_free(sc->ext.ocsp.resp);
-        sc->ext.ocsp.resp = parg;
-        sc->ext.ocsp.resp_len = larg;
+        sc->ext.ocsp.resp = (STACK_OF(OCSP_RESPONSE) *)parg;


you should keep freeing the existing extensions sc->ext.ocsp.resp

FdaSilvaYY · 2022-09-09T18:01:34Z

ssl/s3_lib.c

-                || sc->ext.ocsp.resp_len > LONG_MAX)
-            return -1;
-        return (long)sc->ext.ocsp.resp_len;
+        *(STACK_OF(OCSP_RESPONSE) **)parg = sc->ext.ocsp.resp;


You can't change the code behind a SSL_CTRL like this.
It will break any existing code calling it.

This probably breaks some test code.

mikrue · 2022-09-10T16:39:38Z

Closed and re-open to remove the “CLA missing” tag

t8m · 2022-09-12T06:35:18Z

You've submitted the patch with a different e-mail address than that of your ICLA. Would you please amend the commit and force push?

openssl-machine · 2022-10-13T00:08:53Z

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

mikrue · 2022-10-13T15:46:53Z

Hello! I have already submitted the CLA a few weeks ago. How can I remove this label? Regards Michael Krüger

…

Am 13.10.2022 um 02:09 schrieb openssl-machine ***@***.***>: This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html <https://www.openssl.org/policies/cla.html> — Reply to this email directly, view it on GitHub <#19183 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL6KEQSOVIUZE7Q4YDGQKATWC5HKDANCNFSM6AAAAAAQIWK3HA>. You are receiving this because you modified the open/close state.

FdaSilvaYY · 2022-10-13T18:12:41Z

Hello!

I have already submitted the CLA a few weeks ago. How can I remove this label?

Regards
Michael Krüger

Am 13.10.2022 um 02:09 schrieb openssl-machine @.***>:

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html https://www.openssl.org/policies/cla.html
—

You should close and reopen your PR, to get the bot run again

t8m · 2022-10-13T18:23:37Z

I have already submitted the CLA a few weeks ago. How can I remove this label?

Please rebase the pull request against fresh master branch. Also amend the e-mail address so it is the same as on your CLA submission.

openssl-machine · 2022-11-13T00:08:59Z

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

openssl-machine · 2022-12-14T00:08:54Z

This PR has the label 'hold: cla required' and is stale: it has not been updated in 61 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

openssl-machine · 2023-01-12T00:09:07Z

This PR has been closed. It was waiting for a CLA for 90 days.

added support for OCSP multistapling

1ee91b7

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Sep 9, 2022

FdaSilvaYY reviewed Sep 9, 2022

View reviewed changes

mikrue closed this Sep 10, 2022

mikrue reopened this Sep 10, 2022

t8m added branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature labels Sep 12, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 12, 2022

openssl-machine closed this Jan 12, 2023

FdaSilvaYY mentioned this pull request May 13, 2023

add support for TLS 1.3 OCSP multi-stapling for server certs #20945

Closed

2 tasks

		X509_get_issuer_name(x));


		if(ctx->ocsp_resp == NULL \|\| sk_OCSP_RESPONSE_num(ctx->ocsp_resp) == 0) {


		cert_id = OCSP_cert_to_id(NULL, ctx->current_cert, ctx->current_issuer);

		for (i=0; i<sk_OCSP_RESPONSE_num(ctx->ocsp_resp); i++) {

Uh oh!

Comments

Conversation

mikrue commented Sep 9, 2022

Checklist

Uh oh!

FdaSilvaYY left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY commented Sep 9, 2022

Uh oh!

FdaSilvaYY left a comment

Choose a reason for hiding this comment

Uh oh!

FdaSilvaYY Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikrue commented Sep 10, 2022

Uh oh!

t8m commented Sep 12, 2022

Uh oh!

openssl-machine commented Oct 13, 2022

Uh oh!

mikrue commented Oct 13, 2022 via email

Uh oh!

FdaSilvaYY commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t8m commented Oct 13, 2022

Uh oh!

openssl-machine commented Nov 13, 2022

Uh oh!

openssl-machine commented Dec 14, 2022

Uh oh!

openssl-machine commented Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FdaSilvaYY Sep 9, 2022 •

edited

Loading

FdaSilvaYY Sep 9, 2022 •

edited

Loading

FdaSilvaYY commented Oct 13, 2022 •

edited

Loading