From a8cafab083109349eea2f927f7aa2914951f433d Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 19 Apr 2021 09:40:40 -0700 Subject: [PATCH] pki: fix tidy removal on revoked entries (#11367) * pki: fix tidy removal on revoked entries * add CL entry --- builtin/logical/pki/backend_test.go | 75 +++++++++++++++++++++++-- builtin/logical/pki/path_tidy.go | 18 +++--- changelog/11367.txt | 3 + website/content/api-docs/secret/pki.mdx | 10 ++-- 4 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 changelog/11367.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 3c18493a8..a4329633e 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" @@ -2985,6 +2986,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) @@ -3023,14 +3025,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) } - if secret != nil { - t.Fatalf("expected empty response data, got: %#v", secret.Data) + 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) + } + + 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 5fd8016c0..ccb8f5a7e 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 000000000..5e79b1c73 --- /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/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 80ac35621..3073ff54b 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -1572,9 +1572,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 @@ -1605,7 +1607,7 @@ $ curl \ # Cluster Scalability Most non-introspection operations in the PKI secrets engine require a write to -storage, and so are forwarded to the cluster's active node for execution. +storage, and so are forwarded to the cluster's active node for execution. This table outlines which operations can be executed on performance standbys and thus scale horizontally.