apps/cmp.c: Clean up loading of certificates and CRLs

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

2 files changed, 67 insertions, 112 deletions

diff --git a/apps/cmp.c b/apps/cmp.c
index 4a8b6e7..4d6acdd 100644
--- a/apps/cmp.c
+++ b/apps/cmp.c
@@ -303,7 +303,7 @@ const OPTIONS cmp_options[] = {
     {OPT_MORE_STR, 0, 0,
      "-1 = NONE, 0 = RAVERIFIED, 1 = SIGNATURE (default), 2 = KEYENC"},
     {"csr", OPT_CSR, 's',
-     "CSR file in PKCS#10 format to use in p10cr for legacy support"},
+     "PKCS#10 CSR file in PEM or DER format to use in p10cr for legacy support"},
     {"out_trusted", OPT_OUT_TRUSTED, 's',
      "Certificates to trust when verifying newly enrolled certificates"},
     {"implicit_confirm", OPT_IMPLICIT_CONFIRM, '-',
@@ -653,42 +653,6 @@ static X509 *load_cert_pwd(const char *uri, const char *pass, const char *desc)
     return cert;
 }
 
-/* TODO potentially move this and related functions to apps/lib/apps.c */
-static int adjust_format(const char **infile, int format, int engine_ok)
-{
-    if (!strncasecmp(*infile, "http://", 7)
-            || !strncasecmp(*infile, "https://", 8)) {
-        format = FORMAT_HTTP;
-    } else if (engine_ok && strncasecmp(*infile, "engine:", 7) == 0) {
-        *infile += 7;
-        format = FORMAT_ENGINE;
-    } else {
-        if (strncasecmp(*infile, "file:", 5) == 0)
-            *infile += 5;
-        /*
-         * the following is a heuristic whether first to try PEM or DER
-         * or PKCS12 as the input format for files
-         */
-        if (strlen(*infile) >= 4) {
-            const char *extension = *infile + strlen(*infile) - 4;
-
-            if (strncasecmp(extension, ".crt", 4) == 0
-                    || strncasecmp(extension, ".pem", 4) == 0)
-                /* weak recognition of PEM format */
-                format = FORMAT_PEM;
-            else if (strncasecmp(extension, ".cer", 4) == 0
-                         || strncasecmp(extension, ".der", 4) == 0)
-                /* weak recognition of DER format */
-                format = FORMAT_ASN1;
-            else if (strncasecmp(extension, ".p12", 4) == 0)
-                /* weak recognition of PKCS#12 format */
-                format = FORMAT_PKCS12;
-            /* else retain given format */
-        }
-    }
-    return format;
-}
-
 /*
  * TODO potentially move this and related functions to apps/lib/
  * or even better extend OSSL_STORE with type OSSL_STORE_INFO_CRL
@@ -697,18 +661,13 @@ static X509_REQ *load_csr_autofmt(const char *infile, const char *desc)
 {
     X509_REQ *csr;
     BIO *bio_bak = bio_err;
-    int can_retry;
-    int format = adjust_format(&infile, FORMAT_PEM, 0);
 
-    can_retry = format == FORMAT_PEM || format == FORMAT_ASN1;
-    if (can_retry)
-        bio_err = NULL; /* do not show errors on more than one try */
-    csr = load_csr(infile, format, desc);
+    bio_err = NULL; /* do not show errors on more than one try */
+    csr = load_csr(infile, FORMAT_PEM, desc);
     bio_err = bio_bak;
-    if (csr == NULL && can_retry) {
+    if (csr == NULL) {
         ERR_clear_error();
-        format = (format == FORMAT_PEM ? FORMAT_ASN1 : FORMAT_PEM);
-        csr = load_csr(infile, format, desc);
+        csr = load_csr(infile, FORMAT_ASN1, desc);
     }
     if (csr == NULL) {
         ERR_print_errors(bio_err);
@@ -718,43 +677,59 @@ static X509_REQ *load_csr_autofmt(const char *infile, const char *desc)
     return csr;
 }
 
-static void warn_certs_expired(const char *file, STACK_OF(X509) **certs)
+static void warn_cert_msg(const char *uri, X509 *cert, const char *msg)
 {
-    int i, res;
-    X509 *cert;
-    char *subj;
+    char *subj = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0);
+
+    CMP_warn3("certificate from '%s' with subject '%s' %s", uri, subj, msg);
+    OPENSSL_free(subj);
+}
 
-    for (i = 0; i < sk_X509_num(*certs); i++) {
-        cert = sk_X509_value(*certs, i);
-        res = X509_cmp_timeframe(vpm, X509_get0_notBefore(cert),
+static void warn_cert(const char *uri, X509 *cert, int warn_EE)
+{
+    int res = X509_cmp_timeframe(vpm, X509_get0_notBefore(cert),
                                  X509_get0_notAfter(cert));
-        if (res != 0) {
-            subj = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0);
-            CMP_warn3("certificate from '%s' with subject '%s' %s", file, subj,
-                      res > 0 ? "has expired" : "not yet valid");
-            OPENSSL_free(subj);
-        }
-    }
+
+    if (res != 0)
+        warn_cert_msg(uri, cert, res > 0 ? "has expired" : "not yet valid");
+    if (warn_EE && (X509_get_extension_flags(cert) & EXFLAG_CA) == 0)
+        warn_cert_msg(uri, cert, "is not a CA cert");
 }
 
-static int load_certs_pwd(const char *infile, STACK_OF(X509) **certs,
-                          int exclude_http, const char *pass,
-                          const char *desc)
+static void warn_certs(const char *uri, STACK_OF(X509) *certs, int warn_EE)
+{
+    int i;
+
+    for (i = 0; i < sk_X509_num(certs); i++)
+        warn_cert(uri, sk_X509_value(certs, i), warn_EE);
+}
+
+/* TODO potentially move this and related functions to apps/lib/apps.c */
+static int load_cert_certs(const char *uri,
+                           X509 **pcert, STACK_OF(X509) **pcerts,
+                           int exclude_http, const char *pass, const char *desc)
 {
     int ret = 0;
     char *pass_string;
-    int format = adjust_format(&infile, FORMAT_PEM, 0);
 
-    if (exclude_http && format == FORMAT_HTTP) {
+    if (exclude_http && (strncasecmp(uri, "http://", 7) == 0
+                         || strncasecmp(uri, "https://", 8) == 0)) {
         BIO_printf(bio_err, "error: HTTP retrieval not allowed for %s\n", desc);
         return ret;
     }
     pass_string = get_passwd(pass, desc);
-    ret = load_certs(infile, certs, pass_string, desc);
+    ret = load_key_certs_crls(uri, 0, pass_string, desc, NULL, NULL,
+                              pcert, pcerts, NULL, NULL);
     clear_free(pass_string);
 
-    if (ret)
-        warn_certs_expired(infile, certs);
+    if (ret) {
+        if (pcert != NULL)
+            warn_cert(uri, *pcert, 0);
+        warn_certs(uri, *pcerts, 1);
+    } else {
+        sk_X509_pop_free(*pcerts, X509_free);
+        *pcerts = NULL;
+    }
     return ret;
 }
 
@@ -1034,25 +1009,21 @@ static X509_STORE *load_certstore(char *input, const char *desc)
     X509_STORE *store = NULL;
     STACK_OF(X509) *certs = NULL;
 
-    if (input == NULL)
-        goto err;
-
     while (input != NULL) {
-        char *next = next_item(input);           \
+        char *next = next_item(input);
+        int ok;
 
-        if (!load_certs_pwd(input, &certs, 1, opt_otherpass, desc)
-                || !(store = sk_X509_to_store(store, certs))) {
-            /* CMP_err("out of memory"); */
+        if (!load_cert_certs(input, NULL, &certs, 1, opt_otherpass, desc)) {
             X509_STORE_free(store);
-            store = NULL;
-            goto err;
+            return NULL;
         }
+        ok = (store = sk_X509_to_store(store, certs)) != NULL;
         sk_X509_pop_free(certs, X509_free);
         certs = NULL;
+        if (!ok)
+            return NULL;
         input = next;
     }
- err:
-    sk_X509_pop_free(certs, X509_free);
     return store;
 }
 
@@ -1071,7 +1042,7 @@ static STACK_OF(X509) *load_certs_multifile(char *files,
     while (files != NULL) {
         char *next = next_item(files);
 
-        if (!load_certs_pwd(files, &certs, 0, pass, desc))
+        if (!load_cert_certs(files, NULL, &certs, 0, pass, desc))
             goto err;
         if (!X509_add_certs(result, certs,
                             X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP))
@@ -1254,7 +1225,8 @@ static OSSL_CMP_SRV_CTX *setup_srv_ctx(ENGINE *engine)
     } else {
         CMP_warn("server will not be able to handle signature-protected requests since -srv_trusted is not given");
     }
-    if (!setup_certs(opt_srv_untrusted, "untrusted certificates", ctx,
+    if (!setup_certs(opt_srv_untrusted,
+                     "untrusted certificates for mock server", ctx,
                      (add_X509_stack_fn_t)OSSL_CMP_CTX_set1_untrusted_certs,
                      NULL))
         goto err;
@@ -1458,31 +1430,23 @@ static SSL_CTX *setup_ssl_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
     if (opt_tls_cert != NULL && opt_tls_key != NULL) {
         X509 *cert;
         STACK_OF(X509) *certs = NULL;
+        int ok;
 
-        if (!load_certs_pwd(opt_tls_cert, &certs, 0, opt_tls_keypass,
-                            "TLS client certificate (optionally with chain)"))
-            /*
-             * opt_tls_keypass is needed in case opt_tls_cert is an encrypted
-             * PKCS#12 file
-             */
+        if (!load_cert_certs(opt_tls_cert, &cert, &certs, 0, opt_tls_keypass,
+                             "TLS client certificate (optionally with chain)"))
+            /* need opt_tls_keypass if opt_tls_cert is encrypted PKCS#12 file */
             goto err;
 
-        cert = sk_X509_delete(certs, 0);
-        if (cert == NULL || SSL_CTX_use_certificate(ssl_ctx, cert) <= 0) {
-            CMP_err1("unable to use client TLS certificate file '%s'",
-                     opt_tls_cert);
-            X509_free(cert);
-            sk_X509_pop_free(certs, X509_free);
-            goto err;
-        }
-        X509_free(cert); /* we do not need the handle any more */
+        ok = SSL_CTX_use_certificate(ssl_ctx, cert) > 0;
+        X509_free(cert);
 
         /*
          * Any further certs and any untrusted certs are used for constructing
          * the chain to be provided with the TLS client cert to the TLS server.
          */
-        if (!SSL_CTX_set0_chain(ssl_ctx, certs)) {
-            CMP_err("could not set TLS client cert chain");
+        if (!ok || !SSL_CTX_set0_chain(ssl_ctx, certs)) {
+            CMP_err1("unable to use client TLS certificate file '%s'",
+                     opt_tls_cert);
             sk_X509_pop_free(certs, X509_free);
             goto err;
         }
@@ -1628,19 +1592,12 @@ static int setup_protection_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
     if (opt_cert != NULL) {
         X509 *cert;
         STACK_OF(X509) *certs = NULL;
-        int ok;
+        int ok = 0;
 
-        if (!load_certs_pwd(opt_cert, &certs, 0, opt_keypass,
-                            "CMP client certificate (and optionally extra certs)"))
+        if (!load_cert_certs(opt_cert, &cert, &certs, 0, opt_keypass,
+                             "CMP client certificate (optionally with chain)"))
             /* opt_keypass is needed if opt_cert is an encrypted PKCS#12 file */
             goto err;
-
-        cert = sk_X509_delete(certs, 0);
-        if (cert == NULL) {
-            CMP_err("no client certificate found");
-            sk_X509_pop_free(certs, X509_free);
-            goto err;
-        }
         ok = OSSL_CMP_CTX_set1_cert(ctx, cert);
         X509_free(cert);
 
@@ -1654,7 +1611,7 @@ static int setup_protection_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
         }
         sk_X509_pop_free(certs, X509_free);
         if (!ok)
-            goto oom;
+            goto err;
     }
 
     if (!setup_certs(opt_extracerts, "extra certificates for CMP", ctx,
@@ -1690,8 +1647,6 @@ static int setup_protection_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
     }
     return 1;
 
- oom:
-    CMP_err("out of memory");
  err:
     return 0;
 }
diff --git a/doc/man1/openssl-cmp.pod.in b/doc/man1/openssl-cmp.pod.in
index 77d38d9..760e21c 100644
--- a/doc/man1/openssl-cmp.pod.in
+++ b/doc/man1/openssl-cmp.pod.in
@@ -335,7 +335,7 @@ is provided via the B<-newkey> or B<-key> options.
 
 =item B<-csr> I<filename>
 
-CSR in PKCS#10 format to use in legacy P10CR messages.
+PKCS#10 CSR in PEM or DER format to use in legacy P10CR messages.
 
 =item B<-out_trusted> I<filenames>