From 2a14b1c61643bcbd3aba21da2909f551b70e4cb0 Mon Sep 17 00:00:00 2001 From: Kit Haines Date: Thu, 3 Apr 2025 14:48:54 -0400 Subject: [PATCH] Allow Root + Intermediate Key_Usage to be set (#30034) * outline of key usage fix * Changelog, and test-fix * Simplify code setting key_usage * make fmt * Per internal dicussion to align closer to the CAB guidelines, only allow DigitalSignature. * Breaking Change: error if invalid key_usage to generate root or sign-intermediate. * Change error to warning in order to not break backwards compatibility. --- builtin/logical/pki/backend_test.go | 47 +++++++++++++++++++ builtin/logical/pki/path_root.go | 32 +++++++++++++ changelog/30034.txt | 4 ++ sdk/helper/certutil/types.go | 9 +++- website/content/api-docs/secret/pki/index.mdx | 9 ++-- 5 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 changelog/30034.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index eb2c9f93da..a9f7686897 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -7420,6 +7420,53 @@ func TestIssuance_AlwaysEnforceErr(t *testing.T) { }) } +// TestIssuance_SignIntermediateKeyUsages tests the field "key_usage" both when directly generating a root certificate, +// and when signing a CA CSR. In particular, this test verifies that: +// - the key usage DigitalSignature is added if present +// - non-existent or invalid key usages return a warning, but are ignored by certificate generation +// - CertSign and CRLSign are ignored +func TestIssuance_SignIntermediateKeyUsages(t *testing.T) { + t.Parallel() + b, s := CreateBackendWithStorage(t) + + resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "common_name": "root myvault.com", + "key_type": "ec", + "ttl": "10h", + "issuer_name": "root-ca", + "key_name": "root-key", + "key_usage": "DigitalSignature,DogPetting", + }) + requireSuccessNonNilResponse(t, resp, err, "expected root generation to succeed") + require.Contains(t, resp.Warnings, "Invalid key usage will be ignored: unrecognized key usage DogPetting") + rootCertRaw := resp.Data["certificate"] + rootCert := parseCert(t, rootCertRaw.(string)) + require.Equal(t, x509.KeyUsageDigitalSignature, rootCert.KeyUsage&x509.KeyUsageDigitalSignature, "keyUsage Digital Signature was not present") + require.Equal(t, x509.KeyUsageCertSign, rootCert.KeyUsage&x509.KeyUsageCertSign, "keyUsage CertSign was not present") + require.Equal(t, x509.KeyUsageCRLSign, rootCert.KeyUsage&x509.KeyUsageCRLSign, "keyUsage CRLSign was not present") + require.Equal(t, x509.KeyUsageDigitalSignature|x509.KeyUsageCertSign|x509.KeyUsageCRLSign, rootCert.KeyUsage, "unexpected KeyUsage present") + resp, err = CBWrite(b, s, "intermediate/generate/internal", map[string]interface{}{ + "common_name": "myint.com", + }) + requireSuccessNonNilResponse(t, resp, err, "failed generating intermediary CSR") + requireFieldsSetInResp(t, resp, "csr") + csr := resp.Data["csr"] + + resp, err = CBWrite(b, s, "issuer/root-ca/sign-intermediate", map[string]interface{}{ + "csr": csr, + "key_usage": "CRLSign,CertSign,DigitalSignature,KeyEncipherment,KeyAgreement", + "ttl": "60h", + }) + requireSuccessNonNilResponse(t, resp, err, "expected intermediate signing to succeed") + require.Contains(t, resp.Warnings, "Invalid key usage: key usage KeyEncipherment is only valid for non-Ca certs; key usage KeyAgreement is only valid for non-Ca certs") + intCertRaw := resp.Data["certificate"] + intCert := parseCert(t, intCertRaw.(string)) + require.Equal(t, x509.KeyUsageDigitalSignature, intCert.KeyUsage&x509.KeyUsageDigitalSignature, "keyUsage Digital Signature was not present") + require.Equal(t, x509.KeyUsageCertSign, intCert.KeyUsage&x509.KeyUsageCertSign, "keyUsage CertSign was not present") + require.Equal(t, x509.KeyUsageCRLSign, intCert.KeyUsage&x509.KeyUsageCRLSign, "keyUsage CRLSign was not present") + require.Equal(t, x509.KeyUsageDigitalSignature|x509.KeyUsageCertSign|x509.KeyUsageCRLSign, intCert.KeyUsage, "unexpected KeyUsage present on intermediate certificate") +} + var ( initTest sync.Once rsaCAKey string diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index d5f9b90533..a705ee5f38 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -197,6 +197,13 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request, }, } + if keyUsages, ok := data.GetOk("key_usage"); ok { + err = validateCaKeyUsages(keyUsages.([]string)) + if err != nil { + resp.AddWarning(fmt.Sprintf("Invalid key usage will be ignored: %v", err.Error())) + } + } + if len(parsedBundle.Certificate.RawSubject) <= 2 { // Strictly a subject is a SEQUENCE of SETs of SEQUENCES. // @@ -432,6 +439,13 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R resp.AddWarning(intCaTruncatationWarning) } + if keyUsages, ok := data.GetOk("key_usage"); ok { + err = validateCaKeyUsages(keyUsages.([]string)) + if err != nil { + resp.AddWarning(fmt.Sprintf("Invalid key usage: %v", err.Error())) + } + } + return resp, nil } @@ -631,6 +645,24 @@ func publicKeyType(pub crypto.PublicKey) (pubType x509.PublicKeyAlgorithm, sigAl return } +func validateCaKeyUsages(keyUsages []string) error { + invalidKeyUsages := []string{} + for _, usage := range keyUsages { + cleanUsage := strings.ToLower(strings.TrimSpace(usage)) + switch cleanUsage { + case "crlsign", "certsign", "digitalsignature": + case "contentcommitment", "keyencipherment", "dataencipherment", "keyagreement", "encipheronly", "decipheronly": + invalidKeyUsages = append(invalidKeyUsages, fmt.Sprintf("key usage %s is only valid for non-Ca certs", usage)) + default: + invalidKeyUsages = append(invalidKeyUsages, fmt.Sprintf("unrecognized key usage %s", usage)) + } + } + if invalidKeyUsages != nil { + return fmt.Errorf(strings.Join(invalidKeyUsages, "; ")) + } + return nil +} + const pathGenerateRootHelpSyn = ` Generate a new CA certificate and private key used for signing. ` diff --git a/changelog/30034.txt b/changelog/30034.txt new file mode 100644 index 0000000000..e24ed26ea4 --- /dev/null +++ b/changelog/30034.txt @@ -0,0 +1,4 @@ +```release-note:bug +secrets/pki: fix a bug where key_usage was ignored when generating root certificates, and signing certain +intermediate certificates. +``` \ No newline at end of file diff --git a/sdk/helper/certutil/types.go b/sdk/helper/certutil/types.go index e9834b3586..5ad866ed2f 100644 --- a/sdk/helper/certutil/types.go +++ b/sdk/helper/certutil/types.go @@ -821,7 +821,14 @@ type CreationBundle struct { // information func AddKeyUsages(data *CreationBundle, certTemplate *x509.Certificate) { if data.Params.IsCA { - certTemplate.KeyUsage = x509.KeyUsage(x509.KeyUsageCertSign | x509.KeyUsageCRLSign) + // https://cabforum.org/working-groups/server/baseline-requirements/documents/CA-Browser-Forum-TLS-BR-2.1.4.pdf + // Per Section 7.1.2.10.7, the only acceptable additional key usage is Digital Signature + if data.Params.KeyUsage&x509.KeyUsageDigitalSignature == x509.KeyUsageDigitalSignature { + certTemplate.KeyUsage = x509.KeyUsageDigitalSignature + } else { + certTemplate.KeyUsage = x509.KeyUsage(0) + } + certTemplate.KeyUsage |= x509.KeyUsageCertSign | x509.KeyUsageCRLSign return } diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index ad49b763d8..ed2f543719 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -751,9 +751,9 @@ intermediary goes beyond the prescribed length. - `key_usage` `([]string: CRL,CertSign)` - This list of key usages will be added to the existing set of key usages, CRL,CertSign, on the generated certificate. - Values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage part of the - name. This is overwritten by use_csr_values if a key usage extension exists - within the csr. + Per the CAB Forum requirements, Vault ignores values other than `DigitalSignature`. + To get a certificate with additional `key_usage` values, add the desired values to + the CSR, then call this function with `use_csr_values` set to `true`. - `exclude_cn_from_sans` `(bool: false)` - If true, the given `common_name` will not be included in DNS or Email Subject Alternate Names (as appropriate). @@ -2166,8 +2166,7 @@ use the values set via `config/urls`. - `key_usage` `([]string: CRL,CertSign)` - This list of key usages will be added to the existing set of key usages, CRL,CertSign, on the generated certificate. - Values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage part of the - name. + Per the CAB Forum, Vault ignores additional values other than `DigitalSignature`. - `exclude_cn_from_sans` `(bool: false)` - If true, the given `common_name` will not be included in DNS or Email Subject Alternate Names (as appropriate).