From 1efcefc3693ce57d67cef1d1af6521ff368de13b Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 27 Jul 2020 17:08:59 -0700 Subject: [PATCH 1/3] pki: use revocationInfo.RevocationTimeUTC when revoking certs with tidy_revoked_certs set to true --- builtin/logical/pki/backend_test.go | 112 ++++++++++++++++++++++++++++ builtin/logical/pki/path_tidy.go | 7 +- 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 7c58aeb63570..0bcc1019e3ec 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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 and Cert auth method + 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 diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index a98c14534ef0..e39a732dcb6a 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -178,7 +178,12 @@ 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. + if time.Now().After(revokedCert.NotAfter.Add(bufferDuration)) || time.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) } From 0598af3efb7437a67b8f78aff14c2c17e8c2098f Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 27 Jul 2020 17:17:37 -0700 Subject: [PATCH 2/3] update comment --- builtin/logical/pki/backend_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 0bcc1019e3ec..8a96b81c2c4d 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2911,7 +2911,7 @@ func setCerts() { } func TestBackend_RevokePlusTidy_Intermediate(t *testing.T) { - // Enable PKI secret engine and Cert auth method + // Enable PKI secret engine coreConfig := &vault.CoreConfig{ LogicalBackends: map[string]logical.Factory{ "pki": Factory, From 89425b08e762eff6d59c90ad3a14f393405671fb Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 28 Jul 2020 11:18:35 -0700 Subject: [PATCH 3/3] tidy: use same time snapshot for OR comparison --- builtin/logical/pki/path_tidy.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go index e39a732dcb6a..2a03984402ae 100644 --- a/builtin/logical/pki/path_tidy.go +++ b/builtin/logical/pki/path_tidy.go @@ -183,7 +183,8 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr // 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. - if time.Now().After(revokedCert.NotAfter.Add(bufferDuration)) || time.Now().After(revInfo.RevocationTimeUTC.Add(bufferDuration)) { + 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) }