backport of commit ddaf5038f253546b15eae684a8114c04e552b731 (#21068)

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

builtin/logical/pki/cert_util.go

@@ -1379,71 +1379,11 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 	}
 
 	// Get the TTL and verify it against the max allowed
-	var ttl time.Duration
+	notAfter, ttlWarnings, err := getCertificateNotAfter(b, data, caSign)
-	var maxTTL time.Duration
-	var notAfter time.Time
-	var err error
-	{
-		ttl = time.Duration(data.apiData.Get("ttl").(int)) * time.Second
-		notAfterAlt := data.role.NotAfter
-		if notAfterAlt == "" {
-			notAfterAltRaw, ok := data.apiData.GetOk("not_after")
-			if ok {
-				notAfterAlt = notAfterAltRaw.(string)
-			}
-
-		}
-		if ttl > 0 && notAfterAlt != "" {
-			return nil, nil, errutil.UserError{
-				Err: "Either ttl or not_after should be provided. Both should not be provided in the same request.",
-			}
-		}
-
-		if ttl == 0 && data.role.TTL > 0 {
-			ttl = data.role.TTL
-		}
-
-		if data.role.MaxTTL > 0 {
-			maxTTL = data.role.MaxTTL
-		}
-
-		if ttl == 0 {
-			ttl = b.System().DefaultLeaseTTL()
-		}
-		if maxTTL == 0 {
-			maxTTL = b.System().MaxLeaseTTL()
-		}
-		if ttl > maxTTL {
-			warnings = append(warnings, fmt.Sprintf("TTL %q is longer than permitted maxTTL %q, so maxTTL is being used", ttl, maxTTL))
-			ttl = maxTTL
-		}
-
-		if notAfterAlt != "" {
-			notAfter, err = time.Parse(time.RFC3339, notAfterAlt)
 	if err != nil {
-				return nil, nil, errutil.UserError{Err: err.Error()}
+		return nil, warnings, err
-			}
-		} else {
-			notAfter = time.Now().Add(ttl)
-		}
-		if caSign != nil && notAfter.After(caSign.Certificate.NotAfter) {
-			// If it's not self-signed, verify that the issued certificate
-			// won't be valid past the lifetime of the CA certificate, and
-			// act accordingly. This is dependent based on the issuer's
-			// LeafNotAfterBehavior argument.
-			switch caSign.LeafNotAfterBehavior {
-			case certutil.PermitNotAfterBehavior:
-				// Explicitly do nothing.
-			case certutil.TruncateNotAfterBehavior:
-				notAfter = caSign.Certificate.NotAfter
-			case certutil.ErrNotAfterBehavior:
-				fallthrough
-			default:
-				return nil, nil, errutil.UserError{Err: fmt.Sprintf(
-					"cannot satisfy request, as TTL would result in notAfter of %s that is beyond the expiration of the CA certificate at %s", notAfter.UTC().Format(time.RFC3339Nano), caSign.Certificate.NotAfter.UTC().Format(time.RFC3339Nano))}
-			}
-		}
 	}
+	warnings = append(warnings, ttlWarnings...)
 
 	// Parse SKID from the request for cross-signing.
 	var skid []byte
@@ -1559,6 +1499,73 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 	return creation, warnings, nil
 }
 
+// getCertificateNotAfter compute a certificate's NotAfter date based on the mount ttl, role, signing bundle and input
+// api data being sent. Returns a NotAfter time, a set of warnings or an error.
+func getCertificateNotAfter(b *backend, data *inputBundle, caSign *certutil.CAInfoBundle) (time.Time, []string, error) {
+	var warnings []string
+	var maxTTL time.Duration
+	var notAfter time.Time
+	var err error
+
+	ttl := time.Duration(data.apiData.Get("ttl").(int)) * time.Second
+	notAfterAlt := data.role.NotAfter
+	if notAfterAlt == "" {
+		notAfterAltRaw, ok := data.apiData.GetOk("not_after")
+		if ok {
+			notAfterAlt = notAfterAltRaw.(string)
+		}
+	}
+	if ttl > 0 && notAfterAlt != "" {
+		return time.Time{}, warnings, errutil.UserError{Err: "Either ttl or not_after should be provided. Both should not be provided in the same request."}
+	}
+
+	if ttl == 0 && data.role.TTL > 0 {
+		ttl = data.role.TTL
+	}
+
+	if data.role.MaxTTL > 0 {
+		maxTTL = data.role.MaxTTL
+	}
+
+	if ttl == 0 {
+		ttl = b.System().DefaultLeaseTTL()
+	}
+	if maxTTL == 0 {
+		maxTTL = b.System().MaxLeaseTTL()
+	}
+	if ttl > maxTTL {
+		warnings = append(warnings, fmt.Sprintf("TTL %q is longer than permitted maxTTL %q, so maxTTL is being used", ttl, maxTTL))
+		ttl = maxTTL
+	}
+
+	if notAfterAlt != "" {
+		notAfter, err = time.Parse(time.RFC3339, notAfterAlt)
+		if err != nil {
+			return notAfter, warnings, errutil.UserError{Err: err.Error()}
+		}
+	} else {
+		notAfter = time.Now().Add(ttl)
+	}
+	if caSign != nil && notAfter.After(caSign.Certificate.NotAfter) {
+		// If it's not self-signed, verify that the issued certificate
+		// won't be valid past the lifetime of the CA certificate, and
+		// act accordingly. This is dependent based on the issuer's
+		// LeafNotAfterBehavior argument.
+		switch caSign.LeafNotAfterBehavior {
+		case certutil.PermitNotAfterBehavior:
+			// Explicitly do nothing.
+		case certutil.TruncateNotAfterBehavior:
+			notAfter = caSign.Certificate.NotAfter
+		case certutil.ErrNotAfterBehavior:
+			fallthrough
+		default:
+			return time.Time{}, warnings, errutil.UserError{Err: fmt.Sprintf(
+				"cannot satisfy request, as TTL would result in notAfter of %s that is beyond the expiration of the CA certificate at %s", notAfter.UTC().Format(time.RFC3339Nano), caSign.Certificate.NotAfter.UTC().Format(time.RFC3339Nano))}
+		}
+	}
+	return notAfter, warnings, nil
+}
+
 func convertRespToPKCS8(resp *logical.Response) error {
 	privRaw, ok := resp.Data["private_key"]
 	if !ok {

builtin/logical/pki/path_acme_order.go

@@ -22,6 +22,8 @@ import (
 	"golang.org/x/net/idna"
 )
 
+var maxAcmeCertTTL = 90 * (24 * time.Hour)
+
 func pathAcmeListOrders(b *backend) []*framework.Path {
 	return buildAcmeFrameworkPaths(b, patternAcmeListOrders, "/orders")
 }
@@ -510,6 +512,16 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil.
 		role:    ac.role,
 	}
 
+	normalNotAfter, _, err := getCertificateNotAfter(ac.sc.Backend, input, signingBundle)
+	if err != nil {
+		return nil, "", fmt.Errorf("failed computing certificate TTL from role/mount: %v: %w", err, ErrMalformed)
+	}
+
+	// Force a maximum 90 day TTL or lower for ACME
+	if time.Now().Add(maxAcmeCertTTL).Before(normalNotAfter) {
+		input.apiData.Raw["ttl"] = maxAcmeCertTTL
+	}
+
 	if csr.PublicKeyAlgorithm == x509.UnknownPublicKeyAlgorithm || csr.PublicKey == nil {
 		return nil, "", fmt.Errorf("%w: Refusing to sign CSR with empty PublicKey", ErrBadCSR)
 	}

builtin/logical/pki/path_acme_test.go

@@ -39,7 +39,7 @@ import (
 	"github.com/hashicorp/vault/vault"
 )
 
-// TestAcmeBasicWorkflow a basic test that will validate a basic ACME workflow using the Golang ACME client.
+// TestAcmeBasicWorkflow a test that will validate a basic ACME workflow using the Golang ACME client.
 func TestAcmeBasicWorkflow(t *testing.T) {
 	t.Parallel()
 	cluster, client, _ := setupAcmeBackend(t)
@@ -280,6 +280,13 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 
 			testAcmeCertSignedByCa(t, client, certs, "int-ca")
 
+			// Make sure the certificate has a NotAfter date of a maximum of 90 days
+			acmeCert, err := x509.ParseCertificate(certs[0])
+			require.NoError(t, err, "failed parsing acme cert bytes")
+			maxAcmeNotAfter := time.Now().Add(maxAcmeCertTTL)
+			if maxAcmeNotAfter.Before(acmeCert.NotAfter) {
+				require.Fail(t, fmt.Sprintf("certificate has a NotAfter value %v greater than ACME max ttl %v", acmeCert.NotAfter, maxAcmeNotAfter))
+			}
 			// Deactivate account
 			t.Logf("Testing deactivate account on %s", baseAcmeURL)
 			err = acmeClient.DeactivateReg(testCtx)
@@ -930,14 +937,20 @@ func setupAcmeBackendOnClusterAtPath(t *testing.T, cluster *vault.TestCluster, c
 		err := client.WithNamespace(namespace).Sys().Mount(mountName, &api.MountInput{
 			Type: "pki",
 			Config: api.MountConfigInput{
-				DefaultLeaseTTL: "16h",
+				DefaultLeaseTTL: "3000h",
-				MaxLeaseTTL:     "60h",
+				MaxLeaseTTL:     "600000h",
 			},
 		})
 		require.NoError(t, err, "failed to mount new PKI instance at "+mount)
 	}
 
-	_, err := client.Logical().WriteWithContext(context.Background(), mount+"/config/cluster", map[string]interface{}{
+	err := client.Sys().TuneMountWithContext(ctx, mountName, api.MountConfigInput{
+		DefaultLeaseTTL: "3000h",
+		MaxLeaseTTL:     "600000h",
+	})
+	require.NoError(t, err, "failed updating mount lease times "+mount)
+
+	_, err = client.Logical().WriteWithContext(context.Background(), mount+"/config/cluster", map[string]interface{}{
 		"path":     pathConfig,
 		"aia_path": "http://localhost:8200/cdn/" + mount,
 	})
@@ -979,8 +992,8 @@ func setupAcmeBackendOnClusterAtPath(t *testing.T, cluster *vault.TestCluster, c
 	// Sign the intermediate CSR using /pki
 	resp, err = client.Logical().Write(mount+"/issuer/root-ca/sign-intermediate", map[string]interface{}{
 		"csr":     intermediateCSR,
-		"ttl":     "720h",
+		"ttl":     "7100h",
-		"max_ttl": "7200h",
+		"max_ttl": "910000h",
 	})
 	require.NoError(t, err, "failed signing intermediary CSR")
 	intermediateCertPEM := resp.Data["certificate"].(string)
@@ -1005,8 +1018,8 @@ func setupAcmeBackendOnClusterAtPath(t *testing.T, cluster *vault.TestCluster, c
 	require.NoError(t, err, "failed updating default issuer")
 
 	_, err = client.Logical().Write(mount+"/roles/test-role", map[string]interface{}{
-		"ttl_duration":                "365h",
+		"ttl_duration":                "168h",
-		"max_ttl_duration":            "720h",
+		"max_ttl_duration":            "168h",
 		"key_type":                    "any",
 		"allowed_domains":             "localdomain",
 		"allow_subdomains":            "true",
@@ -1015,8 +1028,8 @@ func setupAcmeBackendOnClusterAtPath(t *testing.T, cluster *vault.TestCluster, c
 	require.NoError(t, err, "failed creating role test-role")
 
 	_, err = client.Logical().Write(mount+"/roles/acme", map[string]interface{}{
-		"ttl_duration":     "365h",
+		"ttl_duration":     "3650h",
-		"max_ttl_duration": "720h",
+		"max_ttl_duration": "7200h",
 		"key_type":         "any",
 	})
 	require.NoError(t, err, "failed creating role acme")

changelog/20981.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/pki: Limit ACME issued certificates NotAfter TTL to a maximum of 90 days 
+```