fix: implement parsing for Retry-After header according to RFC 7231 (#2830)

Co-authored-by: Fernandez Ludovic <ldez@users.noreply.github.com>

acme/api/service.go

@@ -1,8 +1,11 @@
 package api
 
 import (
+	"fmt"
 	"net/http"
 	"regexp"
+	"strconv"
+	"time"
 )
 
 type service struct {
@@ -56,3 +59,29 @@ func getRetryAfter(resp *http.Response) string {
 
 	return resp.Header.Get("Retry-After")
 }
+
+// ParseRetryAfter parses the Retry-After header value according to RFC 7231.
+// The header can be either delay-seconds (numeric) or HTTP-date (RFC 1123 format).
+// https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.3
+// Returns the duration until the retry time.
+// TODO(ldez): unexposed this function in v5.
+func ParseRetryAfter(value string) (time.Duration, error) {
+	if value == "" {
+		return 0, nil
+	}
+
+	if seconds, err := strconv.ParseInt(value, 10, 64); err == nil {
+		return time.Duration(seconds) * time.Second, nil
+	}
+
+	if retryTime, err := time.Parse(time.RFC1123, value); err == nil {
+		duration := time.Until(retryTime)
+		if duration < 0 {
+			return 0, nil
+		}
+
+		return duration, nil
+	}
+
+	return 0, fmt.Errorf("invalid Retry-After value: %q", value)
+}

acme/api/service_test.go

@@ -3,8 +3,10 @@ package api
 import (
 	"net/http"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_getLink(t *testing.T) {
@@ -53,3 +55,38 @@ func Test_getLink(t *testing.T) {
 		})
 	}
 }
+
+func TestParseRetryAfter(t *testing.T) {
+	testCases := []struct {
+		desc     string
+		value    string
+		expected time.Duration
+	}{
+		{
+			desc:     "empty header value",
+			value:    "",
+			expected: time.Duration(0),
+		},
+		{
+			desc:     "delay-seconds",
+			value:    "123",
+			expected: 123 * time.Second,
+		},
+		{
+			desc:     "HTTP-date",
+			value:    time.Now().Add(3 * time.Second).Format(time.RFC1123),
+			expected: 3 * time.Second,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+
+			rt, err := ParseRetryAfter(test.value)
+			require.NoError(t, err)
+
+			assert.InDelta(t, test.expected.Seconds(), rt.Seconds(), 1)
+		})
+	}
+}

certificate/renewal.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/go-acme/lego/v4/acme"
+	"github.com/go-acme/lego/v4/acme/api"
 )
 
 // RenewalInfoRequest contains the necessary renewal information.
@@ -92,9 +93,9 @@ func (c *Certifier) GetRenewalInfo(req RenewalInfoRequest) (*RenewalInfoResponse
 	}
 
 	if retry := resp.Header.Get("Retry-After"); retry != "" {
-		info.RetryAfter, err = time.ParseDuration(retry + "s")
+		info.RetryAfter, err = api.ParseRetryAfter(retry)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to parse Retry-After header: %w", err)
 		}
 	}
 

certificate/renewal_test.go

@@ -74,6 +74,42 @@ func TestCertifier_GetRenewalInfo(t *testing.T) {
 	assert.Equal(t, time.Duration(21600000000000), ri.RetryAfter)
 }
 
+func TestCertifier_GetRenewalInfo_retryAfter(t *testing.T) {
+	leaf, err := certcrypto.ParsePEMCertificate([]byte(ariLeafPEM))
+	require.NoError(t, err)
+
+	server := tester.MockACMEServer().
+		Route("GET /renewalInfo/"+ariLeafCertID,
+			servermock.RawStringResponse(`{
+				"suggestedWindow": {
+					"start": "2020-03-17T17:51:09Z",
+					"end": "2020-03-17T18:21:09Z"
+				},
+				"explanationUrl": "https://aricapable.ca.example/docs/renewal-advice/"
+			}
+		}`).
+				WithHeader("Content-Type", "application/json").
+				WithHeader("Retry-After", time.Now().UTC().Add(6*time.Hour).Format(time.RFC1123))).
+		BuildHTTPS(t)
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err, "Could not generate test key")
+
+	core, err := api.New(server.Client(), "lego-test", server.URL+"/dir", "", key)
+	require.NoError(t, err)
+
+	certifier := NewCertifier(core, &resolverMock{}, CertifierOptions{KeyType: certcrypto.RSA2048})
+
+	ri, err := certifier.GetRenewalInfo(RenewalInfoRequest{leaf})
+	require.NoError(t, err)
+	require.NotNil(t, ri)
+	assert.Equal(t, "2020-03-17T17:51:09Z", ri.SuggestedWindow.Start.Format(time.RFC3339))
+	assert.Equal(t, "2020-03-17T18:21:09Z", ri.SuggestedWindow.End.Format(time.RFC3339))
+	assert.Equal(t, "https://aricapable.ca.example/docs/renewal-advice/", ri.ExplanationURL)
+
+	assert.InDelta(t, 6, ri.RetryAfter.Hours(), 0.001)
+}
+
 func TestCertifier_GetRenewalInfo_errors(t *testing.T) {
 	leaf, err := certcrypto.ParsePEMCertificate([]byte(ariLeafPEM))
 	require.NoError(t, err)

challenge/resolver/solver_manager.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"sort"
-	"strconv"
 	"time"
 
 	"github.com/cenkalti/backoff/v5"
@@ -94,22 +93,20 @@ func validate(core *api.Core, domain string, chlg acme.Challenge) error {
 		return nil
 	}
 
-	ra, err := strconv.Atoi(chlng.RetryAfter)
+	retryAfter, err := api.ParseRetryAfter(chlng.RetryAfter)
-	if err != nil {
+	if err != nil || retryAfter == 0 {
 		// The ACME server MUST return a Retry-After.
-		// If it doesn't, we'll just poll hard.
+		// If it doesn't, or if it's invalid, we'll just poll hard.
 		// Boulder does not implement the ability to retry challenges or the Retry-After header.
 		// https://github.com/letsencrypt/boulder/blob/master/docs/acme-divergences.md#section-82
-		ra = 5
+		retryAfter = 5 * time.Second
 	}
 
-	initialInterval := time.Duration(ra) * time.Second
-
 	ctx := context.Background()
 
 	bo := backoff.NewExponentialBackOff()
-	bo.InitialInterval = initialInterval
+	bo.InitialInterval = retryAfter
-	bo.MaxInterval = 10 * initialInterval
+	bo.MaxInterval = 10 * retryAfter
 
 	// After the path is sent, the ACME server will access our server.
 	// Repeatedly check the server for an updated status on our request.
@@ -134,7 +131,7 @@ func validate(core *api.Core, domain string, chlg acme.Challenge) error {
 
 	return wait.Retry(ctx, operation,
 		backoff.WithBackOff(bo),
-		backoff.WithMaxElapsedTime(100*initialInterval))
+		backoff.WithMaxElapsedTime(100*retryAfter))
 }
 
 func checkChallengeStatus(chlng acme.ExtendedChallenge) (bool, error) {