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>
This commit is contained in:
Alexander Scheel
GitHub
parent
ff7a95d1ac
commit
6089d2e247
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,3 @@
|
|||
```release-note:bug
|
||||
secrets/pki: Allow import of issuers without CRLSign KeyUsage; prohibit setting crl-signing usage on such issuers
|
||||
```
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue