pki: fix tidy removal on revoked entries (#11367) (#11401)

* pki: fix tidy removal on revoked entries

* add CL entry

builtin/logical/pki/backend_test.go

@@ -13,6 +13,7 @@ import (
 	"encoding/base64"
 	"encoding/pem"
 	"fmt"
+	"io/ioutil"
 	"math"
 	"math/big"
 	mathrand "math/rand"
@@ -2979,6 +2980,7 @@ func TestBackend_RevokePlusTidy_Intermediate(t *testing.T) {
 	secret, err = client.Logical().Write("pki/root/sign-intermediate", map[string]interface{}{
 		"permitted_dns_domains": ".myvault.com",
 		"csr":                   intermediateCSR,
+		"ttl":                   "10s",
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -3017,14 +3019,77 @@ func TestBackend_RevokePlusTidy_Intermediate(t *testing.T) {
 	// 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)
+	// Get CRL and ensure the tidied cert is still in the list after the tidy
+	// operation since it's not past the NotAfter (ttl) value yet.
+	req := client.NewRequest("GET", "/v1/pki/crl")
+	resp, err := client.RawRequest(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer resp.Body.Close()
+
+	crlBytes, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if len(crlBytes) == 0 {
+		t.Fatalf("expected CRL in response body")
+	}
+
+	crl, err := x509.ParseDERCRL(crlBytes)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	revokedCerts := crl.TBSCertList.RevokedCertificates
+	if len(revokedCerts) == 0 {
+		t.Fatal("expected CRL to be non-empty")
+	}
+
+	sn := certutil.GetHexFormatted(revokedCerts[0].SerialNumber.Bytes(), ":")
+	if sn != intermediateCertSerial {
+		t.Fatalf("expected: %v, got: %v", intermediateCertSerial, sn)
+	}
+
+	// Wait for cert to expire
+	time.Sleep(10 * 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)
+
+	req = client.NewRequest("GET", "/v1/pki/crl")
+	resp, err = client.RawRequest(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer resp.Body.Close()
+
+	crlBytes, err = ioutil.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if len(crlBytes) == 0 {
+		t.Fatalf("expected CRL in response body")
+	}
+
+	crl, err = x509.ParseDERCRL(crlBytes)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if secret != nil {
-		t.Fatalf("expected empty response data, got: %#v", secret.Data)
+
+	revokedCerts = crl.TBSCertList.RevokedCertificates
+	if len(revokedCerts) != 0 {
+		t.Fatal("expected CRL to be empty")
 	}
 
 }

builtin/logical/pki/path_tidy.go

@@ -138,7 +138,7 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr
 				b.revokeStorageLock.Lock()
 				defer b.revokeStorageLock.Unlock()
 
-				tidiedRevoked := false
+				rebuildCRL := false
 
 				revokedSerials, err := req.Storage.List(ctx, "revoked/")
 				if err != nil {
@@ -178,24 +178,22 @@ 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)
 					}
 
-					// 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)) {
+					// Only remove the entries from revoked/ and certs/ if we're
+					// past its NotAfter value. This is because we use the
+					// information on revoked/ to build the CRL and the
+					// information on certs/ for lookup.
+					if time.Now().After(revokedCert.NotAfter.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)
 						}
 						if err := req.Storage.Delete(ctx, "certs/"+serial); err != nil {
 							return errwrap.Wrapf(fmt.Sprintf("error deleting serial %q from store when tidying revoked: {{err}}", serial), err)
 						}
-						tidiedRevoked = true
+						rebuildCRL = true
 					}
 				}
 
-				if tidiedRevoked {
+				if rebuildCRL {
 					if err := buildCRL(ctx, b, req, false); err != nil {
 						return err
 					}

changelog/11367.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+pki: Only remove revoked entry for certificates during tidy if they are past their NotAfter value
+```

website/pages/api-docs/secret/pki/index.mdx

@@ -1573,9 +1573,11 @@ expiration time.
 - `tidy_cert_store` `(bool: false)` Specifies whether to tidy up the certificate
   store.
 
-- `tidy_revoked_certs` `(bool: false)` Set to true to expire all revoked and
-  expired certificates, removing them both from the CRL and from storage. The
-  CRL will be rotated if this causes any values to be removed.
+- `tidy_revoked_certs` `(bool: false)` Set to true to remove all invalid and
+  expired certificates from storage. A revoked storage entry is considered
+  invalid if the entry is empty, or the value within the entry is empty. If a
+  certificate is removed due to expiry, the entry will also be removed from the
+  CRL, and the CRL will be rotated.
 
 - `safety_buffer` `(string: "")` Specifies A duration (given as an integer
   number of seconds or a string; defaults to `72h`) used as a safety buffer to