From fac5c39f5f9d36798a270af2d71578334001c2cf Mon Sep 17 00:00:00 2001 From: Mortie Torabi Date: Fri, 30 Jan 2026 19:36:46 +0000 Subject: [PATCH] fix: implement parsing for Retry-After header according to RFC 7231 (#2830) Co-authored-by: Fernandez Ludovic --- acme/api/service.go | 29 ++++++++++++++++++++++ acme/api/service_test.go | 37 ++++++++++++++++++++++++++++ certificate/renewal.go | 5 ++-- certificate/renewal_test.go | 36 +++++++++++++++++++++++++++ challenge/resolver/solver_manager.go | 17 ++++++------- 5 files changed, 112 insertions(+), 12 deletions(-) diff --git a/acme/api/service.go b/acme/api/service.go index 65518e1d9..22ce05124 100644 --- a/acme/api/service.go +++ b/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) +} diff --git a/acme/api/service_test.go b/acme/api/service_test.go index 2dbd795c9..57ea45708 100644 --- a/acme/api/service_test.go +++ b/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) + }) + } +} diff --git a/certificate/renewal.go b/certificate/renewal.go index 15e804745..59d31cfb5 100644 --- a/certificate/renewal.go +++ b/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) } } diff --git a/certificate/renewal_test.go b/certificate/renewal_test.go index 6ce43e0aa..23209638a 100644 --- a/certificate/renewal_test.go +++ b/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) diff --git a/challenge/resolver/solver_manager.go b/challenge/resolver/solver_manager.go index 48d9194b9..87cf6e2d8 100644 --- a/challenge/resolver/solver_manager.go +++ b/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) - if err != nil { + retryAfter, err := api.ParseRetryAfter(chlng.RetryAfter) + 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.MaxInterval = 10 * initialInterval + bo.InitialInterval = retryAfter + 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) {