Fix ACME tidy to not reference acmeContext (hashicorp#21870)

* Fix ACME tidy to not reference acmeCtx acmeContext is useful for when we need to reference things with a ACME base URL, but everything used in tidy doesn't need this URL as it is not coming from an ACME request. Refactor tidy to remove references to acmeContext, including dependent functions in acme_state.go. Signed-off-by: Alexander Scheel <[email protected]> * Remove spurious log message Signed-off-by: Alexander Scheel <[email protected]> * Draft Tidy Acme Test with Backdate Storage + Backdate Sysxsx * Fixes to ACME tidy testing Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Correctly set account kid to update account status Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Add TestTidyAcmeWithSafetyBuffer Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Add test for disabling tidy operation Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Add acme_account_safety_buffer to auto-tidy config Resolve: hashicorp#21872 Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Add tests verifying tidy safety buffers Co-authored-by: kitography <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]> * Add account status validations and order cleanup tests --------- Signed-off-by: Alexander Scheel <[email protected]> Co-authored-by: kitography <[email protected]> Co-authored-by: Steve Clark <[email protected]>
cipherboy · Jul 17, 2023 · 4ec5e22 · 4ec5e22
1 parent d791908
commit 4ec5e22
Show file tree

Hide file tree

Showing 7 changed files with 546 additions and 33 deletions.
diff --git a/builtin/logical/pki/acme_challenges_test.go b/builtin/logical/pki/acme_challenges_test.go
@@ -279,7 +279,6 @@ func TestAcmeValidateTLSALPN01Challenge(t *testing.T) {
 
 		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
 		defer func() {
-			t.Logf("[alpn-server] defer context cancel executing")
 			cancel()
 		}()
 

diff --git a/builtin/logical/pki/acme_state.go b/builtin/logical/pki/acme_state.go
@@ -277,13 +277,13 @@ func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string,
 	return acct, nil
 }
 
-func (a *acmeState) UpdateAccount(ac *acmeContext, acct *acmeAccount) error {
+func (a *acmeState) UpdateAccount(sc *storageContext, acct *acmeAccount) error {
 	json, err := logical.StorageEntryJSON(acmeAccountPrefix+acct.KeyId, acct)
 	if err != nil {
 		return fmt.Errorf("error creating account entry: %w", err)
 	}
 
-	if err := ac.sc.Storage.Put(ac.sc.Context, json); err != nil {
+	if err := sc.Storage.Put(sc.Context, json); err != nil {
 		return fmt.Errorf("error writing account entry: %w", err)
 	}
 
@@ -539,10 +539,10 @@ func (a *acmeState) SaveOrder(ac *acmeContext, order *acmeOrder) error {
 	return nil
 }
 
-func (a *acmeState) ListOrderIds(ac *acmeContext, accountId string) ([]string, error) {
+func (a *acmeState) ListOrderIds(sc *storageContext, accountId string) ([]string, error) {
 	accountOrderPrefixPath := acmeAccountPrefix + accountId + "/orders/"
 
-	rawOrderIds, err := ac.sc.Storage.List(ac.sc.Context, accountOrderPrefixPath)
+	rawOrderIds, err := sc.Storage.List(sc.Context, accountOrderPrefixPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed listing order ids for account %s: %w", accountId, err)
 	}

diff --git a/builtin/logical/pki/path_acme_account.go b/builtin/logical/pki/path_acme_account.go
@@ -356,7 +356,7 @@ func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jws
 	}
 
 	if shouldUpdate {
-		err = b.acmeState.UpdateAccount(acmeCtx, account)
+		err = b.acmeState.UpdateAccount(acmeCtx.sc, account)
 		if err != nil {
 			return nil, fmt.Errorf("failed to update account: %w", err)
 		}
@@ -366,8 +366,8 @@ func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jws
 	return resp, nil
 }
 
-func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, keyThumbprint string, certTidyBuffer, accountTidyBuffer time.Duration) error {
-	thumbprintEntry, err := ac.sc.Storage.Get(ac.sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
+func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, sc *storageContext, keyThumbprint string, certTidyBuffer, accountTidyBuffer time.Duration) error {
+	thumbprintEntry, err := sc.Storage.Get(sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
 	if err != nil {
 		return fmt.Errorf("error retrieving thumbprint entry %v, unable to find corresponding account entry: %w", keyThumbprint, err)
 	}
@@ -386,13 +386,13 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 	}
 
 	// Now Get the Account:
-	accountEntry, err := ac.sc.Storage.Get(ac.sc.Context, acmeAccountPrefix+thumbprint.Kid)
+	accountEntry, err := sc.Storage.Get(sc.Context, acmeAccountPrefix+thumbprint.Kid)
 	if err != nil {
 		return err
 	}
 	if accountEntry == nil {
 		// We delete the Thumbprint Associated with the Account, and we are done
-		err = ac.sc.Storage.Delete(ac.sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
+		err = sc.Storage.Delete(sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
 		if err != nil {
 			return err
 		}
@@ -405,16 +405,17 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 	if err != nil {
 		return err
 	}
+	account.KeyId = thumbprint.Kid
 
 	// Tidy Orders On the Account
-	orderIds, err := as.ListOrderIds(ac, thumbprint.Kid)
+	orderIds, err := as.ListOrderIds(sc, thumbprint.Kid)
 	if err != nil {
 		return err
 	}
 	allOrdersTidied := true
 	maxCertExpiryUpdated := false
 	for _, orderId := range orderIds {
-		wasTidied, orderExpiry, err := b.acmeTidyOrder(ac, thumbprint.Kid, getOrderPath(thumbprint.Kid, orderId), certTidyBuffer)
+		wasTidied, orderExpiry, err := b.acmeTidyOrder(sc, thumbprint.Kid, getOrderPath(thumbprint.Kid, orderId), certTidyBuffer)
 		if err != nil {
 			return err
 		}
@@ -436,13 +437,13 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 		// If it is Revoked or Deactivated:
 		if (account.Status == AccountStatusRevoked || account.Status == AccountStatusDeactivated) && now.After(account.AccountRevokedDate.Add(accountTidyBuffer)) {
 			// We Delete the Account Associated with this Thumbprint:
-			err = ac.sc.Storage.Delete(ac.sc.Context, path.Join(acmeAccountPrefix, thumbprint.Kid))
+			err = sc.Storage.Delete(sc.Context, path.Join(acmeAccountPrefix, thumbprint.Kid))
 			if err != nil {
 				return err
 			}
 
 			// Now we delete the Thumbprint Associated with the Account:
-			err = ac.sc.Storage.Delete(ac.sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
+			err = sc.Storage.Delete(sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
 			if err != nil {
 				return err
 			}
@@ -451,7 +452,7 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 			// Revoke This Account
 			account.AccountRevokedDate = now
 			account.Status = AccountStatusRevoked
-			err := as.UpdateAccount(ac, &account)
+			err := as.UpdateAccount(sc, &account)
 			if err != nil {
 				return err
 			}
@@ -464,7 +465,7 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 	// already written above.
 	if maxCertExpiryUpdated && account.Status == AccountStatusValid {
 		// Update our expiry time we previously setup.
-		err := as.UpdateAccount(ac, &account)
+		err := as.UpdateAccount(sc, &account)
 		if err != nil {
 			return err
 		}

diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go
@@ -681,7 +681,7 @@ func (b *backend) acmeGetOrderHandler(ac *acmeContext, _ *logical.Request, field
 }
 
 func (b *backend) acmeListOrdersHandler(ac *acmeContext, _ *logical.Request, _ *framework.FieldData, uc *jwsCtx, _ map[string]interface{}, acct *acmeAccount) (*logical.Response, error) {
-	orderIds, err := b.acmeState.ListOrderIds(ac, acct.KeyId)
+	orderIds, err := b.acmeState.ListOrderIds(ac.sc, acct.KeyId)
 	if err != nil {
 		return nil, err
 	}
@@ -1020,11 +1020,11 @@ func parseOrderIdentifiers(data map[string]interface{}) ([]*ACMEIdentifier, erro
 	return identifiers, nil
 }
 
-func (b *backend) acmeTidyOrder(ac *acmeContext, accountId string, orderPath string, certTidyBuffer time.Duration) (bool, time.Time, error) {
+func (b *backend) acmeTidyOrder(sc *storageContext, accountId string, orderPath string, certTidyBuffer time.Duration) (bool, time.Time, error) {
 	// First we get the order; note that the orderPath includes the account
 	// It's only accessed at acme/orders/<order_id> with the account context
 	// It's saved at acme/<account_id>/orders/<orderId>
-	entry, err := ac.sc.Storage.Get(ac.sc.Context, orderPath)
+	entry, err := sc.Storage.Get(sc.Context, orderPath)
 	if err != nil {
 		return false, time.Time{}, fmt.Errorf("error loading order: %w", err)
 	}
@@ -1069,20 +1069,20 @@ func (b *backend) acmeTidyOrder(ac *acmeContext, accountId string, orderPath str
 
 	// First Authorizations
 	for _, authorizationId := range order.AuthorizationIds {
-		err = ac.sc.Storage.Delete(ac.sc.Context, getAuthorizationPath(accountId, authorizationId))
+		err = sc.Storage.Delete(sc.Context, getAuthorizationPath(accountId, authorizationId))
 		if err != nil {
 			return false, orderExpiry, err
 		}
 	}
 
 	// Normal Tidy will Take Care of the Certificate, we need to clean up the certificate to account tracker though
-	err = ac.sc.Storage.Delete(ac.sc.Context, getAcmeSerialToAccountTrackerPath(accountId, order.CertificateSerialNumber))
+	err = sc.Storage.Delete(sc.Context, getAcmeSerialToAccountTrackerPath(accountId, order.CertificateSerialNumber))
 	if err != nil {
 		return false, orderExpiry, err
 	}
 
 	// And Finally, the order:
-	err = ac.sc.Storage.Delete(ac.sc.Context, orderPath)
+	err = sc.Storage.Delete(sc.Context, orderPath)
 	if err != nil {
 		return false, orderExpiry, err
 	}

diff --git a/builtin/logical/pki/path_tidy.go b/builtin/logical/pki/path_tidy.go
@@ -1546,18 +1546,8 @@ func (b *backend) doTidyAcme(ctx context.Context, req *logical.Request, logger h
 	b.tidyStatus.acmeAccountsCount = uint(len(thumbprints))
 	b.tidyStatusLock.Unlock()
 
-	baseUrl, _, err := getAcmeBaseUrl(sc, req)
-	if err != nil {
-		return err
-	}
-
-	acmeCtx := &acmeContext{
-		baseUrl: baseUrl,
-		sc:      sc,
-	}
-
 	for _, thumbprint := range thumbprints {
-		err := b.tidyAcmeAccountByThumbprint(b.acmeState, acmeCtx, thumbprint, config.SafetyBuffer, config.AcmeAccountSafetyBuffer)
+		err := b.tidyAcmeAccountByThumbprint(b.acmeState, sc, thumbprint, config.SafetyBuffer, config.AcmeAccountSafetyBuffer)
 		if err != nil {
 			logger.Warn("error tidying account %v: %v", thumbprint, err.Error())
 		}
@@ -1838,6 +1828,13 @@ func (b *backend) pathConfigAutoTidyWrite(ctx context.Context, req *logical.Requ
 		config.TidyAcme = tidyAcmeRaw.(bool)
 	}
 
+	if acmeAccountSafetyBufferRaw, ok := d.GetOk("acme_account_safety_buffer"); ok {
+		config.AcmeAccountSafetyBuffer = time.Duration(acmeAccountSafetyBufferRaw.(int)) * time.Second
+		if config.AcmeAccountSafetyBuffer < 1*time.Second {
+			return logical.ErrorResponse(fmt.Sprintf("given acme_account_safety_buffer must be at least one second; got: %v", acmeAccountSafetyBufferRaw)), nil
+		}
+	}
+
 	if config.Enabled && !config.IsAnyTidyEnabled() {
 		return logical.ErrorResponse("Auto-tidy enabled but no tidy operations were requested. Enable at least one tidy operation to be run (" + config.AnyTidyConfig() + ")."), nil
 	}