From 7c91a0f17bceea8f7e944a7712429902c37166e3 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Thu, 2 May 2024 15:44:45 -0400 Subject: [PATCH 1/4] PKI: Change sign-intermediate to truncate notAfter by default - The PKI sign-intermediate API allowed an end-user to request a TTL value that would extend beyond the signing issuer's notAfter. This would generate an invalid CA chain when properly validated. - We are now changing the default behavior to truncate the returned certificate to the signing issuer's notAfter. - End-users can get the old behavior by configuring the signing issuer's leaf_not_after_behavior field to permit, and call sign-intermediary with the new argument enforce_leaf_not_after_behavior to true. The new argument could also be used to enforce an error instead of truncating behavior if the signing issuer's leaf_not_after_behavior is set to err. --- builtin/logical/pki/backend_test.go | 62 +++++++++++++++++++++++- builtin/logical/pki/path_root.go | 19 ++++++-- builtin/logical/pki/path_sign_issuers.go | 5 ++ website/content/api-docs/secret/pki.mdx | 3 ++ 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 05a3dc85e9d2..c46ce0c72eea 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2539,6 +2539,59 @@ func TestBackend_Root_Idempotency(t *testing.T) { } } +// TestBackend_SignIntermediate_EnforceLeafFlag verifies if the flag is true +// that we will leverage the issuer's configured behavior +func TestBackend_SignIntermediate_EnforceLeafFlag(t *testing.T) { + t.Parallel() + b, s := CreateBackendWithStorage(t) + + resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "ttl": "40h", + "common_name": "myvault.com", + }) + require.NoError(t, err, "failed generating root cert") + rootCert := parseCert(t, resp.Data["certificate"].(string)) + + _, err = CBWrite(b, s, "issuer/default", map[string]interface{}{ + "leaf_not_after_behavior": "err", + }) + require.NoError(t, err, "failed updating root issuer cert behavior") + + resp, err = CBWrite(b, s, "intermediate/generate/internal", map[string]interface{}{ + "common_name": "myint.com", + }) + require.NoError(t, err, "failed generating intermediary CSR") + csr := resp.Data["csr"] + + _, err = CBWrite(b, s, "root/sign-intermediate", map[string]interface{}{ + "common_name": "myint.com", + "other_sans": "1.3.6.1.4.1.311.20.2.3;utf8:caadmin@example.com", + "csr": csr, + "ttl": "60h", + "enforce_leaf_not_after_behavior": true, + }) + require.Error(t, err, "sign-intermediate should have failed as root issuer leaf behavior is set to err") + + // Now test with permit, the old default behavior + _, err = CBWrite(b, s, "issuer/default", map[string]interface{}{ + "leaf_not_after_behavior": "permit", + }) + require.NoError(t, err, "failed updating root issuer cert behavior to permit") + + resp, err = CBWrite(b, s, "root/sign-intermediate", map[string]interface{}{ + "common_name": "myint.com", + "other_sans": "1.3.6.1.4.1.311.20.2.3;utf8:caadmin@example.com", + "csr": csr, + "ttl": "60h", + "enforce_leaf_not_after_behavior": true, + }) + require.NoError(t, err, "failed to sign intermediary CA with permit as issuer") + intCert := parseCert(t, resp.Data["certificate"].(string)) + + require.Truef(t, rootCert.NotAfter.Before(intCert.NotAfter), + "root cert notAfter %v was not before ca cert's notAfter %v", rootCert.NotAfter, intCert.NotAfter) +} + func TestBackend_SignIntermediate_AllowedPastCAValidity(t *testing.T) { t.Parallel() b_root, s_root := CreateBackendWithStorage(t) @@ -2546,7 +2599,7 @@ func TestBackend_SignIntermediate_AllowedPastCAValidity(t *testing.T) { var err error // Direct issuing from root - _, err = CBWrite(b_root, s_root, "root/generate/internal", map[string]interface{}{ + resp, err := CBWrite(b_root, s_root, "root/generate/internal", map[string]interface{}{ "ttl": "40h", "common_name": "myvault.com", }) @@ -2554,6 +2607,8 @@ func TestBackend_SignIntermediate_AllowedPastCAValidity(t *testing.T) { t.Fatal(err) } + rootCert := parseCert(t, resp.Data["certificate"].(string)) + _, err = CBWrite(b_root, s_root, "roles/test", map[string]interface{}{ "allow_bare_domains": true, "allow_subdomains": true, @@ -2563,7 +2618,7 @@ func TestBackend_SignIntermediate_AllowedPastCAValidity(t *testing.T) { t.Fatal(err) } - resp, err := CBWrite(b_int, s_int, "intermediate/generate/internal", map[string]interface{}{ + resp, err = CBWrite(b_int, s_int, "intermediate/generate/internal", map[string]interface{}{ "common_name": "myint.com", }) schema.ValidateResponse(t, schema.GetResponseSchema(t, b_root.Route("intermediate/generate/internal"), logical.UpdateOperation), resp, true) @@ -2614,6 +2669,9 @@ func TestBackend_SignIntermediate_AllowedPastCAValidity(t *testing.T) { cert := parseCert(t, resp.Data["certificate"].(string)) certSkid := certutil.GetHexFormatted(cert.SubjectKeyId, ":") require.Equal(t, intSkid, certSkid) + + require.Equal(t, rootCert.NotAfter, cert.NotAfter, "intermediary cert's NotAfter did not match root cert's NotAfter") + require.Contains(t, resp.Warnings, intCaTruncatationWarning, "missing warning about intermediary CA notAfter truncation") } func TestBackend_ConsulSignLeafWithLegacyRole(t *testing.T) { diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index 07f3b5b06271..53b8c81389ad 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -29,6 +29,8 @@ import ( "golang.org/x/crypto/ed25519" ) +const intCaTruncatationWarning = "the signed intermediary CA certificate's notAfter was truncated to the issuer's notAfter" + func pathGenerateRoot(b *backend) *framework.Path { pattern := "root/generate/" + framework.GenericNameRegex("exported") @@ -375,10 +377,14 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R } } - // Since we are signing an intermediate, we explicitly want to override - // the leaf NotAfterBehavior to permit issuing intermediates longer than - // the life of this issuer. - signingBundle.LeafNotAfterBehavior = certutil.PermitNotAfterBehavior + warnAboutTruncate := false + if enforceLeafNotAfter := data.Get("enforce_leaf_not_after_behavior").(bool); !enforceLeafNotAfter { + // Since we are signing an intermediate, we will by default truncate the + // signed intermediary in order to generate a valid intermediary chain. This + // was changed in 1.17.x as the default prior was PermitNotAfterBehavior + warnAboutTruncate = true + signingBundle.LeafNotAfterBehavior = certutil.TruncateNotAfterBehavior + } useCSRValues := data.Get("use_csr_values").(bool) @@ -418,6 +424,11 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R return nil, err } + if warnAboutTruncate && + signingBundle.Certificate.NotAfter.Equal(parsedBundle.Certificate.NotAfter) { + resp.AddWarning(intCaTruncatationWarning) + } + return resp, nil } diff --git a/builtin/logical/pki/path_sign_issuers.go b/builtin/logical/pki/path_sign_issuers.go index e9bdc5e8a301..d139e968ec95 100644 --- a/builtin/logical/pki/path_sign_issuers.go +++ b/builtin/logical/pki/path_sign_issuers.go @@ -36,6 +36,11 @@ func pathSignIntermediate(b *backend) *framework.Path { func buildPathIssuerSignIntermediateRaw(b *backend, pattern string, displayAttrs *framework.DisplayAttributes) *framework.Path { fields := addIssuerRefField(map[string]*framework.FieldSchema{}) + fields["enforce_leaf_not_after_behavior"] = &framework.FieldSchema{ + Type: framework.TypeBool, + Default: false, + Description: "Do not truncate the NotAfter field, use the issuer's configured leaf_not_after_behavior", + } path := &framework.Path{ Pattern: pattern, DisplayAttrs: displayAttrs, diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index c199b4f40bca..bdd282c04e8e 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -1411,6 +1411,9 @@ have access.** ~> Note: This value is only used as a default when the `ExtendedKeyUsage` extension is missing from the CSR. +- `enforce_leaf_not_after_behavior` `(bool: false)` - If true, do not apply the default truncate + behavior to the issued CA certificate, instead defer to the issuer's configured `leaf_not_after_behavior` + - `ttl` `(string: "")` - Specifies the requested Time To Live. Cannot be greater than the engine's `max_ttl` value. If not provided, the engine's `ttl` value will be used, which defaults to system values if not explicitly set. See From 2b7aa0b657b55acb105306c40c358c514a36d6e5 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Thu, 2 May 2024 16:21:55 -0400 Subject: [PATCH 2/4] Add cl --- changelog/26796.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/26796.txt diff --git a/changelog/26796.txt b/changelog/26796.txt new file mode 100644 index 000000000000..1c9809a8c899 --- /dev/null +++ b/changelog/26796.txt @@ -0,0 +1,3 @@ +```release-note:change +secrets/pki: sign-intermediate API will truncate notAfter if calculated to go beyond the signing issuer's notAfter. Previously the notAfter was permitted to go beyond leading to invalid chains. +``` From a8de6f3dd0aa2cc88f67c19c0c6c43de11099916 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Thu, 2 May 2024 16:44:43 -0400 Subject: [PATCH 3/4] Add cl and upgrade note --- website/content/api-docs/secret/pki.mdx | 5 +++ .../docs/upgrading/upgrade-to-1.17.x.mdx | 33 +++++++++++++++++++ website/data/docs-nav-data.json | 4 +++ 3 files changed, 42 insertions(+) create mode 100644 website/content/docs/upgrading/upgrade-to-1.17.x.mdx diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index bdd282c04e8e..1b1dc6748a8f 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -1023,6 +1023,11 @@ when signing an externally-owned intermediate. Access to this endpoint should be restricted by policy to only trusted operators. +~> **Note**: As of 1.17.x the default behavior has changed to truncate the + signed intermediary's notAfter field to the signing issuer's notAfter + if it is computed to go beyond. Prior versions permitted the notAfter + to go beyond. + | Method | Path | Issuer | | :----- | :------------------------------------------ | :------- | | `POST` | `/pki/root/sign-intermediate` | `default` | diff --git a/website/content/docs/upgrading/upgrade-to-1.17.x.mdx b/website/content/docs/upgrading/upgrade-to-1.17.x.mdx new file mode 100644 index 000000000000..e282aca683d6 --- /dev/null +++ b/website/content/docs/upgrading/upgrade-to-1.17.x.mdx @@ -0,0 +1,33 @@ +--- +layout: docs +page_title: Upgrade to Vault 1.17.x - Guides +description: |- + Deprecations, important or breaking changes, and remediation recommendations + for anyone upgrading to 1.17.x from Vault 1.16.x. +--- + +# Overview + +The Vault 1.17.x upgrade guide contains information on deprecations, important +or breaking changes, and remediation recommendations for anyone upgrading from +Vault 1.16. **Please read carefully**. + +## Important changes + +### PKI sign-intermediate now truncates notAfter field to signing issuer + +Prior to 1.17.x the sign-intermediate API would permit a calculated notAfter field to go beyond +the signing issuer's notAfter. This would lead to a CA chain that would not validate properly. As +of 1.17.x the default behavior has changed to truncate the intermediary's notAfter value to +the signing issuer's notAfter if calculated to be greater. + +#### How to opt out + +A new flag has been introduced on the sign-intermediate API `enforce_leaf_not_after_behavior`. Setting +this flag to true, the sign-intermediate API will use the signing issuer's configured +`leaf_not_after_behavior` value to control the behavior. Configuring the issuer to a value of `permit` +will along with setting the `enforce_leaf_not_after_behavior` to true will restore the legacy behavior. + +## Known issues and workarounds + +@include 'known-issues/ocsp-redirect.mdx' diff --git a/website/data/docs-nav-data.json b/website/data/docs-nav-data.json index 9483aad85ef4..50f954fddb2f 100644 --- a/website/data/docs-nav-data.json +++ b/website/data/docs-nav-data.json @@ -2282,6 +2282,10 @@ "title": "Upgrade to Raft WAL", "path": "upgrading/raft-wal" }, + { + "title": "Upgrade to 1.17.x", + "path": "upgrading/upgrade-to-1.17.x" + }, { "title": "Upgrade to 1.16.x", "path": "upgrading/upgrade-to-1.16.x" From f5c8f243ec570758c0857dc594162c5f0322b244 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 8 May 2024 10:17:06 -0400 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com> --- website/content/api-docs/secret/pki.mdx | 7 +++--- .../docs/upgrading/upgrade-to-1.17.x.mdx | 22 ++++++++++++------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 1b1dc6748a8f..cfdf10620f15 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -1023,10 +1023,9 @@ when signing an externally-owned intermediate. Access to this endpoint should be restricted by policy to only trusted operators. -~> **Note**: As of 1.17.x the default behavior has changed to truncate the - signed intermediary's notAfter field to the signing issuer's notAfter - if it is computed to go beyond. Prior versions permitted the notAfter - to go beyond. +By default, Vault truncates the `notAfter` field of the signed intermediary +to use the `notAfter` field of the signing issuer, it the computed field for the +intermediary goes beyond the prescribed length. | Method | Path | Issuer | | :----- | :------------------------------------------ | :------- | diff --git a/website/content/docs/upgrading/upgrade-to-1.17.x.mdx b/website/content/docs/upgrading/upgrade-to-1.17.x.mdx index e282aca683d6..ff1d566607c7 100644 --- a/website/content/docs/upgrading/upgrade-to-1.17.x.mdx +++ b/website/content/docs/upgrading/upgrade-to-1.17.x.mdx @@ -16,17 +16,23 @@ Vault 1.16. **Please read carefully**. ### PKI sign-intermediate now truncates notAfter field to signing issuer -Prior to 1.17.x the sign-intermediate API would permit a calculated notAfter field to go beyond -the signing issuer's notAfter. This would lead to a CA chain that would not validate properly. As -of 1.17.x the default behavior has changed to truncate the intermediary's notAfter value to -the signing issuer's notAfter if calculated to be greater. +Prior to 1.17.x, Vault allowed the calculated sign-intermediate `notAfter` field +to go beyond the signing issuer `notAfter` field. The extended value lead to a +CA chain that would not validate properly. As of 1.17.x, Vault truncates the +intermediary `notAfter` value to the signing issuer `notAfter` if the calculated +field is greater. #### How to opt out -A new flag has been introduced on the sign-intermediate API `enforce_leaf_not_after_behavior`. Setting -this flag to true, the sign-intermediate API will use the signing issuer's configured -`leaf_not_after_behavior` value to control the behavior. Configuring the issuer to a value of `permit` -will along with setting the `enforce_leaf_not_after_behavior` to true will restore the legacy behavior. +You can use the new `enforce_leaf_not_after_behavior` flag on the +sign-intermediate API along with the `leaf_not_after_behavior` flag for the +signing issuer to opt out of the truncating behavior. + +When you set `enforce_leaf_not_after_behavior` to true, the sign-intermediate +API uses the `leaf_not_after_behavior` value configured for the signing issuer +to control truncation the behavior. Setting the issuer `leaf_not_after_behavior` +field to `permit` and `enforce_leaf_not_after_behavior` to true restores the +legacy behavior. ## Known issues and workarounds