diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index af079b13f781..ec179dbb84ef 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -7224,6 +7224,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 b1a67e2feb33..f72ba0701c84 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 396dd3e76933..ab80ee38eb28 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 c1c751f69d8e..493a601c985e 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 b48ef47d3f36..4d82f38b4bd8 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/builtin/logical/pki/path_fetch_issuers.go @@ -529,6 +529,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": @@ -797,6 +799,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 625bcb2946e3..a4ce2d25108b 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 000000000000..9d301b850e68 --- /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 4ebab01827d2..c872454f155d 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 cdfc912e9289..4f16659c1dd6 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 dc7ebf46ecdd..28526c1991f7 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 c9c45034a272..326957a7ef1c 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 0c6d61df14e7..cf7ef00dcc3b 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 5bf656e40784..76ca0bec63fd 100644 --- a/website/content/api-docs/secret/pki/index.mdx +++ b/website/content/api-docs/secret/pki/index.mdx @@ -1086,7 +1086,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 @@ -2608,8 +2610,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