diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 4b8b2db4e..95ed3574f 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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 diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 2e7a818fb..3bec09513 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/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 } diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index 63c26cf16..e30ca5283 100644 --- a/builtin/logical/pki/storage.go +++ b/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 { diff --git a/changelog/16865.txt b/changelog/16865.txt new file mode 100644 index 000000000..2f03b83de --- /dev/null +++ b/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 +``` diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 746837b93..c789071a8 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/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: )` - 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,