fix: deduplicate authz for DNS01 challenge (#2828)

challenge/resolver/prober.go

@@ -98,11 +98,24 @@ func (p *Prober) Solve(authorizations []acme.Authorization) error {
 }
 
 func sequentialSolve(authSolvers []*selectedAuthSolver, failures obtainError) {
+	// Some CA are using the same token,
+	// this can be a problem with the DNS01 challenge when the DNS provider doesn't support duplicate TXT records.
+	// In the sequential mode, this is not a problem because we can solve the challenges in order.
+	// But it can reduce the number of call the DNS provider APIs.
+	uniq := make(map[string]struct{})
+
 	for i, authSolver := range authSolvers {
 		// Submit the challenge
 		domain := challenge.GetTargetedDomain(authSolver.authz)
 
+		chlg, _ := challenge.FindChallenge(challenge.DNS01, authSolver.authz)
+
 		if solvr, ok := authSolver.solver.(preSolver); ok {
+			if _, ok := uniq[authSolver.authz.Identifier.Value+chlg.Token]; ok && chlg.Token != "" {
+				log.Infof("acme: duplicate token for %q (DNS-01); skipping pre-solve.", authSolver.authz.Identifier.Value)
+				continue
+			}
+
 			err := solvr.PreSolve(authSolver.authz)
 			if err != nil {
 				failures[domain] = err
@@ -111,6 +124,8 @@ func sequentialSolve(authSolvers []*selectedAuthSolver, failures obtainError) {
 
 				continue
 			}
+
+			uniq[authSolver.authz.Identifier.Value+chlg.Token] = struct{}{}
 		}
 
 		// Solve challenge
@@ -123,22 +138,43 @@ func sequentialSolve(authSolvers []*selectedAuthSolver, failures obtainError) {
 			continue
 		}
 
-		// Clean challenge
-		cleanUp(authSolver.solver, authSolver.authz)
+		if _, ok := uniq[authSolver.authz.Identifier.Value+chlg.Token]; ok || chlg.Token == "" {
+			// Clean challenge
+			cleanUp(authSolver.solver, authSolver.authz)
 
-		if len(authSolvers)-1 > i {
-			solvr := authSolver.solver.(sequential)
-			_, interval := solvr.Sequential()
-			log.Infof("sequence: wait for %s", interval)
-			time.Sleep(interval)
+			if len(authSolvers)-1 > i {
+				solvr := authSolver.solver.(sequential)
+				_, interval := solvr.Sequential()
+				log.Infof("sequence: wait for %s", interval)
+				time.Sleep(interval)
+			}
+
+			delete(uniq, authSolver.authz.Identifier.Value+chlg.Token)
+		} else {
+			log.Infof("acme: duplicate token for %q (DNS-01); skipping cleanup.", authSolver.authz.Identifier.Value)
 		}
 	}
 }
 
 func parallelSolve(authSolvers []*selectedAuthSolver, failures obtainError) {
+	// Some CA are using the same token,
+	// this can be a problem with the DNS01 challenge when the DNS provider doesn't support duplicate TXT records.
+	uniq := make(map[string]struct{})
+
 	// For all valid preSolvers, first submit the challenges, so they have max time to propagate
 	for _, authSolver := range authSolvers {
 		authz := authSolver.authz
+
+		chlg, err := challenge.FindChallenge(challenge.DNS01, authz)
+		if err == nil {
+			if _, ok := uniq[authz.Identifier.Value+chlg.Token]; ok {
+				log.Infof("acme: duplicate token for %q (DNS-01); skipping pre-solve.", authSolver.authz.Identifier.Value)
+				continue
+			}
+
+			uniq[authz.Identifier.Value+chlg.Token] = struct{}{}
+		}
+
 		if solvr, ok := authSolver.solver.(preSolver); ok {
 			err := solvr.PreSolve(authz)
 			if err != nil {
@@ -150,6 +186,16 @@ func parallelSolve(authSolvers []*selectedAuthSolver, failures obtainError) {
 	defer func() {
 		// Clean all created TXT records
 		for _, authSolver := range authSolvers {
+			chlg, err := challenge.FindChallenge(challenge.DNS01, authSolver.authz)
+			if err == nil {
+				if _, ok := uniq[authSolver.authz.Identifier.Value+chlg.Token]; ok {
+					delete(uniq, authSolver.authz.Identifier.Value+chlg.Token)
+				} else {
+					log.Infof("acme: duplicate token for %q (DNS-01); skipping cleanup.", authSolver.authz.Identifier.Value)
+					continue
+				}
+			}
+
 			cleanUp(authSolver.solver, authSolver.authz)
 		}
 	}()

challenge/resolver/prober_mock_test.go

@@ -1,6 +1,7 @@
 package resolver
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/go-acme/lego/v4/acme"
@@ -11,34 +12,68 @@ type preSolverMock struct {
 	preSolve map[string]error
 	solve    map[string]error
 	cleanUp  map[string]error
+
+	preSolveCounter int
+	solveCounter    int
+	cleanUpCounter  int
 }
 
 func (s *preSolverMock) PreSolve(authorization acme.Authorization) error {
+	s.preSolveCounter++
+
 	return s.preSolve[authorization.Identifier.Value]
 }
 
 func (s *preSolverMock) Solve(authorization acme.Authorization) error {
+	s.solveCounter++
+
 	return s.solve[authorization.Identifier.Value]
 }
 
 func (s *preSolverMock) CleanUp(authorization acme.Authorization) error {
+	s.cleanUpCounter++
+
 	return s.cleanUp[authorization.Identifier.Value]
 }
 
+func (s *preSolverMock) String() string {
+	return fmt.Sprintf("PreSolve: %d, Solve: %d, CleanUp: %d", s.preSolveCounter, s.solveCounter, s.cleanUpCounter)
+}
+
 func createStubAuthorizationHTTP01(domain, status string) acme.Authorization {
+	return createStubAuthorization(domain, status, false, acme.Challenge{
+		Type:      challenge.HTTP01.String(),
+		Validated: time.Now(),
+	})
+}
+
+func createStubAuthorizationDNS01(domain string, wildcard bool) acme.Authorization {
+	var chlgs []acme.Challenge
+
+	if wildcard {
+		chlgs = append(chlgs, acme.Challenge{
+			Type:      challenge.HTTP01.String(),
+			Validated: time.Now(),
+		})
+	}
+
+	chlgs = append(chlgs, acme.Challenge{
+		Type:      challenge.DNS01.String(),
+		Validated: time.Now(),
+	})
+
+	return createStubAuthorization(domain, acme.StatusProcessing, wildcard, chlgs...)
+}
+
+func createStubAuthorization(domain, status string, wildcard bool, chlgs ...acme.Challenge) acme.Authorization {
 	return acme.Authorization{
-		Status:  status,
-		Expires: time.Now(),
+		Wildcard: wildcard,
+		Status:   status,
+		Expires:  time.Now(),
 		Identifier: acme.Identifier{
-			Type:  challenge.HTTP01.String(),
+			Type:  "dns",
 			Value: domain,
 		},
-		Challenges: []acme.Challenge{
-			{
-				Type:      challenge.HTTP01.String(),
-				Validated: time.Now(),
-				Error:     nil,
-			},
-		},
+		Challenges: chlgs,
 	}
 }

challenge/resolver/prober_test.go

@@ -2,19 +2,22 @@ package resolver
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/go-acme/lego/v4/acme"
 	"github.com/go-acme/lego/v4/challenge"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestProber_Solve(t *testing.T) {
 	testCases := []struct {
-		desc          string
-		solvers       map[challenge.Type]solver
-		authz         []acme.Authorization
-		expectedError string
+		desc             string
+		solvers          map[challenge.Type]solver
+		authz            []acme.Authorization
+		expectedError    string
+		expectedCounters map[challenge.Type]string
 	}{
 		{
 			desc: "success",
@@ -30,6 +33,30 @@ func TestProber_Solve(t *testing.T) {
 				createStubAuthorizationHTTP01("example.org", acme.StatusProcessing),
 				createStubAuthorizationHTTP01("example.net", acme.StatusProcessing),
 			},
+			expectedCounters: map[challenge.Type]string{
+				challenge.HTTP01: "PreSolve: 3, Solve: 3, CleanUp: 3",
+			},
+		},
+		{
+			desc: "DNS-01 deduplicate",
+			solvers: map[challenge.Type]solver{
+				challenge.DNS01: &preSolverMock{
+					preSolve: map[string]error{},
+					solve:    map[string]error{},
+					cleanUp:  map[string]error{},
+				},
+			},
+			authz: []acme.Authorization{
+				createStubAuthorizationDNS01("a.example", false),
+				createStubAuthorizationDNS01("a.example", true),
+				createStubAuthorizationDNS01("b.example", false),
+				createStubAuthorizationDNS01("b.example", true),
+				createStubAuthorizationDNS01("c.example", true),
+				createStubAuthorizationDNS01("d.example", false),
+			},
+			expectedCounters: map[challenge.Type]string{
+				challenge.DNS01: "PreSolve: 4, Solve: 6, CleanUp: 4",
+			},
 		},
 		{
 			desc: "already valid",
@@ -45,6 +72,9 @@ func TestProber_Solve(t *testing.T) {
 				createStubAuthorizationHTTP01("example.org", acme.StatusValid),
 				createStubAuthorizationHTTP01("example.net", acme.StatusValid),
 			},
+			expectedCounters: map[challenge.Type]string{
+				challenge.HTTP01: "PreSolve: 0, Solve: 0, CleanUp: 0",
+			},
 		},
 		{
 			desc: "when preSolve fail, auth is flagged as error and skipped",
@@ -69,6 +99,9 @@ func TestProber_Solve(t *testing.T) {
 			expectedError: `error: one or more domains had a problem:
 [example.com] preSolve error example.com
 `,
+			expectedCounters: map[challenge.Type]string{
+				challenge.HTTP01: "PreSolve: 3, Solve: 2, CleanUp: 3",
+			},
 		},
 		{
 			desc: "errors at different stages",
@@ -95,6 +128,9 @@ func TestProber_Solve(t *testing.T) {
 [example.com] preSolve error example.com
 [example.org] solve error example.org
 `,
+			expectedCounters: map[challenge.Type]string{
+				challenge.HTTP01: "PreSolve: 3, Solve: 2, CleanUp: 3",
+			},
 		},
 	}
 
@@ -112,6 +148,10 @@ func TestProber_Solve(t *testing.T) {
 			} else {
 				require.NoError(t, err)
 			}
+
+			for n, s := range test.solvers {
+				assert.Equal(t, test.expectedCounters[n], fmt.Sprintf("%s", s))
+			}
 		})
 	}
 }