pki: use revocationInfo.RevocationTimeUTC when revoking certs with ti… (#9609)

* pki: use revocationInfo.RevocationTimeUTC when revoking certs with tidy_revoked_certs set to true

* update comment

* tidy: use same time snapshot for OR comparison

builtin/logical/pki/backend_test.go

@@ -2910,6 +2910,118 @@ func setCerts() {
 	rsaCACert = strings.TrimSpace(string(pem.EncodeToMemory(caCertPEMBlock)))
 }
 
+func TestBackend_RevokePlusTidy_Intermediate(t *testing.T) {
+	// Enable PKI secret engine
+	coreConfig := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"pki": Factory,
+		},
+	}
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: vaulthttp.Handler,
+	})
+	cluster.Start()
+	defer cluster.Cleanup()
+	cores := cluster.Cores
+	vault.TestWaitActive(t, cores[0].Core)
+	client := cores[0].Client
+
+	var err error
+
+	// Mount /pki as a root CA
+	err = client.Sys().Mount("pki", &api.MountInput{
+		Type: "pki",
+		Config: api.MountConfigInput{
+			DefaultLeaseTTL: "16h",
+			MaxLeaseTTL:     "32h",
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Set the cluster's certificate as the root CA in /pki
+	pemBundleRootCA := string(cluster.CACertPEM) + string(cluster.CAKeyPEM)
+	_, err = client.Logical().Write("pki/config/ca", map[string]interface{}{
+		"pem_bundle": pemBundleRootCA,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Mount /pki2 to operate as an intermediate CA
+	err = client.Sys().Mount("pki2", &api.MountInput{
+		Type: "pki",
+		Config: api.MountConfigInput{
+			DefaultLeaseTTL: "16h",
+			MaxLeaseTTL:     "32h",
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Create a CSR for the intermediate CA
+	secret, err := client.Logical().Write("pki2/intermediate/generate/internal", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	intermediateCSR := secret.Data["csr"].(string)
+
+	// Sign the intermediate CSR using /pki
+	secret, err = client.Logical().Write("pki/root/sign-intermediate", map[string]interface{}{
+		"permitted_dns_domains": ".myvault.com",
+		"csr":                   intermediateCSR,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	intermediateCertSerial := secret.Data["serial_number"].(string)
+	intermediateCASerialColon := strings.ReplaceAll(strings.ToLower(intermediateCertSerial), ":", "-")
+
+	// Get the intermediate cert after signing
+	secret, err = client.Logical().Read("pki/cert/" + intermediateCASerialColon)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if secret == nil || len(secret.Data) == 0 || len(secret.Data["certificate"].(string)) == 0 {
+		t.Fatal("expected certificate information from read operation")
+	}
+
+	// Issue a revoke on on /pki
+	_, err = client.Logical().Write("pki/revoke", map[string]interface{}{
+		"serial_number": intermediateCertSerial,
+	})
+
+	// Revoke adds a fixed 2s buffer, so we sleep for a bit longer to ensure
+	// the revocation time is past the current time.
+	time.Sleep(3 * time.Second)
+
+	// Issue a tidy on /pki
+	_, err = client.Logical().Write("pki/tidy", map[string]interface{}{
+		"tidy_cert_store":    true,
+		"tidy_revoked_certs": true,
+		"safety_buffer":      "1s",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Sleep a bit to make sure we're past the safety buffer
+	time.Sleep(2 * time.Second)
+
+	// Attempt to read the intermediate cert after revoke + tidy, and ensure
+	// that it's no longer present
+	secret, err = client.Logical().Read("pki/cert/" + intermediateCASerialColon)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if secret != nil {
+		t.Fatalf("expected empty response data, got: %#v", secret.Data)
+	}
+
+}
+
 var (
 	initTest  sync.Once
 	rsaCAKey  string

builtin/logical/pki/path_tidy.go

@@ -178,7 +178,13 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr
 						return errwrap.Wrapf(fmt.Sprintf("unable to parse stored revoked certificate with serial %q: {{err}}", serial), err)
 					}
 
-					if time.Now().After(revokedCert.NotAfter.Add(bufferDuration)) {
+					// Remove the matched certificate entries from revoked/ and
+					// cert/ paths. We compare against both the NotAfter time
+					// within the cert itself and the time from the revocation
+					// entry, and perform tidy if either one tells us that the
+					// certificate has already been revoked.
+					now := time.Now()
+					if now.After(revokedCert.NotAfter.Add(bufferDuration)) || now.After(revInfo.RevocationTimeUTC.Add(bufferDuration)) {
 						if err := req.Storage.Delete(ctx, "revoked/"+serial); err != nil {
 							return errwrap.Wrapf(fmt.Sprintf("error deleting serial %q from revoked list: {{err}}", serial), err)
 						}