Don't allow crl-signing issuer usage without CRLSign KeyUsage (#16865)

* Allow correct importing of certs without CRL KU

When Vault imports certificates without KU for CRLSign, we shouldn't
provision CRLUsage on the backing issuer; otherwise, we'll attempt to
build CRLs and Go will cause us to err out. This change makes it clear
(at issuer configuration time) that we can't possibly support this
operation and hopefully prevent users from running into the more cryptic
Go error.

Note that this does not apply for OCSP EKU: the EKU exists, per RFC 6960
Section 2.6 OCSP Signature Authority Delegation, to allow delegation of
OCSP signing to a child certificate. This EKU is not necessary on the
issuer itself, and generally assumes issuers are allowed to issue OCSP
responses regardless of KU/EKU.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add docs to clarify issue with import, CRL usage

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Update website/content/api-docs/secret/pki.mdx

* Add additional test assertion

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/logical/pki/backend_test.go

@@ -4999,6 +4999,96 @@ func TestPerIssuerAIA(t *testing.T) {
 	require.Equal(t, leafCert.CRLDistributionPoints, []string{"https://example.com/crl", "https://backup.example.com/crl"})
 }
 
+func TestIssuersWithoutCRLBits(t *testing.T) {
+	t.Parallel()
+	b, s := createBackendWithStorage(t)
+
+	// Importing a root without CRL signing bits should work fine.
+	customBundleWithoutCRLBits := `
+-----BEGIN CERTIFICATE-----
+MIIDGTCCAgGgAwIBAgIBATANBgkqhkiG9w0BAQsFADATMREwDwYDVQQDDAhyb290
+LW5ldzAeFw0yMjA4MjQxMjEzNTVaFw0yMzA5MDMxMjEzNTVaMBMxETAPBgNVBAMM
+CHJvb3QtbmV3MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAojTA/Mx7
+LVW/Zgn/N4BqZbaF82MrTIBFug3ob7mqycNRlWp4/PH8v37+jYn8e691HUsKjden
+rDTrO06kiQKiJinAzmlLJvgcazE3aXoh7wSzVG9lFHYvljEmVj+yDbkeaqaCktup
+skuNjxCoN9BLmKzZIwVCHn92ZHlhN6LI7CNaU3SDJdu7VftWF9Ugzt9FIvI+6Gcn
+/WNE9FWvZ9o7035rZ+1vvTn7/tgxrj2k3XvD51Kq4tsSbqjnSf3QieXT6E6uvtUE
+TbPp3xjBElgBCKmeogR1l28rs1aujqqwzZ0B/zOeF8ptaH0aZOIBsVDJR8yTwHzq
+s34hNdNfKLHzOwIDAQABo3gwdjAdBgNVHQ4EFgQUF4djNmx+1+uJINhZ82pN+7jz
+H8EwHwYDVR0jBBgwFoAUF4djNmx+1+uJINhZ82pN+7jzH8EwDwYDVR0TAQH/BAUw
+AwEB/zAOBgNVHQ8BAf8EBAMCAoQwEwYDVR0lBAwwCgYIKwYBBQUHAwEwDQYJKoZI
+hvcNAQELBQADggEBAICQovBz4KLWlLmXeZ2Vf6WfQYyGNgGyJa10XNXtWQ5dM2NU
+OLAit4x1c2dz+aFocc8ZsX/ikYi/bruT2rsGWqMAGC4at3U4GuaYGO5a6XzMKIDC
+nxIlbiO+Pn6Xum7fAqUri7+ZNf/Cygmc5sByi3MAAIkszeObUDZFTJL7gEOuXIMT
+rKIXCINq/U+qc7m9AQ8vKhF1Ddj+dLGLzNQ5j3cKfilPs/wRaYqbMQvnmarX+5Cs
+k1UL6kWSQsiP3+UWaBlcWkmD6oZ3fIG7c0aMxf7RISq1eTAM9XjH3vMxWQJlS5q3
+2weJ2LYoPe/DwX5CijR0IezapBCrin1BscJMLFQ=
+-----END CERTIFICATE-----
+-----BEGIN PRIVATE KEY-----
+MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCiNMD8zHstVb9m
+Cf83gGpltoXzYytMgEW6DehvuarJw1GVanj88fy/fv6Nifx7r3UdSwqN16esNOs7
+TqSJAqImKcDOaUsm+BxrMTdpeiHvBLNUb2UUdi+WMSZWP7INuR5qpoKS26myS42P
+EKg30EuYrNkjBUIef3ZkeWE3osjsI1pTdIMl27tV+1YX1SDO30Ui8j7oZyf9Y0T0
+Va9n2jvTfmtn7W+9Ofv+2DGuPaTde8PnUqri2xJuqOdJ/dCJ5dPoTq6+1QRNs+nf
+GMESWAEIqZ6iBHWXbyuzVq6OqrDNnQH/M54Xym1ofRpk4gGxUMlHzJPAfOqzfiE1
+018osfM7AgMBAAECggEAAVd6kZZaN69IZITIc1vHRYa2rlZpKS2JP7c8Vd3Z/4Fz
+ZZvnJ7LgVAmUYg5WPZ2sOqBNLfKVN/oke5Q0dALgdxYl7dWQIhPjHeRFbZFtjqEV
+OXZGBniamMO/HSKGWGrqFf7BM/H7AhClUwQgjnzVSz+B+LJJidM+SVys3n1xuDmC
+EP+iOda+bAHqHv/7oCELQKhLmCvPc9v2fDy+180ttdo8EHuxwVnKiyR/ryKFhSyx
+K1wgAPQ9jO+V+GESL90rqpX/r501REsIOOpm4orueelHTD4+dnHxvUPqJ++9aYGX
+79qBNPPUhxrQI1yoHxwW0cTxW5EqkZ9bT2lSd5rjcQKBgQDNyPBpidkHPrYemQDT
+RldtS6FiW/jc1It/CRbjU4A6Gi7s3Cda43pEUObKNLeXMyLQaMf4GbDPDX+eh7B8
+RkUq0Q/N0H4bn1hbxYSUdgv0j/6czpMo6rLcJHGwOTSpHGsNsxSLL7xlpgzuzqrG
+FzEgjMA1aD3w8B9+/77AoSLoMQKBgQDJyYMw82+euLYRbR5Wc/SbrWfh2n1Mr2BG
+pp1ZNYorXE5CL4ScdLcgH1q/b8r5XGwmhMcpeA+geAAaKmk1CGG+gPLoq20c9Q1Y
+Ykq9tUVJasIkelvbb/SPxyjkJdBwylzcPP14IJBsqQM0be+yVqLJJVHSaoKhXZcl
+IW2xgCpjKwKBgFpeX5U5P+F6nKebMU2WmlYY3GpBUWxIummzKCX0SV86mFjT5UR4
+mPzfOjqaI/V2M1eqbAZ74bVLjDumAs7QXReMb5BGetrOgxLqDmrT3DQt9/YMkXtq
+ddlO984XkRSisjB18BOfhvBsl0lX4I7VKHHO3amWeX0RNgOjc7VMDfRBAoGAWAQH
+r1BfvZHACLXZ58fISCdJCqCsysgsbGS8eW77B5LJp+DmLQBT6DUE9j+i/0Wq/ton
+rRTrbAkrsj4RicpQKDJCwe4UN+9DlOu6wijRQgbJC/Q7IOoieJxcX7eGxcve2UnZ
+HY7GsD7AYRwa02UquCYJHIjM1enmxZFhMW1AD+UCgYEAm4jdNz5e4QjA4AkNF+cB
+ZenrAZ0q3NbTyiSsJEAtRe/c5fNFpmXo3mqgCannarREQYYDF0+jpSoTUY8XAc4q
+wL7EZNzwxITLqBnnHQbdLdAvYxB43kvWTy+JRK8qY9LAMCCFeDoYwXkWV4Wkx/b0
+TgM7RZnmEjNdeaa4M52o7VY=
+-----END PRIVATE KEY-----
+	`
+	resp, err := CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{
+		"pem_bundle": customBundleWithoutCRLBits,
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data)
+	require.NotEmpty(t, resp.Data["imported_issuers"])
+	require.NotEmpty(t, resp.Data["imported_keys"])
+	require.NotEmpty(t, resp.Data["mapping"])
+
+	// Shouldn't have crl-signing on the newly imported issuer's usage.
+	resp, err = CBRead(b, s, "issuer/default")
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data)
+	require.NotEmpty(t, resp.Data["usage"])
+	require.NotContains(t, resp.Data["usage"], "crl-signing")
+
+	// Modifying to set CRL should fail.
+	resp, err = CBPatch(b, s, "issuer/default", map[string]interface{}{
+		"usage": "issuing-certificates,crl-signing",
+	})
+	require.Error(t, err)
+	require.True(t, resp.IsError())
+
+	// Modifying to set issuing-certificates and ocsp-signing should succeed.
+	resp, err = CBPatch(b, s, "issuer/default", map[string]interface{}{
+		"usage": "issuing-certificates,ocsp-signing",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.NotEmpty(t, resp.Data)
+	require.NotEmpty(t, resp.Data["usage"])
+	require.NotContains(t, resp.Data["usage"], "crl-signing")
+}
+
 var (
 	initTest  sync.Once
 	rsaCAKey  string

builtin/logical/pki/path_fetch_issuers.go

@@ -360,6 +360,16 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da
 			return logical.ErrorResponse(fmt.Sprintf("This issuer was revoked; unable to modify its usage to include certificate signing again. Reissue this certificate (preferably with a new key) and modify that entry instead.")), nil
 		}
 
+		// Ensure we deny adding CRL usage if the bits are missing from the
+		// cert itself.
+		cert, err := issuer.GetCertificate()
+		if err != nil {
+			return nil, fmt.Errorf("unable to parse issuer's certificate: %v", err)
+		}
+		if (cert.KeyUsage&x509.KeyUsageCRLSign) == 0 && newUsage.HasUsage(CRLSigningUsage) {
+			return logical.ErrorResponse(fmt.Sprintf("This issuer's underlying certificate lacks the CRLSign KeyUsage value; unable to set CRLSigningUsage on this issuer as a result.")), nil
+		}
+
 		issuer.Usage = newUsage
 		modified = true
 	}
@@ -561,6 +571,14 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat
 				return logical.ErrorResponse(fmt.Sprintf("This issuer was revoked; unable to modify its usage to include certificate signing again. Reissue this certificate (preferably with a new key) and modify that entry instead.")), nil
 			}
 
+			cert, err := issuer.GetCertificate()
+			if err != nil {
+				return nil, fmt.Errorf("unable to parse issuer's certificate: %v", err)
+			}
+			if (cert.KeyUsage&x509.KeyUsageCRLSign) == 0 && newUsage.HasUsage(CRLSigningUsage) {
+				return logical.ErrorResponse(fmt.Sprintf("This issuer's underlying certificate lacks the CRLSign KeyUsage value; unable to set CRLSigningUsage on this issuer as a result.")), nil
+			}
+
 			issuer.Usage = newUsage
 			modified = true
 		}

builtin/logical/pki/storage.go

@@ -588,8 +588,27 @@ func (sc *storageContext) upgradeIssuerIfRequired(issuer *issuerEntry) *issuerEn
 	}
 
 	if issuer.Version == 0 {
-		// handle our new OCSPSigning usage flag for earlier versions
-		if issuer.Usage.HasUsage(CRLSigningUsage) && !issuer.Usage.HasUsage(OCSPSigningUsage) {
+		// Upgrade at this step requires interrogating the certificate itself;
+		// if this decode fails, it indicates internal problems and the
+		// request will subsequently fail elsewhere. However, decoding this
+		// certificate is mildly expensive, so we only do it in the event of
+		// a Version 0 certificate.
+		cert, err := issuer.GetCertificate()
+		if err != nil {
+			return issuer
+		}
+
+		hadCRL := issuer.Usage.HasUsage(CRLSigningUsage)
+		// Remove CRL signing usage if it exists on the issuer but doesn't
+		// exist in the KU of the x509 certificate.
+		if hadCRL && (cert.KeyUsage&x509.KeyUsageCRLSign) == 0 {
+			issuer.Usage.ToggleUsage(OCSPSigningUsage)
+		}
+
+		// Handle our new OCSPSigning usage flag for earlier versions. If we
+		// had it (prior to removing it in this upgrade), we'll add the OCSP
+		// flag since EKUs don't matter.
+		if hadCRL && !issuer.Usage.HasUsage(OCSPSigningUsage) {
 			issuer.Usage.ToggleUsage(OCSPSigningUsage)
 		}
 	}
@@ -707,6 +726,12 @@ func (sc *storageContext) importIssuer(certValue string, issuerName string) (*is
 	result.Usage.ToggleUsage(AllIssuerUsages)
 	result.Version = latestIssuerVersion
 
+	// If we lack relevant bits for CRL, prohibit it from being set
+	// on the usage side.
+	if (issuerCert.KeyUsage&x509.KeyUsageCRLSign) == 0 && result.Usage.HasUsage(CRLSigningUsage) {
+		result.Usage.ToggleUsage(CRLSigningUsage)
+	}
+
 	// We shouldn't add CSRs or multiple certificates in this
 	countCertificates := strings.Count(result.Certificate, "-BEGIN ")
 	if countCertificates != 1 {

changelog/16865.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Allow import of issuers without CRLSign KeyUsage; prohibit setting crl-signing usage on such issuers
+```

website/content/api-docs/secret/pki.mdx

@@ -1831,6 +1831,10 @@ imported entries present in the same bundle).
    issuers. This means the returned certificate _may_ differ in encoding from
    the one provided on subsequent re-imports of the same issuer or key.
 
+~> Note: This import may fail due to CRL rebuilding issuers or other potential
+   issues; this may impact long-term use of these issuers, but some issuers or
+   keys may still be imported as a result of this process.
+
 #### Parameters
 
 - `pem_bundle` `(string: <required>)` - Specifies the unencrypted private key
@@ -2001,7 +2005,9 @@ do so, import a new issuer and a new `issuer_id` will be assigned.
   - `read-only`, to allow this issuer to be read; implict; always allowed;
   - `issuing-certificates`, to allow this issuer to be used for issuing other
     certificates; or
-  - `crl-signing`, to allow this issuer to be used for signing CRLs.
+  - `crl-signing`, to allow this issuer to be used for signing CRLs. This is
+    separate from the CRLSign KeyUsage on the x509 certificate, but this usage
+    cannot be set unless that KeyUsage is allowed on the x509 certificate.
   - `ocsp-signing`, to allow this issuer to be used for signing OCSP responses
 
 ~> Note: The `usage` field allows for a soft-delete capability on the issuer,