Clean up CMP chain building for CMP signer, TLS client, and newly enrolled certs

* Use strenghtened cert chain building, verifying chain using optional trust store
  while making sure that no certificate status (e.g., CRL) checks are done
* Use OSSL_CMP_certConf_cb() by default and move its doc to OSSL_CMP_CTX_new.pod
* Simplify certificate and cert store loading in apps/cmp.c

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/12741)

4 files changed, 67 insertions, 40 deletions

diff --git a/crypto/cmp/cmp_client.c b/crypto/cmp/cmp_client.c
index d5a4f3ced5..4f8a9e2444 100644
--- a/crypto/cmp/cmp_client.c
+++ b/crypto/cmp/cmp_client.c
@@ -22,6 +22,7 @@
 #include "openssl/cmp_util.h"
 
 DEFINE_STACK_OF(ASN1_UTF8STRING)
+DEFINE_STACK_OF(X509)
 DEFINE_STACK_OF(X509_CRL)
 DEFINE_STACK_OF(OSSL_CMP_CERTRESPONSE)
 DEFINE_STACK_OF(OSSL_CMP_PKISI)
@@ -487,14 +488,35 @@ int OSSL_CMP_certConf_cb(OSSL_CMP_CTX *ctx, X509 *cert, int fail_info,
                          const char **text)
 {
     X509_STORE *out_trusted = OSSL_CMP_CTX_get_certConf_cb_arg(ctx);
+    STACK_OF(X509) *chain = NULL;
     (void)text; /* make (artificial) use of var to prevent compiler warning */
 
     if (fail_info != 0) /* accept any error flagged by CMP core library */
         return fail_info;
 
-    if (out_trusted != NULL
-            && !OSSL_CMP_validate_cert_path(ctx, out_trusted, cert))
-        fail_info = 1 << OSSL_CMP_PKIFAILUREINFO_incorrectData;
+    ossl_cmp_debug(ctx, "trying to build chain for newly enrolled cert");
+    chain = ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq,
+                                      out_trusted /* may be NULL */,
+                                      ctx->untrusted, cert);
+    if (sk_X509_num(chain) > 0)
+        X509_free(sk_X509_shift(chain)); /* remove leaf (EE) cert */
+    if (out_trusted != NULL) {
+        if (chain == NULL) {
+            ossl_cmp_err(ctx, "failed building chain for newly enrolled cert");
+            fail_info = 1 << OSSL_CMP_PKIFAILUREINFO_incorrectData;
+        } else {
+            ossl_cmp_debug(ctx,
+                           "succeeded building proper chain for newly enrolled cert");
+        }
+    } else if (chain == NULL) {
+        ossl_cmp_warn(ctx, "could not build approximate chain for newly enrolled cert, resorting to received extraCerts");
+        chain = OSSL_CMP_CTX_get1_extraCertsIn(ctx);
+    } else {
+        ossl_cmp_debug(ctx,
+                       "success building approximate chain for newly enrolled cert");
+    }
+    (void)ossl_cmp_ctx_set1_newChain(ctx, chain);
+    sk_X509_pop_free(chain, X509_free);
 
     return fail_info;
 }
@@ -515,10 +537,14 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid,
     const char *txt = NULL;
     OSSL_CMP_CERTREPMESSAGE *crepmsg;
     OSSL_CMP_CERTRESPONSE *crep;
+    OSSL_CMP_certConf_cb_t cb;
     X509 *cert;
     char *subj = NULL;
     int ret = 1;
 
+    if (!ossl_assert(ctx != NULL))
+        return 0;
+
  retry:
     crepmsg = (*resp)->body->value.ip; /* same for cp and kup */
     if (sk_OSSL_CMP_CERTRESPONSE_num(crepmsg->response) > 1) {
@@ -584,21 +610,19 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid,
          *                    OSSL_CMP_PKISTATUS_rejection, fail_info, txt)
          * not throwing CMP_R_CERTIFICATE_NOT_ACCEPTED with txt
          * not returning 0
-         * since we better leave this for any ctx->certConf_cb to decide
+         * since we better leave this for the certConf_cb to decide
          */
     }
 
     /*
-     * Execute the certification checking callback function possibly set in ctx,
+     * Execute the certification checking callback function,
      * which can determine whether to accept a newly enrolled certificate.
      * It may overrule the pre-decision reflected in 'fail_info' and '*txt'.
      */
-    if (ctx->certConf_cb
-            && (fail_info = ctx->certConf_cb(ctx, ctx->newCert,
-                                             fail_info, &txt)) != 0) {
-        if (txt == NULL)
-            txt = "CMP client application did not accept it";
-    }
+    cb = ctx->certConf_cb != NULL ? ctx->certConf_cb : OSSL_CMP_certConf_cb;
+    if ((fail_info = cb(ctx, ctx->newCert, fail_info, &txt)) != 0
+            && txt == NULL)
+        txt = "CMP client did not accept it";
     if (fail_info != 0) /* immediately log error before any certConf exchange */
         ossl_cmp_log1(ERROR, ctx,
                       "rejecting newly enrolled cert with subject: %s", subj);
diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c
index 5b61108f8b..6bbd3510c7 100644
--- a/crypto/cmp/cmp_ctx.c
+++ b/crypto/cmp/cmp_ctx.c
@@ -189,6 +189,7 @@ void OSSL_CMP_CTX_free(OSSL_CMP_CTX *ctx)
     sk_X509_pop_free(ctx->untrusted, X509_free);
 
     X509_free(ctx->cert);
+    sk_X509_pop_free(ctx->chain, X509_free);
     EVP_PKEY_free(ctx->pkey);
     ASN1_OCTET_STRING_free(ctx->referenceValue);
     if (ctx->secretValue != NULL)
@@ -489,11 +490,7 @@ int ossl_cmp_ctx_set1_newChain(OSSL_CMP_CTX *ctx, STACK_OF(X509) *newChain)
     return (ctx->newChain = X509_chain_up_ref(newChain)) != NULL;
 }
 
-/*
- * Returns the stack of certificates received in a response message.
- * The stack is duplicated so the caller must handle freeing it!
- * Returns pointer to created stack on success, NULL on error
- */
+/* Returns the stack of extraCerts received in CertRepMessage, NULL on error */
 STACK_OF(X509) *OSSL_CMP_CTX_get1_extraCertsIn(const OSSL_CMP_CTX *ctx)
 {
     if (ctx == NULL) {
@@ -523,7 +520,7 @@ int ossl_cmp_ctx_set1_extraCertsIn(OSSL_CMP_CTX *ctx,
 }
 
 /*
- * Duplicate and set the given stack as the new stack of X509
+ * Copies any given stack as the new stack of X509
  * certificates to send out in the extraCerts field.
  */
 int OSSL_CMP_CTX_set1_extraCertsOut(OSSL_CMP_CTX *ctx,
@@ -596,7 +593,7 @@ STACK_OF(X509) *OSSL_CMP_CTX_get1_caPubs(const OSSL_CMP_CTX *ctx)
 }
 
 /*
- * Duplicate and copy the given stack of certificates to the given
+ * Copies any given stack of certificates to the given
  * OSSL_CMP_CTX structure so that they may be retrieved later.
  */
 int ossl_cmp_ctx_set1_caPubs(OSSL_CMP_CTX *ctx, STACK_OF(X509) *caPubs)
@@ -766,7 +763,7 @@ int OSSL_CMP_CTX_build_cert_chain(OSSL_CMP_CTX *ctx, X509_STORE *own_trusted,
         return 0;
     }
     ossl_cmp_debug(ctx, "success building chain for own CMP signer cert");
-    sk_X509_pop_free(chain, X509_free); /* TODO(3.0) replace this by 'ctx->chain = chain;' when ctx->chain is available */    
+    ctx->chain = chain;
     return 1;
 }
 
diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h
index d5ac7a521d..434f9e093f 100644
--- a/crypto/cmp/cmp_local.h
+++ b/crypto/cmp/cmp_local.h
@@ -71,6 +71,7 @@ struct ossl_cmp_ctx_st {
     /* client authentication */
     int unprotectedSend; /* send unprotected PKI messages */
     X509 *cert; /* protection cert used to identify and sign for MSG_SIG_ALG */
+    STACK_OF(X509) *chain; /* (cached) chain of protection cert including it */
     EVP_PKEY *pkey; /* the key pair corresponding to cert */
     ASN1_OCTET_STRING *referenceValue; /* optional user name for MSG_MAC_ALG */
     ASN1_OCTET_STRING *secretValue; /* password/shared secret for MSG_MAC_ALG */
diff --git a/crypto/cmp/cmp_protect.c b/crypto/cmp/cmp_protect.c
index b65de09517..6313cc94ce 100644
--- a/crypto/cmp/cmp_protect.c
+++ b/crypto/cmp/cmp_protect.c
@@ -139,32 +139,37 @@ int ossl_cmp_msg_add_extraCerts(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg)
             && (msg->extraCerts = sk_X509_new_null()) == NULL)
         return 0;
 
-    if (ctx->cert != NULL && ctx->pkey != NULL) {
-        /* make sure that our own cert is included in the first position */
-        if (!X509_add_cert(msg->extraCerts, ctx->cert,
-                           X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP
-                           | X509_ADD_FLAG_PREPEND))
-            return 0;
-        /* if we have untrusted certs, try to add intermediate certs */
-        if (ctx->untrusted != NULL) {
-            STACK_OF(X509) *chain;
-            int res;
+    /* Add first ctx->cert and its chain if using signature-based protection */
+    if (!ctx->unprotectedSend && ctx->secretValue == NULL) {
+        int flags_prepend = X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP
+            | X509_ADD_FLAG_PREPEND | X509_ADD_FLAG_NO_SS;
 
+        /* if not yet done try to build chain using available untrusted certs */
+        if (ctx->chain == NULL) {
             ossl_cmp_debug(ctx,
                            "trying to build chain for own CMP signer cert");
-            chain = ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq, NULL,
-                                              ctx->untrusted, ctx->cert);
-            res = X509_add_certs(msg->extraCerts, chain,
-                                 X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP
-                                 | X509_ADD_FLAG_NO_SS);
-            sk_X509_pop_free(chain, X509_free);
-            if (res == 0) {
-                ossl_cmp_err(ctx,
-                             "could not build chain for own CMP signer cert");
-                return 0;
+            ctx->chain =
+                ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq, NULL,
+                                          ctx->untrusted, ctx->cert);
+            if (ctx->chain != NULL) {
+                ossl_cmp_debug(ctx,
+                               "success building chain for own CMP signer cert");
+            } else {
+                /* dump errors to avoid confusion when printing further ones */
+                OSSL_CMP_CTX_print_errors(ctx);
+                ossl_cmp_warn(ctx,
+                              "could not build chain for own CMP signer cert");
             }
+        }
+        if (ctx->chain != NULL) {
+            if (!X509_add_certs(msg->extraCerts, ctx->chain, flags_prepend))
+                return 0;
+        } else {
+            /* make sure that at least our own signer cert is included first */
+            if (!X509_add_cert(msg->extraCerts, ctx->cert, flags_prepend))
+                return 0;
             ossl_cmp_debug(ctx,
-                           "succeeded building chain for own CMP signer cert");
+                           "fallback: adding just own CMP signer cert");
         }
     }