Restore previous semantics of authorityKeyIdentifier=keyid

when the function X509V3_set_issuer_pkey is not used, since that
would break code written for 1.1.1 and before, where e.g.
X509V3_EXT_conf(NULL, &ctx, "authorityKeyIdentifier", "keyid,issuer");
was guaranteed to never fail and always generate a valid
extension, and not an empty AKID.
But due to the change, which happened between 1.1.1 and 3.0
that is no longer the case.
Code written for 3.0 must use X509V3_set_issuer_pkey and
detect the possible empty extensions, or use keyid:always
and be prepared that it may be impossible to create this
extension.

Author: bernd-edlinger

crypto/x509/v3_akid.c

@@ -107,7 +107,7 @@ static AUTHORITY_KEYID *v2i_AUTHORITY_KEYID(X509V3_EXT_METHOD *method,
     ASN1_INTEGER *serial = NULL;
     X509_EXTENSION *ext;
     X509 *issuer_cert;
-    int same_issuer, ss;
+    int self_signed = 0;
     AUTHORITY_KEYID *akeyid = AUTHORITY_KEYID_new();
 
     if (akeyid == NULL)
@@ -156,53 +156,50 @@ static AUTHORITY_KEYID *v2i_AUTHORITY_KEYID(X509V3_EXT_METHOD *method,
         ERR_raise(ERR_LIB_X509V3, X509V3_R_NO_ISSUER_CERTIFICATE);
         goto err;
     }
-    same_issuer = ctx->subject_cert == ctx->issuer_cert;
-    ERR_set_mark();
-    if (ctx->issuer_pkey != NULL)
-        ss = X509_check_private_key(ctx->subject_cert, ctx->issuer_pkey);
-    else
-        ss = same_issuer;
-    ERR_pop_to_mark();
+
+    if (ctx->subject_cert != NULL && ctx->issuer_pkey != NULL) {
+        ERR_set_mark();
+        self_signed = X509_check_private_key(ctx->subject_cert,
+                                             ctx->issuer_pkey);
+        ERR_pop_to_mark();
+    }
 
     /* unless forced with "always", AKID is suppressed for self-signed certs */
-    if (keyid == 2 || (keyid == 1 && !ss)) {
+    if (keyid == 2 || (keyid == 1 && !self_signed)) {
         /*
          * prefer any pre-existing subject key identifier of the issuer cert
-         * except issuer cert is same as subject cert and is not self-signed
+         * except issuer cert is same as subject cert and private key is given
          */
-        i = X509_get_ext_by_NID(issuer_cert, NID_subject_key_identifier, -1);
-        if (i >= 0 && (ext = X509_get_ext(issuer_cert, i)) != NULL
-            && !(same_issuer && !ss)) {
-            ikeyid = X509V3_EXT_d2i(ext);
-            if (ASN1_STRING_length(ikeyid) == 0) /* indicating "none" */ {
-                ASN1_OCTET_STRING_free(ikeyid);
-                ikeyid = NULL;
-            }
-        }
-        if (ikeyid == NULL && same_issuer && ctx->issuer_pkey != NULL) {
+        if (ctx->subject_cert == issuer_cert && ctx->issuer_pkey != NULL) {
             /* generate fallback AKID, emulating s2i_skey_id(..., "hash") */
             X509_PUBKEY *pubkey = NULL;
 
             if (X509_PUBKEY_set(&pubkey, ctx->issuer_pkey))
                 ikeyid = ossl_x509_pubkey_hash(pubkey);
             X509_PUBKEY_free(pubkey);
+        } else {
+            i = X509_get_ext_by_NID(issuer_cert, NID_subject_key_identifier, -1);
+            if (i >= 0 && (ext = X509_get_ext(issuer_cert, i)) != NULL) {
+                ikeyid = X509V3_EXT_d2i(ext);
+                if (ASN1_STRING_length(ikeyid) == 0) /* indicating "none" */ {
+                    ASN1_OCTET_STRING_free(ikeyid);
+                    ikeyid = NULL;
+                }
+            }
         }
         if (keyid == 2 && ikeyid == NULL) {
             ERR_raise(ERR_LIB_X509V3, X509V3_R_UNABLE_TO_GET_ISSUER_KEYID);
             goto err;
         }
     }
 
-    if (issuer == 2 || (issuer == 1 && !ss && ikeyid == NULL)) {
+    if (issuer == 2 || (issuer == 1 && ikeyid == NULL && !self_signed)) {
         isname = X509_NAME_dup(X509_get_issuer_name(issuer_cert));
         serial = ASN1_INTEGER_dup(X509_get0_serialNumber(issuer_cert));
         if (isname == NULL || serial == NULL) {
             ERR_raise(ERR_LIB_X509V3, X509V3_R_UNABLE_TO_GET_ISSUER_DETAILS);
             goto err;
         }
-    }
-
-    if (isname != NULL) {
         if ((gens = sk_GENERAL_NAME_new_null()) == NULL
             || (gen = GENERAL_NAME_new()) == NULL) {
             ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB);
@@ -217,8 +214,6 @@ static AUTHORITY_KEYID *v2i_AUTHORITY_KEYID(X509V3_EXT_METHOD *method,
     }
 
     akeyid->issuer = gens;
-    gen = NULL;
-    gens = NULL;
     akeyid->serial = serial;
     akeyid->keyid = ikeyid;
 

doc/man5/x509v3_config.pod

@@ -208,13 +208,17 @@ or both of them, separated by C<,>.
 Either or both can have the option B<always>,
 indicated by putting a colon C<:> between the value and this option.
 For self-signed certificates the AKID is suppressed unless B<always> is present.
+In order to find out, if the certificate is about to be self-signed, the
+private key as given by B<X509V3_set_issuer_pkey> is compared to the
+public key in the subject_certificate.
+When there is no private key available, the AKID will not be suppressed.
 
 By default the B<x509>, B<req>, and B<ca> apps behave as if B<none> was given
 for self-signed certificates and B<keyid>C<,> B<issuer> otherwise.
 
 If B<keyid> is present, an attempt is made to
 copy the subject key identifier (SKID) from the issuer certificate except if
-the issuer certificate is the same as the current one and it is not self-signed.
+the issuer certificate does not have the subject key identifier extension.
 The hash of the public key related to the signing key is taken as fallback
 if the issuer certificate is the same as the current certificate.
 If B<always> is present but no value can be obtained, an error is returned.

test/x509_test.c

@@ -10,6 +10,7 @@
 #define OPENSSL_SUPPRESS_DEPRECATED /* EVP_PKEY_get1/set1_RSA */
 
 #include <openssl/x509.h>
+#include <openssl/x509v3.h>
 #include <openssl/asn1.h>
 #include <openssl/evp.h>
 #include <openssl/rsa.h>
@@ -282,6 +283,83 @@ static int test_x509_revoked_delete_last_extension(void)
     return ret;
 }
 
+static int test_x509_old_abi_compat_mode_extension(void)
+{
+    static const unsigned char commonName[] = "test";
+    X509 *x = NULL;
+    X509_NAME *subject = NULL;
+    X509_NAME_ENTRY *name_entry = NULL;
+    X509_EXTENSION *ext = NULL;
+    X509V3_CTX ctx;
+    const STACK_OF(X509_EXTENSION) *exts;
+    ASN1_OCTET_STRING *encoded;
+    int ret = 0;
+
+    if (!TEST_ptr(x = X509_new())
+            || !TEST_int_eq(X509_set_version(x, X509_VERSION_3), 1)
+            || !TEST_int_eq(ASN1_INTEGER_set(X509_get_serialNumber(x), 1), 1)
+            || !TEST_ptr(subject = X509_NAME_new()))
+        goto err;
+
+    name_entry = X509_NAME_ENTRY_create_by_NID(NULL, NID_commonName,
+                                               MBSTRING_ASC, commonName, -1);
+    if (!TEST_ptr(name_entry)
+            || !TEST_int_eq(X509_NAME_add_entry(subject, name_entry, -1, 0), 1)
+            || !TEST_int_eq(X509_set_subject_name(x, subject), 1)
+            || !TEST_int_eq(X509_set_issuer_name(x, subject), 1)
+            || !TEST_ptr(X509_gmtime_adj(X509_getm_notBefore(x), 0))
+            || !TEST_ptr(X509_gmtime_adj(X509_getm_notAfter(x), 24 * 3600))
+            || !TEST_int_eq(X509_set_pubkey(x, pubkey), 1))
+        goto err;
+
+    X509V3_set_ctx(&ctx, x, x, NULL, NULL, 0);
+    /*
+     * By not calling X509V3_set_issuer_pkey(&ctx, privkey);
+     * here, we are exempt from the 3.x convention, regarding
+     * the authorityKeyIdentifier=keyid,issuer which may return
+     * an empty extension for self signed certificates.
+     * Therefore an empty extension cannot be produced here.
+     */
+    if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "subjectKeyIdentifier",
+                                        "hash"))
+            || !TEST_int_eq(X509_add_ext(x, ext, -1), 1))
+        goto err;
+
+    X509_EXTENSION_free(ext);
+    if (!TEST_ptr(ext = X509V3_EXT_conf(NULL, &ctx, "authorityKeyIdentifier",
+                                        "keyid,issuer"))
+            || !TEST_int_eq(X509_add_ext(x, ext, -1), 1))
+        goto err;
+
+    if (!TEST_int_gt(X509_sign(x, privkey, signmd), 0)
+            || !TEST_int_gt(X509_verify(x, pubkey), 0))
+        goto err;
+
+    exts = X509_get0_extensions(x);
+    if (!TEST_ptr(exts)
+            || !TEST_int_eq(sk_X509_EXTENSION_num(exts), 2))
+        goto err;
+
+    encoded = X509_EXTENSION_get_data(X509v3_get_ext(exts, 0));
+    if (!TEST_ptr(encoded)
+            || !TEST_int_gt(ASN1_STRING_length(encoded), 2))
+        goto err;
+
+    encoded = X509_EXTENSION_get_data(X509v3_get_ext(exts, 1));
+    if (!TEST_ptr(encoded)
+            || !TEST_int_gt(ASN1_STRING_length(encoded), 2))
+        goto err;
+
+    ret = 1;
+
+err:
+    X509_NAME_ENTRY_free(name_entry);
+    X509_NAME_free(subject);
+    X509_EXTENSION_free(ext);
+    X509_free(x);
+    return ret;
+}
+
 OPT_TEST_DECLARE_USAGE("<pss-self-signed-cert.pem>\n")
 
 int setup_tests(void)
@@ -319,6 +397,7 @@ int setup_tests(void)
     ADD_TEST(test_x509_delete_last_extension);
     ADD_TEST(test_x509_crl_delete_last_extension);
     ADD_TEST(test_x509_revoked_delete_last_extension);
+    ADD_TEST(test_x509_old_abi_compat_mode_extension);
     return 1;
 }