Add issuance checks around NotAfter and NotBefore (#10478) (#10691)

* Refuse to issue or sign certs that have a NotAfter before NotBefore
* Add checks to ensure that validity period of cert being issued is contained within CA's validity period

builtin/logical/pki/backend_test.go

@@ -617,8 +617,9 @@ func generateURLSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s
 			Operation: logical.UpdateOperation,
 			Path:      "root/generate/exported",
 			Data: map[string]interface{}{
-				"common_name": "Root Cert",
-				"ttl":         "180h",
+				"common_name":         "Root Cert",
+				"ttl":                 "180h",
+				"not_before_duration": "3h",
 			},
 			Check: func(resp *logical.Response) error {
 				if resp.Secret != nil && resp.Secret.LeaseID != "" {
@@ -2247,8 +2248,9 @@ func runTestSignVerbatim(t *testing.T, keyType string) {
 
 	// generate root
 	rootData := map[string]interface{}{
-		"common_name": "test.com",
-		"not_after":   "9999-12-31T23:59:59Z",
+		"common_name":         "test.com",
+		"not_after":           "9999-12-31T23:59:59Z",
+		"not_before_duration": "3h",
 	}
 
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{
@@ -7599,6 +7601,87 @@ func TestIssuance_AlwaysEnforceErr(t *testing.T) {
 	})
 }
 
+// TestIssuance_NotBefore verifies that issuance fails with a NotAfter in the past.
+func TestIssuance_NotBefore(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 = CBWrite(b, s, "roles/test-role", map[string]interface{}{
+		"allow_any_name":         true,
+		"key_type":               "ec",
+		"allowed_serial_numbers": "*",
+	})
+
+	// Make sure sign-intermediate fails if the NotAfter is in the past
+	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,
+			"not_before_duration": "1s",
+			"not_after":           time.Now().Add(-2 * time.Hour).Format(time.RFC3339),
+		})
+		require.ErrorContains(t, err, "notAfter before notBefore", "expected error from sign csr got: %v", resp)
+
+		_, err = CBWrite(b, s, "issuer/root-ca/sign-intermediate", map[string]interface{}{
+			"csr":                 csr,
+			"use_csr_values":      true,
+			"not_before_duration": "1s",
+			"not_after":           time.Now().Add(2 * time.Hour).Format(time.RFC3339),
+		})
+		requireSuccessNonNilResponse(t, resp, err, "notAfter after notBefore")
+		time.Sleep(time.Second)
+	})
+
+	// Make sure sign of leaf cert fails if the NotAfter is in the past
+	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{}{
+			"csr":       csrPem,
+			"not_after": time.Now().Add(-2 * time.Hour).Format(time.RFC3339),
+		})
+		require.ErrorContains(t, err, "notAfter before notBefore", "expected error from sign csr got: %v", resp)
+
+		resp, err = CBWrite(b, s, "issuer/root-ca/sign/test-role", map[string]interface{}{
+			"csr":       csrPem,
+			"not_after": time.Now().Add(2 * time.Hour).Format(time.RFC3339),
+		})
+		requireSuccessNonNilResponse(t, resp, err, "sign should have succeeded with a NotAfter in the future")
+	})
+
+	// Make sure issue of leaf cert fails if the NotAfter is in the past
+	t.Run("issue-leaf-csr", func(t *testing.T) {
+		resp, err = CBWrite(b, s, "issuer/root-ca/issue/test-role", map[string]interface{}{
+			"common_name": "leaf.example.com",
+			"not_after":   time.Now().Add(-2 * time.Hour).Format(time.RFC3339),
+		})
+		require.ErrorContains(t, err, "notAfter before notBefore", "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{}{
+			"common_name": "leaf.example.com",
+			"not_after":   time.Now().Add(2 * time.Hour).Format(time.RFC3339),
+		})
+		requireSuccessNonNilResponse(t, resp, err, "issue should have worked with a NotAfter in the future")
+	})
+}
+
 // TestIssuance_SignIntermediateKeyUsages tests the field "key_usage" both when directly generating a root certificate,
 // and when signing a CA CSR.  In particular, this test verifies that:
 // - the key usage DigitalSignature is added if present
@@ -7701,3 +7784,122 @@ var (
 	edCAKey   string
 	edCACert  string
 )
+
+func getCACert(notBefore, notAfter time.Time) (string, error) {
+	caCertTemplate := &x509.Certificate{
+		Subject: pkix.Name{
+			CommonName: "root.localhost",
+		},
+		DNSNames:              []string{"root.localhost"},
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		SerialNumber:          big.NewInt(mathrand.Int63()),
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+	}
+
+	rak, err := cryptoutil.GenerateRSAKey(rand.Reader, 2048)
+	if err != nil {
+		return "", err
+	}
+
+	caBytes, err := x509.CreateCertificate(rand.Reader, caCertTemplate, caCertTemplate, rak.Public(), rak)
+	if err != nil {
+		panic(err)
+	}
+	pemCert := string(pem.EncodeToMemory(&pem.Block{
+		Type:  "CERTIFICATE",
+		Bytes: caBytes,
+	}))
+
+	pemKey := string(pem.EncodeToMemory(&pem.Block{
+		Type:  "RSA PRIVATE KEY",
+		Bytes: x509.MarshalPKCS1PrivateKey(rak),
+	}))
+
+	return pemCert + "\n" + pemKey, nil
+}
+
+func requireResponse(t *testing.T, expectErr bool, resp *logical.Response, err error, msgAndArgs ...interface{}) {
+	t.Helper()
+
+	if expectErr {
+		if err == nil {
+			err = resp.Error()
+		}
+		require.Error(t, err, msgAndArgs)
+	} else {
+		requireSuccessNonNilResponse(t, resp, err, msgAndArgs)
+	}
+}
+
+// TestIssuance_ValidityPeriodContainedByCA attempt to issue/sign certs with a 1h ttl,
+// having first imported a CA with NotBefore/NotAfter set to test various conditions.
+func TestIssuance_ValidityPeriodContainedByCA(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		caNotBefore time.Time
+		caNotAfter  time.Time
+		expectErr   bool
+	}{
+		{
+			name:        "valid",
+			caNotBefore: time.Now().Add(-time.Hour),
+			caNotAfter:  time.Now().Add(5 * time.Hour),
+			expectErr:   false,
+		},
+		{
+			name:        "caExpired",
+			caNotBefore: time.Now().Add(-time.Hour),
+			caNotAfter:  time.Now().Add(-time.Minute),
+			expectErr:   true,
+		},
+		{
+			name:        "caWillExpire",
+			caNotBefore: time.Now().Add(-time.Hour),
+			caNotAfter:  time.Now().Add(30 * time.Minute),
+			expectErr:   true,
+		},
+		{
+			name:        "caNotYet",
+			caNotBefore: time.Now().Add(time.Hour),
+			caNotAfter:  time.Now().Add(2 * time.Hour),
+			expectErr:   true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			b, s := CreateBackendWithStorage(t)
+
+			ca, err := getCACert(tc.caNotBefore, tc.caNotAfter)
+			require.NoError(t, err)
+
+			resp, err := CBWrite(b, s, "config/ca", map[string]interface{}{"pem_bundle": ca})
+			requireSuccessNonNilResponse(t, resp, err)
+
+			resp, err = CBWrite(b, s, "roles/test-role", map[string]interface{}{
+				"allow_any_name":         true,
+				"key_type":               "ec",
+				"allowed_serial_numbers": "*",
+			})
+
+			t.Run("sign-leaf-csr", func(t *testing.T) {
+				_, csrPem := generateTestCsr(t, certutil.ECPrivateKey, 256)
+
+				resp, err = CBWrite(b, s, "sign/test-role", map[string]interface{}{
+					"csr":       csrPem,
+					"not_after": time.Now().Add(time.Hour).Format(time.RFC3339),
+				})
+				requireResponse(t, tc.expectErr, resp, err)
+			})
+
+			t.Run("issue-leaf-csr", func(t *testing.T) {
+				resp, err = CBWrite(b, s, "issue/test-role", map[string]interface{}{
+					"common_name": "leaf.example.com",
+					"not_after":   time.Now().Add(time.Hour).Format(time.RFC3339),
+				})
+				requireResponse(t, tc.expectErr, resp, err)
+			})
+		})
+	}
+}

builtin/logical/pki/issuing/issue_common.go

@@ -999,6 +999,15 @@ func GetCertificateNotAfter(b logical.SystemView, role *RoleEntry, input CertNot
 	if err != nil {
 		return time.Time{}, warnings, err
 	}
+	notBefore := time.Now().Add(-role.NotBeforeDuration)
+	if notAfter.Before(notBefore) {
+		return time.Time{}, warnings, errutil.UserError{Err: "notAfter before notBefore"}
+	}
+
+	if caSign != nil && notBefore.Before(caSign.Certificate.NotBefore) {
+		return time.Time{}, warnings, errutil.UserError{Err: "notBefore before signer's notBefore"}
+	}
+
 	return notAfter, warnings, nil
 }
 

changelog/_10478.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Return error when issuing/signing certs whose NotAfter is before NotBefore or whose validity period isn't contained by the CA's.
+```