Sync ACME account registration (#293)

https://caddy.community/t/lets-encrypt-hits-rate-limit-too-many-registrations-for-this-ip/24343

account.go

@@ -415,6 +415,15 @@ func (am *ACMEIssuer) mostRecentAccountEmail(ctx context.Context, caURL string)
 	return getPrimaryContact(account), true
 }
 
+func accountRegLockKey(acc acme.Account) string {
+	key := "register_acme_account"
+	if len(acc.Contact) == 0 {
+		return key
+	}
+	key += "_" + getPrimaryContact(acc)
+	return key
+}
+
 // getPrimaryContact returns the first contact on the account (if any)
 // without the scheme. (I guess we assume an email address.)
 func getPrimaryContact(account acme.Account) string {

acmeclient.go

@@ -50,16 +50,28 @@ func (iss *ACMEIssuer) newACMEClientWithAccount(ctx context.Context, useTestCA,
 		return nil, err
 	}
 
-	// look up or create the ACME account
+	// we try loading the account from storage before a potential
-	var account acme.Account
+	// lock, and 0after obtaining the lock as well, to ensure we don't
-	if iss.AccountKeyPEM != "" {
+	// repeat work done by another instance or goroutine
-		iss.Logger.Info("using configured ACME account")
+	getAccount := func() (acme.Account, error) {
-		account, err = iss.GetAccount(ctx, []byte(iss.AccountKeyPEM))
+		// look up or create the ACME account
-	} else {
+		var account acme.Account
-		account, err = iss.getAccount(ctx, client.Directory, iss.getEmail())
+		if iss.AccountKeyPEM != "" {
+			iss.Logger.Info("using configured ACME account")
+			account, err = iss.GetAccount(ctx, []byte(iss.AccountKeyPEM))
+		} else {
+			account, err = iss.getAccount(ctx, client.Directory, iss.getEmail())
+		}
+		if err != nil {
+			return acme.Account{}, fmt.Errorf("getting ACME account: %v", err)
+		}
+		return account, nil
 	}
+
+	// first try getting the account
+	account, err := getAccount()
 	if err != nil {
-		return nil, fmt.Errorf("getting ACME account: %v", err)
+		return nil, err
 	}
 
 	// register account if it is new
@@ -68,67 +80,93 @@ func (iss *ACMEIssuer) newACMEClientWithAccount(ctx context.Context, useTestCA,
 			zap.Strings("contact", account.Contact),
 			zap.String("location", account.Location))
 
-		if iss.NewAccountFunc != nil {
+		// synchronize this so the account is only created once
-			// obtain lock here, since NewAccountFunc calls happen concurrently and they typically read and change the issuer
+		acctLockKey := accountRegLockKey(account)
-			iss.mu.Lock()
+		err = iss.config.Storage.Lock(ctx, acctLockKey)
-			account, err = iss.NewAccountFunc(ctx, iss, account)
+		if err != nil {
-			iss.mu.Unlock()
+			return nil, fmt.Errorf("locking account registration: %v", err)
-			if err != nil {
+		}
-				return nil, fmt.Errorf("account pre-registration callback: %v", err)
+		defer func() {
+			if err := iss.config.Storage.Unlock(ctx, acctLockKey); err != nil {
+				iss.Logger.Error("failed to unlock account registration lock", zap.Error(err))
 			}
+		}()
+
+		// if we're not the only one waiting for this account, then by this point it should already be registered and in storage; reload it
+		account, err = getAccount()
+		if err != nil {
+			return nil, err
 		}
 
-		// agree to terms
+		// if we are the only or first one waiting for this account, then proceed to register it while we have the lock
-		if interactive {
+		if account.Status == "" {
-			if !iss.isAgreed() {
+			if iss.NewAccountFunc != nil {
-				var termsURL string
+				// obtain lock here, since NewAccountFunc calls happen concurrently and they typically read and change the issuer
-				dir, err := client.GetDirectory(ctx)
+				iss.mu.Lock()
+				account, err = iss.NewAccountFunc(ctx, iss, account)
+				iss.mu.Unlock()
 				if err != nil {
-					return nil, fmt.Errorf("getting directory: %w", err)
+					return nil, fmt.Errorf("account pre-registration callback: %v", err)
 				}
-				if dir.Meta != nil {
+			}
-					termsURL = dir.Meta.TermsOfService
+
-				}
+			// agree to terms
-				if termsURL != "" {
+			if interactive {
-					agreed := iss.askUserAgreement(termsURL)
+				if !iss.isAgreed() {
-					if !agreed {
+					var termsURL string
-						return nil, fmt.Errorf("user must agree to CA terms")
+					dir, err := client.GetDirectory(ctx)
+					if err != nil {
+						return nil, fmt.Errorf("getting directory: %w", err)
+					}
+					if dir.Meta != nil {
+						termsURL = dir.Meta.TermsOfService
+					}
+					if termsURL != "" {
+						agreed := iss.askUserAgreement(termsURL)
+						if !agreed {
+							return nil, fmt.Errorf("user must agree to CA terms")
+						}
+						iss.mu.Lock()
+						iss.agreed = agreed
+						iss.mu.Unlock()
 					}
-					iss.mu.Lock()
-					iss.agreed = agreed
-					iss.mu.Unlock()
 				}
+			} else {
+				// can't prompt a user who isn't there; they should
+				// have reviewed the terms beforehand
+				iss.mu.Lock()
+				iss.agreed = true
+				iss.mu.Unlock()
+			}
+			account.TermsOfServiceAgreed = iss.isAgreed()
+
+			// associate account with external binding, if configured
+			if iss.ExternalAccount != nil {
+				err := account.SetExternalAccountBinding(ctx, client.Client, *iss.ExternalAccount)
+				if err != nil {
+					return nil, err
+				}
+			}
+
+			// create account
+			account, err = client.NewAccount(ctx, account)
+			if err != nil {
+				return nil, fmt.Errorf("registering account %v with server: %w", account.Contact, err)
+			}
+			iss.Logger.Info("new ACME account registered",
+				zap.Strings("contact", account.Contact),
+				zap.String("status", account.Status))
+
+			// persist the account to storage
+			err = iss.saveAccount(ctx, client.Directory, account)
+			if err != nil {
+				return nil, fmt.Errorf("could not save account %v: %v", account.Contact, err)
 			}
 		} else {
-			// can't prompt a user who isn't there; they should
+			iss.Logger.Info("account has already been registered; reloaded",
-			// have reviewed the terms beforehand
+				zap.Strings("contact", account.Contact),
-			iss.mu.Lock()
+				zap.String("status", account.Status),
-			iss.agreed = true
+				zap.String("location", account.Location))
-			iss.mu.Unlock()
-		}
-		account.TermsOfServiceAgreed = iss.isAgreed()
-
-		// associate account with external binding, if configured
-		if iss.ExternalAccount != nil {
-			err := account.SetExternalAccountBinding(ctx, client.Client, *iss.ExternalAccount)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		// create account
-		account, err = client.NewAccount(ctx, account)
-		if err != nil {
-			return nil, fmt.Errorf("registering account %v with server: %w", account.Contact, err)
-		}
-		iss.Logger.Info("new ACME account registered",
-			zap.Strings("contact", account.Contact),
-			zap.String("status", account.Status))
-
-		// persist the account to storage
-		err = iss.saveAccount(ctx, client.Directory, account)
-		if err != nil {
-			return nil, fmt.Errorf("could not save account %v: %v", account.Contact, err)
 		}
 	}