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

@@ -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

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(

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
 	}

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

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":

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)

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.
+```

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) {

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",

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;
 

ui/lib/pki/addon/components/page/pki-issuer-edit.hbs

@@ -39,8 +39,13 @@
               >
                 {{#each field.options.valueOptions as |value|}}
                   <option selected={{eq @model.leafNotAfterBehavior value}} value={{value}}>
-                    {{capitalize (if (eq value "err") "error" value)}}
-                    if the computed NotAfter exceeds that of this issuer
+                    {{#if (eq value "always_enforce_err")}}
+                      Error if the computed NotAfter exceeds that of this issuer in all circumstances (leaf, CA issuance and
+                      ACME)
+                    {{else}}
+                      {{capitalize (if (eq value "err") "error" value)}}
+                      if the computed NotAfter exceeds that of this issuer
+                    {{/if}}
                   </option>
                 {{/each}}
               </select>

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');

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