diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 990fdea4e..1795fcab8 100644 --- a/builtin/logical/pki/cert_util.go +++ b/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 - 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()} - } - } 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))} - } - } + notAfter, ttlWarnings, err := getCertificateNotAfter(b, data, caSign) + if err != nil { + return nil, warnings, err } + 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 { diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index bd3ed2bda..294274d52 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/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) } diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index b43e959de..c7d1265d1 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/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", - MaxLeaseTTL: "60h", + DefaultLeaseTTL: "3000h", + 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", - "max_ttl": "7200h", + "ttl": "7100h", + "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", - "max_ttl_duration": "720h", + "ttl_duration": "168h", + "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", - "max_ttl_duration": "720h", + "ttl_duration": "3650h", + "max_ttl_duration": "7200h", "key_type": "any", }) require.NoError(t, err, "failed creating role acme") diff --git a/changelog/20981.txt b/changelog/20981.txt new file mode 100644 index 000000000..26a5304c5 --- /dev/null +++ b/changelog/20981.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Limit ACME issued certificates NotAfter TTL to a maximum of 90 days +```