apps/{ca,req,x509}.c: Improve diag and doc mostly on X.509 extensions, fix multiple instances

This includes a general correction in the code (now using the X509V3_CTX_REPLACE flag)
and adding a prominent clarification in the documentation:

    If multiple entries are processed for the same extension name,
    later entries override earlier ones with the same name.

This is due to an RFC 5280 requirement - the intro of its section 4.2 says:

    A certificate MUST NOT include more than one instance of a particular extension.

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

5 files changed, 70 insertions, 57 deletions

diff --git a/apps/ca.c b/apps/ca.c
index f17acdcf73..82b008cbce 100755
--- a/apps/ca.c
+++ b/apps/ca.c
@@ -138,7 +138,7 @@ static int make_revoked(X509_REVOKED *rev, const char *str);
 static int old_entry_print(const ASN1_OBJECT *obj, const ASN1_STRING *str);
 static void write_new_certificate(BIO *bp, X509 *x, int output_der, int notext);
 
-static CONF *extconf = NULL;
+static CONF *extfile_conf = NULL;
 static int preserve = 0;
 static int msie_hack = 0;
 
@@ -761,7 +761,7 @@ end_of_options:
     /*****************************************************************/
     /* Read extensions config file                                   */
     if (extfile) {
-        if ((extconf = app_load_config(extfile)) == NULL) {
+        if ((extfile_conf = app_load_config(extfile)) == NULL) {
             ret = 1;
             goto end;
         }
@@ -772,7 +772,7 @@ end_of_options:
 
         /* We can have sections in the ext file */
         if (extensions == NULL) {
-            extensions = NCONF_get_string(extconf, "default", "extensions");
+            extensions = NCONF_get_string(extfile_conf, "default", "extensions");
             if (extensions == NULL)
                 extensions = "default";
         }
@@ -836,7 +836,20 @@ end_of_options:
                 goto end;
         }
 
-        if (extconf == NULL) {
+        if (extfile_conf != NULL) {
+            /* Check syntax of extfile */
+            X509V3_CTX ctx;
+
+            X509V3_set_ctx_test(&ctx);
+            X509V3_set_nconf(&ctx, extfile_conf);
+            if (!X509V3_EXT_add_nconf(extfile_conf, &ctx, extensions, NULL)) {
+                BIO_printf(bio_err,
+                           "Error checking certificate extensions from extfile section %s\n",
+                           extensions);
+                ret = 1;
+                goto end;
+            }
+        } else {
             /*
              * no '-extfile' option, so we look for extensions in the main
              * configuration file
@@ -847,13 +860,13 @@ end_of_options:
                     ERR_clear_error();
             }
             if (extensions != NULL) {
-                /* Check syntax of file */
+                /* Check syntax of config file section */
                 X509V3_CTX ctx;
                 X509V3_set_ctx_test(&ctx);
                 X509V3_set_nconf(&ctx, conf);
                 if (!X509V3_EXT_add_nconf(conf, &ctx, extensions, NULL)) {
                     BIO_printf(bio_err,
-                               "Error Loading extension section %s\n",
+                               "Error checking certificate extension config section %s\n",
                                extensions);
                     ret = 1;
                     goto end;
@@ -1131,7 +1144,7 @@ end_of_options:
             X509V3_set_nconf(&ctx, conf);
             if (!X509V3_EXT_add_nconf(conf, &ctx, crl_ext, NULL)) {
                 BIO_printf(bio_err,
-                           "Error Loading CRL extension section %s\n", crl_ext);
+                           "Error checking CRL extension section %s\n", crl_ext);
                 ret = 1;
                 goto end;
             }
@@ -1220,8 +1233,11 @@ end_of_options:
             X509V3_set_nconf(&crlctx, conf);
 
             if (crl_ext != NULL)
-                if (!X509V3_EXT_CRL_add_nconf(conf, &crlctx, crl_ext, crl))
+                if (!X509V3_EXT_CRL_add_nconf(conf, &crlctx, crl_ext, crl)) {
+                    BIO_printf(bio_err,
+                               "Error adding CRL extensions from section %s\n", crl_ext);
                     goto end;
+                }
             if (crlnumberfile != NULL) {
                 tmpser = BN_to_ASN1_INTEGER(crlnumber, NULL);
                 if (!tmpser)
@@ -1312,7 +1328,7 @@ end_of_options:
     X509_free(x509);
     X509_CRL_free(crl);
     NCONF_free(conf);
-    NCONF_free(extconf);
+    NCONF_free(extfile_conf);
     release_engine(e);
     return ret;
 }
@@ -1681,28 +1697,23 @@ static int do_body(X509 **xret, EVP_PKEY *pkey, X509 *x509,
 
     /* Lets add the extensions, if there are any */
     if (ext_sect) {
-        X509V3_CTX ctx;
+        X509V3_CTX ext_ctx;
 
         /* Initialize the context structure */
-        if (selfsign)
-            X509V3_set_ctx(&ctx, ret, ret, req, NULL, 0);
-        else
-            X509V3_set_ctx(&ctx, x509, ret, req, NULL, 0);
+        X509V3_set_ctx(&ext_ctx, selfsign ? ret : x509,
+                       ret, req, NULL, X509V3_CTX_REPLACE);
 
-        if (extconf != NULL) {
+        if (extfile_conf != NULL) {
             if (verbose)
                 BIO_printf(bio_err, "Extra configuration file found\n");
 
-            /* Use the extconf configuration db LHASH */
-            X509V3_set_nconf(&ctx, extconf);
-
-            /* Test the structure (needed?) */
-            /* X509V3_set_ctx_test(&ctx); */
+            /* Use the extfile_conf configuration db LHASH */
+            X509V3_set_nconf(&ext_ctx, extfile_conf);
 
             /* Adds exts contained in the configuration file */
-            if (!X509V3_EXT_add_nconf(extconf, &ctx, ext_sect, ret)) {
+            if (!X509V3_EXT_add_nconf(extfile_conf, &ext_ctx, ext_sect, ret)) {
                 BIO_printf(bio_err,
-                           "ERROR: adding extensions in section %s\n",
+                           "Error adding certificate extensions from extfile section %s\n",
                            ext_sect);
                 goto end;
             }
@@ -1711,11 +1722,11 @@ static int do_body(X509 **xret, EVP_PKEY *pkey, X509 *x509,
                            "Successfully added extensions from file.\n");
         } else if (ext_sect) {
             /* We found extensions to be set from config file */
-            X509V3_set_nconf(&ctx, lconf);
+            X509V3_set_nconf(&ext_ctx, lconf);
 
-            if (!X509V3_EXT_add_nconf(lconf, &ctx, ext_sect, ret)) {
+            if (!X509V3_EXT_add_nconf(lconf, &ext_ctx, ext_sect, ret)) {
                 BIO_printf(bio_err,
-                           "ERROR: adding extensions in section %s\n",
+                           "Error adding certificate extensions from config section %s\n",
                            ext_sect);
                 goto end;
             }
diff --git a/apps/req.c b/apps/req.c
index bc23c7d3a5..ad79866a5a 100644
--- a/apps/req.c
+++ b/apps/req.c
@@ -525,7 +525,7 @@ int req_main(int argc, char **argv)
         X509V3_set_nconf(&ctx, req_conf);
         if (!X509V3_EXT_add_nconf(req_conf, &ctx, extensions, NULL)) {
             BIO_printf(bio_err,
-                       "Error loading extension section %s\n", extensions);
+                       "Error checking x509 extension section %s\n", extensions);
             goto end;
         }
     }
@@ -535,7 +535,7 @@ int req_main(int argc, char **argv)
         X509V3_set_ctx_test(&ctx);
         X509V3_set_nconf(&ctx, addext_conf);
         if (!X509V3_EXT_add_nconf(addext_conf, &ctx, "default", NULL)) {
-            BIO_printf(bio_err, "Error loading extensions defined using -addext\n");
+            BIO_printf(bio_err, "Error checking extensions defined using -addext\n");
             goto end;
         }
     }
@@ -583,7 +583,7 @@ int req_main(int argc, char **argv)
         X509V3_set_nconf(&ctx, req_conf);
         if (!X509V3_EXT_add_nconf(req_conf, &ctx, req_exts, NULL)) {
             BIO_printf(bio_err,
-                       "Error loading request extension section %s\n",
+                       "Error checking request extension section %s\n",
                        req_exts);
             goto end;
         }
@@ -769,21 +769,21 @@ int req_main(int argc, char **argv)
 
             /* Set up V3 context struct */
 
-            X509V3_set_ctx(&ext_ctx, x509ss, x509ss, NULL, NULL, 0);
+            X509V3_set_ctx(&ext_ctx, x509ss, x509ss, NULL, NULL, X509V3_CTX_REPLACE);
             X509V3_set_nconf(&ext_ctx, req_conf);
 
             /* Add extensions */
             if (extensions != NULL && !X509V3_EXT_add_nconf(req_conf,
                                                             &ext_ctx, extensions,
                                                             x509ss)) {
-                BIO_printf(bio_err, "Error loading extension section %s\n",
+                BIO_printf(bio_err, "Error adding x509 extensions from section %s\n",
                            extensions);
                 goto end;
             }
             if (addext_conf != NULL
                 && !X509V3_EXT_add_nconf(addext_conf, &ext_ctx, "default",
                                          x509ss)) {
-                BIO_printf(bio_err, "Error loading extensions defined via -addext\n");
+                BIO_printf(bio_err, "Error adding extensions defined via -addext\n");
                 goto end;
             }
 
@@ -813,14 +813,14 @@ int req_main(int argc, char **argv)
             if (req_exts != NULL
                 && !X509V3_EXT_REQ_add_nconf(req_conf, &ext_ctx,
                                              req_exts, req)) {
-                BIO_printf(bio_err, "Error loading extension section %s\n",
+                BIO_printf(bio_err, "Error adding request extensions from section %s\n",
                            req_exts);
                 goto end;
             }
             if (addext_conf != NULL
                 && !X509V3_EXT_REQ_add_nconf(addext_conf, &ext_ctx, "default",
                                              req)) {
-                BIO_printf(bio_err, "Error loading extensions defined via -addext\n");
+                BIO_printf(bio_err, "Error adding extensions defined via -addext\n");
                 goto end;
             }
             i = do_X509_REQ_sign(req, pkey, digest, sigopts);
diff --git a/apps/x509.c b/apps/x509.c
index 42ef448416..8cd84f5afe 100644
--- a/apps/x509.c
+++ b/apps/x509.c
@@ -465,7 +465,7 @@ int x509_main(int argc, char **argv)
                     goto opthelp;
                 checkoffset = (time_t)temp;
                 if ((intmax_t)checkoffset != temp) {
-                    BIO_printf(bio_err, "%s: checkend time out of range %s\n",
+                    BIO_printf(bio_err, "%s: Checkend time out of range %s\n",
                                prog, opt_arg());
                     goto opthelp;
                 }
@@ -536,11 +536,11 @@ int x509_main(int argc, char **argv)
         CAkeyfile = CAfile;
     } else if (CA_flag && CAkeyfile == NULL) {
         BIO_printf(bio_err,
-                   "need to specify a CAkey if using the CA command\n");
+                   "Need to specify a CAkey if using the CA command\n");
         goto end;
     } else if (!CA_flag && CAkeyfile != NULL) {
         BIO_printf(bio_err,
-                   "ignoring -CAkey option since no -CA option is given\n");
+                   "Ignoring -CAkey option since no -CA option is given\n");
     }
 
     if (extfile != NULL) {
@@ -558,7 +558,7 @@ int x509_main(int argc, char **argv)
         X509V3_set_nconf(&ctx2, extconf);
         if (!X509V3_EXT_add_nconf(extconf, &ctx2, extsect, NULL)) {
             BIO_printf(bio_err,
-                       "Error Loading extension section %s\n", extsect);
+                       "Error checking extension section %s\n", extsect);
             ERR_print_errors(bio_err);
             goto end;
         }
@@ -572,7 +572,7 @@ int x509_main(int argc, char **argv)
             goto end;
 
         if ((pkey = X509_REQ_get0_pubkey(req)) == NULL) {
-            BIO_printf(bio_err, "error unpacking public key\n");
+            BIO_printf(bio_err, "Error unpacking public key\n");
             goto end;
         }
         i = do_X509_REQ_verify(req, pkey, vfyopts);
@@ -807,7 +807,7 @@ int x509_main(int argc, char **argv)
                     fdig = EVP_sha1();
 
                 if (!X509_digest(x, fdig, md, &n)) {
-                    BIO_printf(bio_err, "out of memory\n");
+                    BIO_printf(bio_err, "Out of memory\n");
                     goto end;
                 }
                 BIO_printf(out, "%s Fingerprint=",
@@ -820,7 +820,6 @@ int x509_main(int argc, char **argv)
 
             /* should be in the library */
             else if (sign_flag == i && x509req == 0) {
-                BIO_printf(bio_err, "Getting Private key\n");
                 if (Upkey == NULL) {
                     Upkey = load_key(keyfile, keyformat, 0,
                                      passin, e, "private key");
@@ -835,7 +834,6 @@ int x509_main(int argc, char **argv)
                     goto end;
                 }
             } else if (CA_flag == i) {
-                BIO_printf(bio_err, "Getting CA Private Key\n");
                 if (CAkeyfile != NULL) {
                     CApkey = load_key(CAkeyfile, CAkeyformat,
                                       0, passin, e, "CA private key");
@@ -851,9 +849,8 @@ int x509_main(int argc, char **argv)
             } else if (x509req == i) {
                 EVP_PKEY *pk;
 
-                BIO_printf(bio_err, "Getting request Private Key\n");
                 if (keyfile == NULL) {
-                    BIO_printf(bio_err, "no request key file specified\n");
+                    BIO_printf(bio_err, "No request key file specified\n");
                     goto end;
                 } else {
                     pk = load_key(keyfile, keyformat, 0,
@@ -862,8 +859,6 @@ int x509_main(int argc, char **argv)
                         goto end;
                 }
 
-                BIO_printf(bio_err, "Generating certificate request\n");
-
                 rq = X509_to_X509_REQ(x, pk, digest);
                 EVP_PKEY_free(pk);
                 if (rq == NULL) {
@@ -911,11 +906,11 @@ int x509_main(int argc, char **argv)
         else
             i = PEM_write_bio_X509(out, x);
     } else {
-        BIO_printf(bio_err, "bad output format specified for outfile\n");
+        BIO_printf(bio_err, "Bad output format specified for outfile\n");
         goto end;
     }
     if (!i) {
-        BIO_printf(bio_err, "unable to write certificate\n");
+        BIO_printf(bio_err, "Unable to write certificate\n");
         ERR_print_errors(bio_err);
         goto end;
     }
@@ -965,7 +960,7 @@ static ASN1_INTEGER *x509_load_serial(const char *CAfile,
         goto end;
 
     if (!BN_add_word(serial, 1)) {
-        BIO_printf(bio_err, "add_word failure\n");
+        BIO_printf(bio_err, "Serial number increment failure\n");
         goto end;
     }
 
@@ -1059,13 +1054,13 @@ static int callb(int ok, X509_STORE_CTX *ctx)
      */
     if (ok) {
         BIO_printf(bio_err,
-                   "error with certificate to be certified - should be self-signed\n");
+                   "Error with certificate to be certified - should be self-signed\n");
         return 0;
     } else {
         err_cert = X509_STORE_CTX_get_current_cert(ctx);
         print_name(bio_err, NULL, X509_get_subject_name(err_cert), 0);
         BIO_printf(bio_err,
-                   "error with certificate - error %d at depth %d\n%s\n", err,
+                   "Error with certificate - error %d at depth %d\n%s\n", err,
                    X509_STORE_CTX_get_error_depth(ctx),
                    X509_verify_cert_error_string(err));
         return 1;
@@ -1089,12 +1084,15 @@ static int sign(X509 *x, EVP_PKEY *pkey, X509 *issuer,
             X509_delete_ext(x, 0);
     }
     if (conf != NULL) {
-        X509V3_CTX ctx;
+        X509V3_CTX ext_ctx;
 
-        X509V3_set_ctx(&ctx, issuer, x, NULL, NULL, 0);
-        X509V3_set_nconf(&ctx, conf);
-        if (!X509V3_EXT_add_nconf(conf, &ctx, section, x))
+        X509V3_set_ctx(&ext_ctx, issuer, x, NULL, NULL, X509V3_CTX_REPLACE);
+        X509V3_set_nconf(&ext_ctx, conf);
+        if (!X509V3_EXT_add_nconf(conf, &ext_ctx, section, x)) {
+            BIO_printf(bio_err,
+                       "Error adding extensions from section %s\n", section);
             return 0;
+        }
     }
     return do_X509_sign(x, pkey, digest, sigopts);
 }
diff --git a/doc/man5/x509v3_config.pod b/doc/man5/x509v3_config.pod
index a20065a8d9..cf08f78695 100644
--- a/doc/man5/x509v3_config.pod
+++ b/doc/man5/x509v3_config.pod
@@ -7,8 +7,9 @@ x509v3_config - X509 V3 certificate extension configuration format
 =head1 DESCRIPTION
 
 Several OpenSSL commands can add extensions to a certificate or
-certificate request based on the contents of a configuration file.
-The syntax of this file is described in L<config(5)>.
+certificate request based on the contents of a configuration file
+and CLI options such as B<-addext>.
+The syntax of configuration files is described in L<config(5)>.
 The commands typically have an option to specify the name of the configuration
 file, and a section within that file; see the documentation of the
 individual command for details.
@@ -22,6 +23,9 @@ Each entry in the extension section takes the form:
 
 If B<critical> is present then the extension will be marked as critical.
 
+If multiple entries are processed for the same extension name,
+later entries override earlier ones with the same name.
+
 The format of B<values> depends on the value of B<name>, many have a
 type-value pairing where the type and value are separated by a colon.
 There are four main types of extension:
diff --git a/test/recipes/25-test_x509.t b/test/recipes/25-test_x509.t
index 54fbe78e96..19ff335f82 100644
--- a/test/recipes/25-test_x509.t
+++ b/test/recipes/25-test_x509.t
@@ -126,7 +126,7 @@ sub test_errors { # actually tests diagnostics of OSSL_STORE
 
 # 3 tests for non-existence of spurious OSSL_STORE ASN.1 parse error output.
 # This requires provoking a failure exit of the app after reading input files.
-ok(test_errors("bad output format", "root-cert.pem", '-outform', 'http'),
+ok(test_errors("Bad output format", "root-cert.pem", '-outform', 'http'),
    "load root-cert errors");
 ok(test_errors("RC2-40-CBC", "v3-certs-RC2.p12", '-passin', 'pass:v3-certs'),
    "load v3-certs-RC2 no asn1 errors"); # error msg should mention "RC2-40-CBC"