From 6089d2e2479a388458cf4ea4da22594e3a1456f5 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 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,