Add a last issued date on ACME accounts (#20534)

* Add a last issued date on ACME accounts

 - When we issue a new ACME certificate, attempt to update the account's last issued field
 - Within ACME account tidy, use both account creation and last issue date to provide a buffer before we mark the account as revoked.
 - Cleanup the cert serial to account tracker
 - Misc formatting fixes in JSON objects

* Move account max-cert-expiry updates within tidy

 - Perform the account update of max-cert-expiry within
   the tidy operation as it has the account write lock
   and is already iterating over all orders.
 - With this the order path does not need any account
   level locks

* Prefix ACME account status constants with AccountStatusX

builtin/logical/pki/acme_state.go

@@ -207,20 +207,21 @@ func (aas ACMEAccountStatus) String() string {
 }
 
 const (
-	StatusValid       ACMEAccountStatus = "valid"
+	AccountStatusValid       ACMEAccountStatus = "valid"
-	StatusDeactivated ACMEAccountStatus = "deactivated"
+	AccountStatusDeactivated ACMEAccountStatus = "deactivated"
-	StatusRevoked     ACMEAccountStatus = "revoked"
+	AccountStatusRevoked     ACMEAccountStatus = "revoked"
 )
 
 type acmeAccount struct {
 	KeyId                string            `json:"-"`
 	Status               ACMEAccountStatus `json:"status"`
 	Contact              []string          `json:"contact"`
-	TermsOfServiceAgreed bool              `json:"termsOfServiceAgreed"`
+	TermsOfServiceAgreed bool              `json:"terms-of-service-agreed"`
 	Jwk                  []byte            `json:"jwk"`
 	AcmeDirectory        string            `json:"acme-directory"`
-	AccountCreatedDate   time.Time         `json:"account_created_date"`
+	AccountCreatedDate   time.Time         `json:"account-created-date"`
-	AccountRevokedDate   time.Time         `json:"account_revoked_date"`
+	MaxCertExpiry        time.Time         `json:"account-max-cert-expiry"`
+	AccountRevokedDate   time.Time         `json:"account-revoked-date"`
 	Eab                  *eabType          `json:"eab"`
 }
 
@@ -291,7 +292,7 @@ func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string,
 		Contact:              contact,
 		TermsOfServiceAgreed: termsOfServiceAgreed,
 		Jwk:                  c.Jwk,
-		Status:               StatusValid,
+		Status:               AccountStatusValid,
 		AcmeDirectory:        ac.acmeDirectory,
 		AccountCreatedDate:   time.Now(),
 		Eab:                  eab,
@@ -596,7 +597,7 @@ type acmeCertEntry struct {
 }
 
 func (a *acmeState) TrackIssuedCert(ac *acmeContext, accountId string, serial string, orderId string) error {
-	path := acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial)
+	path := getAcmeSerialToAccountTrackerPath(accountId, serial)
 	entry := acmeCertEntry{
 		Order: orderId,
 	}
@@ -696,6 +697,10 @@ func (a *acmeState) ListEabIds(sc *storageContext) ([]string, error) {
 	return ids, nil
 }
 
+func getAcmeSerialToAccountTrackerPath(accountId string, serial string) string {
+	return acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial)
+}
+
 func getAuthorizationPath(accountId string, authId string) string {
 	return acmeAccountPrefix + accountId + "/authorizations/" + authId
 }

builtin/logical/pki/acme_wrappers.go

@@ -197,7 +197,7 @@ func (b *backend) acmeAccountRequiredWrapper(op acmeAccountRequiredOperation) fr
 			return nil, err
 		}
 
-		if account.Status != StatusValid {
+		if account.Status != AccountStatusValid {
 			// Treating "revoked" and "deactivated" as the same here.
 			return nil, fmt.Errorf("%w: account in status: %s", ErrUnauthorized, account.Status)
 		}

builtin/logical/pki/path_acme_account.go

@@ -6,6 +6,7 @@ package pki
 import (
 	"fmt"
 	"net/http"
+	"path"
 	"strings"
 	"time"
 
@@ -332,7 +333,7 @@ func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jws
 
 	// Per RFC 8555 7.3.6 Account deactivation, if we were previously deactivated, we should return
 	// unauthorized. There is no way to reactivate any accounts per ACME RFC.
-	if account.Status != StatusValid {
+	if account.Status != AccountStatusValid {
 		// Treating "revoked" and "deactivated" as the same here.
 		return nil, ErrUnauthorized
 	}
@@ -346,11 +347,11 @@ func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jws
 
 	// Check to process account de-activation status was requested.
 	// 7.3.6. Account Deactivation
-	if string(StatusDeactivated) == status {
+	if string(AccountStatusDeactivated) == status {
 		shouldUpdate = true
 		// TODO: This should cancel any ongoing operations (do not revoke certs),
 		//       perhaps we should delete this account here?
-		account.Status = StatusDeactivated
+		account.Status = AccountStatusDeactivated
 		account.AccountRevokedDate = time.Now()
 	}
 
@@ -366,7 +367,7 @@ func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jws
 }
 
 func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, keyThumbprint string, certTidyBuffer, accountTidyBuffer time.Duration) error {
-	thumbprintEntry, err := ac.sc.Storage.Get(ac.sc.Context, acmeThumbprintPrefix+keyThumbprint)
+	thumbprintEntry, err := ac.sc.Storage.Get(ac.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)
 	}
@@ -391,7 +392,7 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 	}
 	if accountEntry == nil {
 		// We delete the Thumbprint Associated with the Account, and we are done
-		err = ac.sc.Storage.Delete(ac.sc.Context, acmeThumbprintPrefix+keyThumbprint)
+		err = ac.sc.Storage.Delete(ac.sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
 		if err != nil {
 			return err
 		}
@@ -411,36 +412,45 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 		return err
 	}
 	allOrdersTidied := true
+	maxCertExpiryUpdated := false
 	for _, orderId := range orderIds {
-		wasTidied, err := b.acmeTidyOrder(ac, thumbprint.Kid, acmeAccountPrefix+thumbprint.Kid+"/orders/"+orderId, ac.sc, certTidyBuffer)
+		wasTidied, orderExpiry, err := b.acmeTidyOrder(ac, thumbprint.Kid, getOrderPath(thumbprint.Kid, orderId), certTidyBuffer)
 		if err != nil {
 			return err
 		}
 		if !wasTidied {
 			allOrdersTidied = false
 		}
+
+		if !orderExpiry.IsZero() && account.MaxCertExpiry.Before(orderExpiry) {
+			account.MaxCertExpiry = orderExpiry
+			maxCertExpiryUpdated = true
+		}
 	}
 
-	if allOrdersTidied && time.Now().After(account.AccountCreatedDate.Add(accountTidyBuffer)) {
+	now := time.Now()
+	if allOrdersTidied &&
+		now.After(account.AccountCreatedDate.Add(accountTidyBuffer)) &&
+		now.After(account.MaxCertExpiry.Add(accountTidyBuffer)) {
 		// Tidy this account
 		// If it is Revoked or Deactivated:
-		if (account.Status == StatusRevoked || account.Status == StatusDeactivated) && time.Now().After(account.AccountRevokedDate.Add(accountTidyBuffer)) {
+		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, acmeAccountPrefix+thumbprint.Kid)
+			err = ac.sc.Storage.Delete(ac.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, acmeThumbprintPrefix+keyThumbprint)
+			err = ac.sc.Storage.Delete(ac.sc.Context, path.Join(acmeThumbprintPrefix, keyThumbprint))
 			if err != nil {
 				return err
 			}
 			b.tidyStatusIncDeletedAcmeAccountCount()
-		} else if account.Status == StatusValid {
+		} else if account.Status == AccountStatusValid {
 			// Revoke This Account
-			account.AccountRevokedDate = time.Now()
+			account.AccountRevokedDate = now
-			account.Status = StatusRevoked
+			account.Status = AccountStatusRevoked
 			err := as.UpdateAccount(ac, &account)
 			if err != nil {
 				return err
@@ -449,5 +459,16 @@ func (b *backend) tidyAcmeAccountByThumbprint(as *acmeState, ac *acmeContext, ke
 		}
 	}
 
+	// Only update the account if we modified the max cert expiry values and the account is still valid,
+	// to prevent us from adding back a deleted account or not re-writing the revoked account that was
+	// already written above.
+	if maxCertExpiryUpdated && account.Status == AccountStatusValid {
+		// Update our expiry time we previously setup.
+		err := as.UpdateAccount(ac, &account)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }

builtin/logical/pki/path_acme_order.go

@@ -245,7 +245,8 @@ func (b *backend) acmeFinalizeOrderHandler(ac *acmeContext, _ *logical.Request,
 		return nil, fmt.Errorf("%w: order is status %s, needs to be in ready state", ErrOrderNotReady, order.Status)
 	}
 
-	if !order.Expires.IsZero() && time.Now().After(order.Expires) {
+	now := time.Now()
+	if !order.Expires.IsZero() && now.After(order.Expires) {
 		return nil, fmt.Errorf("%w: order %s is expired", ErrMalformed, orderId)
 	}
 
@@ -885,25 +886,30 @@ func parseOrderIdentifiers(data map[string]interface{}) ([]*ACMEIdentifier, erro
 	return identifiers, nil
 }
 
-func (b *backend) acmeTidyOrder(ac *acmeContext, accountId string, orderPath string, sc *storageContext, certTidyBuffer time.Duration) (wasTidied bool, err error) {
+func (b *backend) acmeTidyOrder(ac *acmeContext, 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)
 	if err != nil {
-		return false, fmt.Errorf("error loading order: %w", err)
+		return false, time.Time{}, fmt.Errorf("error loading order: %w", err)
 	}
 	if entry == nil {
-		return false, fmt.Errorf("order does not exist: %w", ErrMalformed)
+		return false, time.Time{}, fmt.Errorf("order does not exist: %w", ErrMalformed)
 	}
 	var order acmeOrder
 	err = entry.DecodeJSON(&order)
 	if err != nil {
-		return false, fmt.Errorf("error decoding order: %w", err)
+		return false, time.Time{}, fmt.Errorf("error decoding order: %w", err)
 	}
 
 	// Determine whether we should tidy this order
 	shouldTidy := false
+
+	// Track either the order expiry or certificate expiry to return to the caller, this
+	// can be used to influence the account's expiry
+	orderExpiry := order.CertificateExpiry
+
 	// It is faster to check certificate information on the order entry rather than fetch the cert entry to parse:
 	if !order.CertificateExpiry.IsZero() {
 		// This implies that a certificate exists
@@ -917,9 +923,10 @@ func (b *backend) acmeTidyOrder(ac *acmeContext, accountId string, orderPath str
 		if time.Now().After(order.Expires) {
 			shouldTidy = true
 		}
+		orderExpiry = order.Expires
 	}
 	if shouldTidy == false {
-		return shouldTidy, nil
+		return shouldTidy, orderExpiry, nil
 	}
 
 	// Tidy this Order
@@ -930,18 +937,22 @@ func (b *backend) acmeTidyOrder(ac *acmeContext, accountId string, orderPath str
 	for _, authorizationId := range order.AuthorizationIds {
 		err = ac.sc.Storage.Delete(ac.sc.Context, getAuthorizationPath(accountId, authorizationId))
 		if err != nil {
-			return false, err
+			return false, orderExpiry, err
 		}
 	}
 
-	// Normal Tidy will Take Care of the Certificate
+	// 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))
+	if err != nil {
+		return false, orderExpiry, err
+	}
 
 	// And Finally, the order:
 	err = ac.sc.Storage.Delete(ac.sc.Context, orderPath)
 	if err != nil {
-		return false, err
+		return false, orderExpiry, err
 	}
 	b.tidyStatusIncDelAcmeOrderCount()
 
-	return true, nil
+	return true, orderExpiry, nil
 }

builtin/logical/pki/path_acme_revoke.go

@@ -160,7 +160,7 @@ func (b *backend) acmeRevocationByAccount(acmeCtx *acmeContext, r *logical.Reque
 	if err != nil {
 		return nil, fmt.Errorf("failed to lookup account: %w", err)
 	}
-	if account.Status != StatusValid {
+	if account.Status != AccountStatusValid {
 		return nil, fmt.Errorf("account isn't presently valid: %w", ErrUnauthorized)
 	}