From 7f90f83d3d808d65368afa8e8857967354674e88 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 24 Aug 2022 10:45:54 -0400 Subject: [PATCH] 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 * Add docs to clarify issue with import, CRL usage Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel * Update website/content/api-docs/secret/pki.mdx * Add additional test assertion Signed-off-by: Alexander Scheel Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 90 +++++++++++++++++++++++ builtin/logical/pki/path_fetch_issuers.go | 18 +++++ builtin/logical/pki/storage.go | 29 +++++++- changelog/16865.txt | 3 + website/content/api-docs/secret/pki.mdx | 8 +- 5 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 changelog/16865.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 4b8b2db4eb..95ed3574fc 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 2e7a818fb3..3bec095138 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 63c26cf163..e30ca5283a 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 0000000000..2f03b83de4 --- /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 746837b930..c789071a89 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,