diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 5a9c480dd28e..50ccc62116fd 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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") } } diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index 2a03984402ae..8a125110cb06 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/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 } diff --git a/changelog/11367.txt b/changelog/11367.txt new file mode 100644 index 000000000000..5e79b1c73658 --- /dev/null +++ b/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 +``` \ No newline at end of file diff --git a/website/pages/api-docs/secret/pki/index.mdx b/website/pages/api-docs/secret/pki/index.mdx index 8e0633bc02f0..2392c63ec2ca 100644 --- a/website/pages/api-docs/secret/pki/index.mdx +++ b/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