From 95a16dbafeba4e26a6f64cdc9c1de7f2625e970e Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 15 Nov 2024 11:59:54 -0500 Subject: [PATCH] PKI: Add a new leaf_not_after_behavior value to force erroring in all circumstances (#28907) * PKI: Add a new leaf_not_after_behavior value to force erroring in all circumstances - We introduce a new value called `always_enforce_err` for the existing leaf_not_after_behavior on a PKI issuer. The new value will force we error out all requests that have a TTL beyond the issuer's NotAfter value. - This will apply to leaf certificates issued through the API as did err, but now to CA issuance and ACME requests for which we previously changed the err configuration to truncate. * Add cl * Update UI test * Fix changelog type --- builtin/logical/pki/backend_test.go | 88 +++++++++++++++++++ builtin/logical/pki/issuing/issue_common.go | 2 +- builtin/logical/pki/path_acme_order.go | 3 +- builtin/logical/pki/path_acme_test.go | 79 +++++++++++++++++ builtin/logical/pki/path_fetch_issuers.go | 4 + builtin/logical/pki/path_root.go | 6 +- changelog/28907.txt | 3 + sdk/helper/certutil/certutil_test.go | 4 + sdk/helper/certutil/types.go | 2 + ui/app/models/pki/issuer.js | 2 +- .../addon/components/page/pki-issuer-edit.hbs | 9 +- .../pki/page/pki-issuer-edit-test.js | 2 +- website/content/api-docs/secret/pki/index.mdx | 10 ++- 13 files changed, 204 insertions(+), 10 deletions(-) create mode 100644 changelog/28907.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 988890c232..3cdd73833e 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -7243,6 +7243,94 @@ func TestGenerateRootCAWithAIA(t *testing.T) { requireSuccessNonNilResponse(t, resp, err, "expected root generation to succeed") } +// TestIssuance_AlwaysEnforceErr validates that we properly return an error in all request +// types that go beyond the issuer's NotAfter +func TestIssuance_AlwaysEnforceErr(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", + }) + requireSuccessNonNilResponse(t, resp, err, "expected root generation to succeed") + + resp, err = CBPatch(b, s, "issuer/root-ca", map[string]interface{}{ + "leaf_not_after_behavior": "always_enforce_err", + }) + requireSuccessNonNilResponse(t, resp, err, "failed updating root issuer with always_enforce_err") + + resp, err = CBWrite(b, s, "roles/test-role", map[string]interface{}{ + "allow_any_name": true, + "key_type": "ec", + "allowed_serial_numbers": "*", + }) + + expectedErrContains := "cannot satisfy request, as TTL would result in notAfter" + + // Make sure we fail on CA issuance requests now + t.Run("ca-issuance", func(t *testing.T) { + 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"] + + _, err = CBWrite(b, s, "issuer/root-ca/sign-intermediate", map[string]interface{}{ + "csr": csr, + "use_csr_values": true, + "ttl": "60h", + }) + require.ErrorContains(t, err, expectedErrContains, "sign-intermediate should have failed as root issuer leaf behavior is set to always_enforce_err") + + // Make sure it works if we are under + resp, err = CBWrite(b, s, "issuer/root-ca/sign-intermediate", map[string]interface{}{ + "csr": csr, + "use_csr_values": true, + "ttl": "30m", + }) + requireSuccessNonNilResponse(t, resp, err, "sign-intermediate should have passed with a lower TTL value and always_enforce_err") + }) + + // Make sure we fail on leaf csr signing leaf as we always did for 'err' + t.Run("sign-leaf-csr", func(t *testing.T) { + _, csrPem := generateTestCsr(t, certutil.ECPrivateKey, 256) + + resp, err = CBWrite(b, s, "issuer/root-ca/sign/test-role", map[string]interface{}{ + "ttl": "60h", + "csr": csrPem, + }) + require.ErrorContains(t, err, expectedErrContains, "expected error from sign csr got: %v", resp) + + // Make sure it works if we are under + resp, err = CBWrite(b, s, "issuer/root-ca/sign/test-role", map[string]interface{}{ + "ttl": "30m", + "csr": csrPem, + }) + requireSuccessNonNilResponse(t, resp, err, "sign should have succeeded with a lower TTL and always_enforce_err") + }) + + // Make sure we fail on leaf csr signing leaf as we always did for 'err' + t.Run("issue-leaf-csr", func(t *testing.T) { + resp, err = CBWrite(b, s, "issuer/root-ca/issue/test-role", map[string]interface{}{ + "ttl": "60h", + "common_name": "leaf.example.com", + }) + require.ErrorContains(t, err, expectedErrContains, "expected error from issue got: %v", resp) + + // Make sure it works if we are under + resp, err = CBWrite(b, s, "issuer/root-ca/issue/test-role", map[string]interface{}{ + "ttl": "30m", + "common_name": "leaf.example.com", + }) + requireSuccessNonNilResponse(t, resp, err, "issue should have worked with a lower TTL and always_enforce_err") + }) +} + var ( initTest sync.Once rsaCAKey string diff --git a/builtin/logical/pki/issuing/issue_common.go b/builtin/logical/pki/issuing/issue_common.go index b1a67e2feb..f72ba0701c 100644 --- a/builtin/logical/pki/issuing/issue_common.go +++ b/builtin/logical/pki/issuing/issue_common.go @@ -1008,7 +1008,7 @@ func ApplyIssuerLeafNotAfterBehavior(caSign *certutil.CAInfoBundle, notAfter tim // Explicitly do nothing. case certutil.TruncateNotAfterBehavior: notAfter = caSign.Certificate.NotAfter - case certutil.ErrNotAfterBehavior: + case certutil.ErrNotAfterBehavior, certutil.AlwaysEnforceErr: fallthrough default: return time.Time{}, errutil.UserError{Err: fmt.Sprintf( diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index 396dd3e769..ab80ee38eb 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/builtin/logical/pki/path_acme_order.go @@ -529,7 +529,8 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil. } // ACME issued cert will override the TTL values to truncate to the issuer's - // expiration if we go beyond, no matter the setting + // expiration if we go beyond, no matter the setting. + // Note that if set to certutil.AlwaysEnforceErr we will error out if signingBundle.LeafNotAfterBehavior == certutil.ErrNotAfterBehavior { signingBundle.LeafNotAfterBehavior = certutil.TruncateNotAfterBehavior } diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index c1c751f69d..493a601c98 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/builtin/logical/pki/path_acme_test.go @@ -710,6 +710,85 @@ func TestAcmeConfigChecksPublicAcmeEnv(t *testing.T) { require.NoError(t, err) } +// TestAcmeHonorsAlwaysEnforceErr verifies that we get an error and not truncated if the issuer's +// leaf_not_after_behavior is set to always_enforce_err +func TestAcmeHonorsAlwaysEnforceErr(t *testing.T) { + t.Parallel() + + cluster, client, _ := setupAcmeBackend(t) + defer cluster.Cleanup() + + testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + mount := "pki" + resp, err := client.Logical().WriteWithContext(context.Background(), mount+"/issuers/generate/intermediate/internal", + map[string]interface{}{ + "key_name": "short-key", + "key_type": "ec", + "common_name": "test.com", + }) + require.NoError(t, err, "failed creating intermediary CSR") + intermediateCSR := resp.Data["csr"].(string) + + // Sign the intermediate CSR using /pki + resp, err = client.Logical().Write(mount+"/issuer/root-ca/sign-intermediate", map[string]interface{}{ + "csr": intermediateCSR, + "ttl": "10m", + "max_ttl": "1h", + }) + require.NoError(t, err, "failed signing intermediary CSR") + intermediateCertPEM := resp.Data["certificate"].(string) + + // Configure the intermediate cert as the CA in /pki2 + resp, err = client.Logical().Write(mount+"/issuers/import/cert", map[string]interface{}{ + "pem_bundle": intermediateCertPEM, + }) + require.NoError(t, err, "failed importing intermediary cert") + importedIssuersRaw := resp.Data["imported_issuers"].([]interface{}) + require.Len(t, importedIssuersRaw, 1) + shortCaUuid := importedIssuersRaw[0].(string) + + _, err = client.Logical().Write(mount+"/issuer/"+shortCaUuid, map[string]interface{}{ + "leaf_not_after_behavior": "always_enforce_err", + "issuer_name": "short-ca", + }) + require.NoError(t, err, "failed updating issuer name") + + baseAcmeURL := "/v1/pki/issuer/short-ca/acme/" + accountKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err, "failed creating rsa key") + + acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, accountKey) + + // Create new account + t.Logf("Testing register on %s", baseAcmeURL) + acct, err := acmeClient.Register(testCtx, &acme.Account{}, func(tosURL string) bool { return true }) + require.NoError(t, err, "failed registering account") + + // Create an order + t.Logf("Testing Authorize Order on %s", baseAcmeURL) + identifiers := []string{"*.localdomain"} + order, err := acmeClient.AuthorizeOrder(testCtx, []acme.AuthzID{ + {Type: "dns", Value: identifiers[0]}, + }) + require.NoError(t, err, "failed creating order") + + // HACK: Update authorization/challenge to completed as we can't really do it properly in this workflow + // test. + markAuthorizationSuccess(t, client, acmeClient, acct, order) + + // Build a proper CSR, with the correct name and signed with a different key works. + goodCr := &x509.CertificateRequest{DNSNames: []string{identifiers[0]}} + csrKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "failed generated key for CSR") + csr, err := x509.CreateCertificateRequest(rand.Reader, goodCr, csrKey) + require.NoError(t, err, "failed generating csr") + + _, _, err = acmeClient.CreateOrderCert(testCtx, order.FinalizeURL, csr, true) + require.ErrorContains(t, err, "cannot satisfy request, as TTL would result in notAfter", "failed finalizing order") +} + // TestAcmeTruncatesToIssuerExpiry make sure that if the selected issuer's expiry is shorter than the // CSR's selected TTL value in ACME and the issuer's leaf_not_after_behavior setting is set to Err, // we will override the configured behavior and truncate to the issuer's NotAfter diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index 094e4eb55c..fa21443a59 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -544,6 +544,8 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da switch rawLeafBehavior { case "err": newLeafBehavior = certutil.ErrNotAfterBehavior + case "always_enforce_err": + newLeafBehavior = certutil.AlwaysEnforceErr case "truncate": newLeafBehavior = certutil.TruncateNotAfterBehavior case "permit": @@ -816,6 +818,8 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat switch rawLeafBehavior { case "err": newLeafBehavior = certutil.ErrNotAfterBehavior + case "always_enforce_err": + newLeafBehavior = certutil.AlwaysEnforceErr case "truncate": newLeafBehavior = certutil.TruncateNotAfterBehavior case "permit": diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index 625bcb2946..a4ce2d2510 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -383,8 +383,10 @@ func (b *backend) pathIssuerSignIntermediate(ctx context.Context, req *logical.R // 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 + if signingBundle.LeafNotAfterBehavior != certutil.AlwaysEnforceErr { + warnAboutTruncate = true + signingBundle.LeafNotAfterBehavior = certutil.TruncateNotAfterBehavior + } } useCSRValues := data.Get("use_csr_values").(bool) diff --git a/changelog/28907.txt b/changelog/28907.txt new file mode 100644 index 0000000000..9d301b850e --- /dev/null +++ b/changelog/28907.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secret/pki: Introduce a new value `always_enforce_err` within `leaf_not_after_behavior` to force the error in all circumstances such as CA issuance and ACME requests if requested TTL values are beyond the issuer's NotAfter. +``` diff --git a/sdk/helper/certutil/certutil_test.go b/sdk/helper/certutil/certutil_test.go index 4ebab01827..c872454f15 100644 --- a/sdk/helper/certutil/certutil_test.go +++ b/sdk/helper/certutil/certutil_test.go @@ -916,6 +916,10 @@ func TestNotAfterValues(t *testing.T) { if PermitNotAfterBehavior != 2 { t.Fatalf("Expected PermitNotAfterBehavior=%v to have value 2", PermitNotAfterBehavior) } + + if AlwaysEnforceErr != 3 { + t.Fatalf("Expected AlwaysEnforceErr=%v to have value 3", AlwaysEnforceErr) + } } func TestSignatureAlgorithmRoundTripping(t *testing.T) { diff --git a/sdk/helper/certutil/types.go b/sdk/helper/certutil/types.go index cdfc912e92..4f16659c1d 100644 --- a/sdk/helper/certutil/types.go +++ b/sdk/helper/certutil/types.go @@ -708,9 +708,11 @@ const ( ErrNotAfterBehavior NotAfterBehavior = iota TruncateNotAfterBehavior PermitNotAfterBehavior + AlwaysEnforceErr ) var notAfterBehaviorNames = map[NotAfterBehavior]string{ + AlwaysEnforceErr: "always_enforce_err", ErrNotAfterBehavior: "err", TruncateNotAfterBehavior: "truncate", PermitNotAfterBehavior: "permit", diff --git a/ui/app/models/pki/issuer.js b/ui/app/models/pki/issuer.js index dc7ebf46ec..28526c1991 100644 --- a/ui/app/models/pki/issuer.js +++ b/ui/app/models/pki/issuer.js @@ -63,7 +63,7 @@ export default class PkiIssuerModel extends Model { 'What happens when a leaf certificate is issued, but its NotAfter field (and therefore its expiry date) exceeds that of this issuer.', docLink: '/vault/api-docs/secret/pki#update-issuer', editType: 'yield', - valueOptions: ['err', 'truncate', 'permit'], + valueOptions: ['always_enforce_err', 'err', 'truncate', 'permit'], }) leafNotAfterBehavior; diff --git a/ui/lib/pki/addon/components/page/pki-issuer-edit.hbs b/ui/lib/pki/addon/components/page/pki-issuer-edit.hbs index c9c45034a2..326957a7ef 100644 --- a/ui/lib/pki/addon/components/page/pki-issuer-edit.hbs +++ b/ui/lib/pki/addon/components/page/pki-issuer-edit.hbs @@ -39,8 +39,13 @@ > {{#each field.options.valueOptions as |value|}} {{/each}} diff --git a/ui/tests/integration/components/pki/page/pki-issuer-edit-test.js b/ui/tests/integration/components/pki/page/pki-issuer-edit-test.js index 0c6d61df14..cf7ef00dcc 100644 --- a/ui/tests/integration/components/pki/page/pki-issuer-edit-test.js +++ b/ui/tests/integration/components/pki/page/pki-issuer-edit-test.js @@ -79,7 +79,7 @@ module('Integration | Component | pki | Page::PkiIssuerEditPage::PkiIssuerEdit', assert .dom(selectors.leafOption) .hasText( - 'Error if the computed NotAfter exceeds that of this issuer', + 'Error if the computed NotAfter exceeds that of this issuer in all circumstances (leaf, CA issuance and ACME)', 'Correct text renders for leaf option' ); assert.dom(selectors.usageCert).isChecked('Usage issuing certificates is checked'); diff --git a/website/content/api-docs/secret/pki/index.mdx b/website/content/api-docs/secret/pki/index.mdx index e87361c9a3..198c4eda0a 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -1089,7 +1089,9 @@ have access.** 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` + behavior to the issued CA certificate, instead defer to the issuer's configured `leaf_not_after_behavior`. + If an issuer's `leaf_not_after_behavior` is set to `always_enforce_err`, this flag is not required if + the desired behavior is to error out on requests who's TTL extends beyond the issuer's NotAfter. - `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 @@ -2611,8 +2613,12 @@ do so, import a new issuer and a new `issuer_id` will be assigned. - `leaf_not_after_behavior` `(string: "err")` - Behavior of a leaf's `NotAfter` field during issuance. Valid options are: - + - `always_enforce_err` overrides all hardcoded behaviors to enforce an + error if any requested TTL is beyond the issuer. This applies to CA issuance, + and ACME issuance, along with the normal err on leaf certificates through Vault's API. (Available from 1.18.2+) - `err`, to error if the computed `NotAfter` exceeds that of this issuer; + - **Note** for CA issuance and ACME issuance this behavior is overridden + with truncate behavior, use `always_enforce_err` to disable these overrides - `truncate` to silently truncate the requested `NotAfter` value to that of this issuer; or - `permit` to allow this issuance to succeed with a `NotAfter` value