acme: clarify retries and backoff algorithm

There's been some confusion about failed request retries.
Rightfully so: some requests are retried, others are not.

This change attempts to clarify the issue and unify backoff
usage in all Client's methods by introducing a new exported
optional field RetryBackoff and adding retry logic where missing.

Also, updates golang/go#22457.

Change-Id: Ied434edf998d52925a48b6b3b2407d45a6e9d2ee
Reviewed-on: https://go-review.googlesource.com/109615
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

acme/acme.go

@@ -14,7 +14,6 @@
 package acme
 
 import (
-	"bytes"
 	"context"
 	"crypto"
 	"crypto/ecdsa"
@@ -33,7 +32,6 @@ import (
 	"io/ioutil"
 	"math/big"
 	"net/http"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -76,6 +74,22 @@ type Client struct {
 	// will have no effect.
 	DirectoryURL string
 
+	// RetryBackoff computes the duration after which the nth retry of a failed request
+	// should occur. The value of n for the first call on failure is 1.
+	// The values of r and resp are the request and response of the last failed attempt.
+	// If the returned value is negative or zero, no more retries are done and an error
+	// is returned to the caller of the original method.
+	//
+	// Requests which result in a 4xx client error are not retried,
+	// except for 400 Bad Request due to "bad nonce" errors and 429 Too Many Requests.
+	//
+	// If RetryBackoff is nil, a truncated exponential backoff algorithm
+	// with the ceiling of 10 seconds is used, where each subsequent retry n
+	// is done after either ("Retry-After" + jitter) or (2^n seconds + jitter),
+	// preferring the former if "Retry-After" header is found in the resp.
+	// The jitter is a random value up to 1 second.
+	RetryBackoff func(n int, r *http.Request, resp *http.Response) time.Duration
+
 	dirMu sync.Mutex // guards writes to dir
 	dir   *Directory // cached result of Client's Discover method
 
@@ -99,15 +113,12 @@ func (c *Client) Discover(ctx context.Context) (Directory, error) {
 	if dirURL == "" {
 		dirURL = LetsEncryptURL
 	}
-	res, err := c.get(ctx, dirURL)
+	res, err := c.get(ctx, dirURL, wantStatus(http.StatusOK))
 	if err != nil {
 		return Directory{}, err
 	}
 	defer res.Body.Close()
 	c.addNonce(res.Header)
-	if res.StatusCode != http.StatusOK {
-		return Directory{}, responseError(res)
-	}
 
 	var v struct {
 		Reg    string `json:"new-reg"`
@@ -166,14 +177,11 @@ func (c *Client) CreateCert(ctx context.Context, csr []byte, exp time.Duration,
 		req.NotAfter = now.Add(exp).Format(time.RFC3339)
 	}
 
-	res, err := c.retryPostJWS(ctx, c.Key, c.dir.CertURL, req)
+	res, err := c.post(ctx, c.Key, c.dir.CertURL, req, wantStatus(http.StatusCreated))
 	if err != nil {
 		return nil, "", err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusCreated {
-		return nil, "", responseError(res)
-	}
 
 	curl := res.Header.Get("Location") // cert permanent URL
 	if res.ContentLength == 0 {
@@ -196,26 +204,11 @@ func (c *Client) CreateCert(ctx context.Context, csr []byte, exp time.Duration,
 // Callers are encouraged to parse the returned value to ensure the certificate is valid
 // and has expected features.
 func (c *Client) FetchCert(ctx context.Context, url string, bundle bool) ([][]byte, error) {
-	for {
-		res, err := c.get(ctx, url)
-		if err != nil {
-			return nil, err
-		}
-		defer res.Body.Close()
-		if res.StatusCode == http.StatusOK {
-			return c.responseCert(ctx, res, bundle)
-		}
-		if res.StatusCode > 299 {
-			return nil, responseError(res)
-		}
-		d := retryAfter(res.Header.Get("Retry-After"), 3*time.Second)
-		select {
-		case <-time.After(d):
-			// retry
-		case <-ctx.Done():
-			return nil, ctx.Err()
-		}
+	res, err := c.get(ctx, url, wantStatus(http.StatusOK))
+	if err != nil {
+		return nil, err
 	}
+	return c.responseCert(ctx, res, bundle)
 }
 
 // RevokeCert revokes a previously issued certificate cert, provided in DER format.
@@ -241,14 +234,11 @@ func (c *Client) RevokeCert(ctx context.Context, key crypto.Signer, cert []byte,
 	if key == nil {
 		key = c.Key
 	}
-	res, err := c.retryPostJWS(ctx, key, c.dir.RevokeURL, body)
+	res, err := c.post(ctx, key, c.dir.RevokeURL, body, wantStatus(http.StatusOK))
 	if err != nil {
 		return err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK {
-		return responseError(res)
-	}
 	return nil
 }
 
@@ -329,14 +319,11 @@ func (c *Client) Authorize(ctx context.Context, domain string) (*Authorization,
 		Resource:   "new-authz",
 		Identifier: authzID{Type: "dns", Value: domain},
 	}
-	res, err := c.retryPostJWS(ctx, c.Key, c.dir.AuthzURL, req)
+	res, err := c.post(ctx, c.Key, c.dir.AuthzURL, req, wantStatus(http.StatusCreated))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusCreated {
-		return nil, responseError(res)
-	}
 
 	var v wireAuthz
 	if err := json.NewDecoder(res.Body).Decode(&v); err != nil {
@@ -353,14 +340,11 @@ func (c *Client) Authorize(ctx context.Context, domain string) (*Authorization,
 // If a caller needs to poll an authorization until its status is final,
 // see the WaitAuthorization method.
 func (c *Client) GetAuthorization(ctx context.Context, url string) (*Authorization, error) {
-	res, err := c.get(ctx, url)
+	res, err := c.get(ctx, url, wantStatus(http.StatusOK, http.StatusAccepted))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK && res.StatusCode != http.StatusAccepted {
-		return nil, responseError(res)
-	}
 	var v wireAuthz
 	if err := json.NewDecoder(res.Body).Decode(&v); err != nil {
 		return nil, fmt.Errorf("acme: invalid response: %v", err)
@@ -387,14 +371,11 @@ func (c *Client) RevokeAuthorization(ctx context.Context, url string) error {
 		Status:   "deactivated",
 		Delete:   true,
 	}
-	res, err := c.retryPostJWS(ctx, c.Key, url, req)
+	res, err := c.post(ctx, c.Key, url, req, wantStatus(http.StatusOK))
 	if err != nil {
 		return err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK {
-		return responseError(res)
-	}
 	return nil
 }
 
@@ -406,44 +387,42 @@ func (c *Client) RevokeAuthorization(ctx context.Context, url string) error {
 // In all other cases WaitAuthorization returns an error.
 // If the Status is StatusInvalid, the returned error is of type *AuthorizationError.
 func (c *Client) WaitAuthorization(ctx context.Context, url string) (*Authorization, error) {
-	sleep := sleeper(ctx)
 	for {
-		res, err := c.get(ctx, url)
+		res, err := c.get(ctx, url, wantStatus(http.StatusOK, http.StatusAccepted))
 		if err != nil {
 			return nil, err
 		}
-		if res.StatusCode >= 400 && res.StatusCode <= 499 {
-			// Non-retriable error. For instance, Let's Encrypt may return 404 Not Found
-			// when requesting an expired authorization.
-			defer res.Body.Close()
-			return nil, responseError(res)
-		}
 
-		retry := res.Header.Get("Retry-After")
-		if res.StatusCode != http.StatusOK && res.StatusCode != http.StatusAccepted {
-			res.Body.Close()
-			if err := sleep(retry, 1); err != nil {
-				return nil, err
-			}
-			continue
-		}
 		var raw wireAuthz
 		err = json.NewDecoder(res.Body).Decode(&raw)
 		res.Body.Close()
-		if err != nil {
-			if err := sleep(retry, 0); err != nil {
-				return nil, err
-			}
-			continue
-		}
-		if raw.Status == StatusValid {
+		switch {
+		case err != nil:
+			// Skip and retry.
+		case raw.Status == StatusValid:
 			return raw.authorization(url), nil
-		}
-		if raw.Status == StatusInvalid {
+		case raw.Status == StatusInvalid:
 			return nil, raw.error(url)
 		}
-		if err := sleep(retry, 0); err != nil {
-			return nil, err
+
+		// Exponential backoff is implemented in c.get above.
+		// This is just to prevent continuously hitting the CA
+		// while waiting for a final authorization status.
+		d := retryAfter(res.Header.Get("Retry-After"))
+		if d == 0 {
+			// Given that the fastest challenges TLS-SNI and HTTP-01
+			// require a CA to make at least 1 network round trip
+			// and most likely persist a challenge state,
+			// this default delay seems reasonable.
+			d = time.Second
+		}
+		t := time.NewTimer(d)
+		select {
+		case <-ctx.Done():
+			t.Stop()
+			return nil, ctx.Err()
+		case <-t.C:
+			// Retry.
 		}
 	}
 }
@@ -452,14 +431,11 @@ func (c *Client) WaitAuthorization(ctx context.Context, url string) (*Authorizat
 //
 // A client typically polls a challenge status using this method.
 func (c *Client) GetChallenge(ctx context.Context, url string) (*Challenge, error) {
-	res, err := c.get(ctx, url)
+	res, err := c.get(ctx, url, wantStatus(http.StatusOK, http.StatusAccepted))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK && res.StatusCode != http.StatusAccepted {
-		return nil, responseError(res)
-	}
 	v := wireChallenge{URI: url}
 	if err := json.NewDecoder(res.Body).Decode(&v); err != nil {
 		return nil, fmt.Errorf("acme: invalid response: %v", err)
@@ -486,16 +462,14 @@ func (c *Client) Accept(ctx context.Context, chal *Challenge) (*Challenge, error
 		Type:     chal.Type,
 		Auth:     auth,
 	}
-	res, err := c.retryPostJWS(ctx, c.Key, chal.URI, req)
+	res, err := c.post(ctx, c.Key, chal.URI, req, wantStatus(
+		http.StatusOK,       // according to the spec
+		http.StatusAccepted, // Let's Encrypt: see https://goo.gl/WsJ7VT (acme-divergences.md)
+	))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	// Note: the protocol specifies 200 as the expected response code, but
-	// letsencrypt seems to be returning 202.
-	if res.StatusCode != http.StatusOK && res.StatusCode != http.StatusAccepted {
-		return nil, responseError(res)
-	}
 
 	var v wireChallenge
 	if err := json.NewDecoder(res.Body).Decode(&v); err != nil {
@@ -619,14 +593,14 @@ func (c *Client) doReg(ctx context.Context, url string, typ string, acct *Accoun
 		req.Contact = acct.Contact
 		req.Agreement = acct.AgreedTerms
 	}
-	res, err := c.retryPostJWS(ctx, c.Key, url, req)
+	res, err := c.post(ctx, c.Key, url, req, wantStatus(
+		http.StatusOK,      // updates and deletes
+		http.StatusCreated, // new account creation
+	))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode < 200 || res.StatusCode > 299 {
-		return nil, responseError(res)
-	}
 
 	var v struct {
 		Contact        []string
@@ -656,59 +630,6 @@ func (c *Client) doReg(ctx context.Context, url string, typ string, acct *Accoun
 	}, nil
 }
 
-// retryPostJWS will retry calls to postJWS if there is a badNonce error,
-// clearing the stored nonces after each error.
-// If the response was 4XX-5XX, then responseError is called on the body,
-// the body is closed, and the error returned.
-func (c *Client) retryPostJWS(ctx context.Context, key crypto.Signer, url string, body interface{}) (*http.Response, error) {
-	sleep := sleeper(ctx)
-	for {
-		res, err := c.postJWS(ctx, key, url, body)
-		if err != nil {
-			return nil, err
-		}
-		// handle errors 4XX-5XX with responseError
-		if res.StatusCode >= 400 && res.StatusCode <= 599 {
-			err := responseError(res)
-			res.Body.Close()
-			// according to spec badNonce is urn:ietf:params:acme:error:badNonce
-			// however, acme servers in the wild return their version of the error
-			// https://tools.ietf.org/html/draft-ietf-acme-acme-02#section-5.4
-			if ae, ok := err.(*Error); ok && strings.HasSuffix(strings.ToLower(ae.ProblemType), ":badnonce") {
-				// clear any nonces that we might've stored that might now be
-				// considered bad
-				c.clearNonces()
-				retry := res.Header.Get("Retry-After")
-				if err := sleep(retry, 1); err != nil {
-					return nil, err
-				}
-				continue
-			}
-			return nil, err
-		}
-		return res, nil
-	}
-}
-
-// postJWS signs the body with the given key and POSTs it to the provided url.
-// The body argument must be JSON-serializable.
-func (c *Client) postJWS(ctx context.Context, key crypto.Signer, url string, body interface{}) (*http.Response, error) {
-	nonce, err := c.popNonce(ctx, url)
-	if err != nil {
-		return nil, err
-	}
-	b, err := jwsEncodeJSON(body, key, nonce)
-	if err != nil {
-		return nil, err
-	}
-	res, err := c.post(ctx, url, "application/jose+json", bytes.NewReader(b))
-	if err != nil {
-		return nil, err
-	}
-	c.addNonce(res.Header)
-	return res, nil
-}
-
 // popNonce returns a nonce value previously stored with c.addNonce
 // or fetches a fresh one from the given URL.
 func (c *Client) popNonce(ctx context.Context, url string) (string, error) {
@@ -749,58 +670,12 @@ func (c *Client) addNonce(h http.Header) {
 	c.nonces[v] = struct{}{}
 }
 
-func (c *Client) httpClient() *http.Client {
-	if c.HTTPClient != nil {
-		return c.HTTPClient
-	}
-	return http.DefaultClient
-}
-
-func (c *Client) get(ctx context.Context, urlStr string) (*http.Response, error) {
-	req, err := http.NewRequest("GET", urlStr, nil)
-	if err != nil {
-		return nil, err
-	}
-	return c.do(ctx, req)
-}
-
-func (c *Client) head(ctx context.Context, urlStr string) (*http.Response, error) {
-	req, err := http.NewRequest("HEAD", urlStr, nil)
-	if err != nil {
-		return nil, err
-	}
-	return c.do(ctx, req)
-}
-
-func (c *Client) post(ctx context.Context, urlStr, contentType string, body io.Reader) (*http.Response, error) {
-	req, err := http.NewRequest("POST", urlStr, body)
-	if err != nil {
-		return nil, err
-	}
-	req.Header.Set("Content-Type", contentType)
-	return c.do(ctx, req)
-}
-
-func (c *Client) do(ctx context.Context, req *http.Request) (*http.Response, error) {
-	res, err := c.httpClient().Do(req.WithContext(ctx))
-	if err != nil {
-		select {
-		case <-ctx.Done():
-			// Prefer the unadorned context error.
-			// (The acme package had tests assuming this, previously from ctxhttp's
-			// behavior, predating net/http supporting contexts natively)
-			// TODO(bradfitz): reconsider this in the future. But for now this
-			// requires no test updates.
-			return nil, ctx.Err()
-		default:
-			return nil, err
-		}
-	}
-	return res, nil
-}
-
 func (c *Client) fetchNonce(ctx context.Context, url string) (string, error) {
-	resp, err := c.head(ctx, url)
+	r, err := http.NewRequest("HEAD", url, nil)
+	if err != nil {
+		return "", err
+	}
+	resp, err := c.doNoRetry(ctx, r)
 	if err != nil {
 		return "", err
 	}
@@ -852,24 +727,6 @@ func (c *Client) responseCert(ctx context.Context, res *http.Response, bundle bo
 	return cert, nil
 }
 
-// responseError creates an error of Error type from resp.
-func responseError(resp *http.Response) error {
-	// don't care if ReadAll returns an error:
-	// json.Unmarshal will fail in that case anyway
-	b, _ := ioutil.ReadAll(resp.Body)
-	e := &wireError{Status: resp.StatusCode}
-	if err := json.Unmarshal(b, e); err != nil {
-		// this is not a regular error response:
-		// populate detail with anything we received,
-		// e.Status will already contain HTTP response code value
-		e.Detail = string(b)
-		if e.Detail == "" {
-			e.Detail = resp.Status
-		}
-	}
-	return e.error(resp.Header)
-}
-
 // chainCert fetches CA certificate chain recursively by following "up" links.
 // Each recursive call increments the depth by 1, resulting in an error
 // if the recursion level reaches maxChainLen.
@@ -880,14 +737,11 @@ func (c *Client) chainCert(ctx context.Context, url string, depth int) ([][]byte
 		return nil, errors.New("acme: certificate chain is too deep")
 	}
 
-	res, err := c.get(ctx, url)
+	res, err := c.get(ctx, url, wantStatus(http.StatusOK))
 	if err != nil {
 		return nil, err
 	}
 	defer res.Body.Close()
-	if res.StatusCode != http.StatusOK {
-		return nil, responseError(res)
-	}
 	b, err := ioutil.ReadAll(io.LimitReader(res.Body, maxCertSize+1))
 	if err != nil {
 		return nil, err
@@ -932,65 +786,6 @@ func linkHeader(h http.Header, rel string) []string {
 	return links
 }
 
-// sleeper returns a function that accepts the Retry-After HTTP header value
-// and an increment that's used with backoff to increasingly sleep on
-// consecutive calls until the context is done. If the Retry-After header
-// cannot be parsed, then backoff is used with a maximum sleep time of 10
-// seconds.
-func sleeper(ctx context.Context) func(ra string, inc int) error {
-	var count int
-	return func(ra string, inc int) error {
-		count += inc
-		d := backoff(count, 10*time.Second)
-		d = retryAfter(ra, d)
-		wakeup := time.NewTimer(d)
-		defer wakeup.Stop()
-		select {
-		case <-ctx.Done():
-			return ctx.Err()
-		case <-wakeup.C:
-			return nil
-		}
-	}
-}
-
-// retryAfter parses a Retry-After HTTP header value,
-// trying to convert v into an int (seconds) or use http.ParseTime otherwise.
-// It returns d if v cannot be parsed.
-func retryAfter(v string, d time.Duration) time.Duration {
-	if i, err := strconv.Atoi(v); err == nil {
-		return time.Duration(i) * time.Second
-	}
-	t, err := http.ParseTime(v)
-	if err != nil {
-		return d
-	}
-	return t.Sub(timeNow())
-}
-
-// backoff computes a duration after which an n+1 retry iteration should occur
-// using truncated exponential backoff algorithm.
-//
-// The n argument is always bounded between 0 and 30.
-// The max argument defines upper bound for the returned value.
-func backoff(n int, max time.Duration) time.Duration {
-	if n < 0 {
-		n = 0
-	}
-	if n > 30 {
-		n = 30
-	}
-	var d time.Duration
-	if x, err := rand.Int(rand.Reader, big.NewInt(1000)); err == nil {
-		d = time.Duration(x.Int64()) * time.Millisecond
-	}
-	d += time.Duration(1<<uint(n)) * time.Second
-	if d > max {
-		return max
-	}
-	return d
-}
-
 // keyAuth generates a key authorization string for a given token.
 func keyAuth(pub crypto.PublicKey, token string) (string, error) {
 	th, err := JWKThumbprint(pub)

acme/acme_test.go

@@ -15,7 +15,6 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"math/big"
 	"net/http"
 	"net/http/httptest"
@@ -485,88 +484,37 @@ func TestGetAuthorization(t *testing.T) {
 }
 
 func TestWaitAuthorization(t *testing.T) {
-	var count int
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		count++
-		w.Header().Set("Retry-After", "0")
-		if count > 1 {
-			fmt.Fprintf(w, `{"status":"valid"}`)
-			return
+	t.Run("wait loop", func(t *testing.T) {
+		var count int
+		authz, err := runWaitAuthorization(context.Background(), t, func(w http.ResponseWriter, r *http.Request) {
+			count++
+			w.Header().Set("Retry-After", "0")
+			if count > 1 {
+				fmt.Fprintf(w, `{"status":"valid"}`)
+				return
+			}
+			fmt.Fprintf(w, `{"status":"pending"}`)
+		})
+		if err != nil {
+			t.Fatalf("non-nil error: %v", err)
 		}
-		fmt.Fprintf(w, `{"status":"pending"}`)
-	}))
-	defer ts.Close()
-
-	type res struct {
-		authz *Authorization
-		err   error
-	}
-	done := make(chan res)
-	defer close(done)
-	go func() {
-		var client Client
-		a, err := client.WaitAuthorization(context.Background(), ts.URL)
-		done <- res{a, err}
-	}()
-
-	select {
-	case <-time.After(5 * time.Second):
-		t.Fatal("WaitAuthz took too long to return")
-	case res := <-done:
-		if res.err != nil {
-			t.Fatalf("res.err =  %v", res.err)
-		}
-		if res.authz == nil {
-			t.Fatal("res.authz is nil")
-		}
-	}
-}
-
-func TestWaitAuthorizationInvalid(t *testing.T) {
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprintf(w, `{"status":"invalid"}`)
-	}))
-	defer ts.Close()
-
-	res := make(chan error)
-	defer close(res)
-	go func() {
-		var client Client
-		_, err := client.WaitAuthorization(context.Background(), ts.URL)
-		res <- err
-	}()
-
-	select {
-	case <-time.After(3 * time.Second):
-		t.Fatal("WaitAuthz took too long to return")
-	case err := <-res:
-		if err == nil {
-			t.Error("err is nil")
+		if authz == nil {
+			t.Fatal("authz is nil")
 		}
+	})
+	t.Run("invalid status", func(t *testing.T) {
+		_, err := runWaitAuthorization(context.Background(), t, func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprintf(w, `{"status":"invalid"}`)
+		})
 		if _, ok := err.(*AuthorizationError); !ok {
-			t.Errorf("err is %T; want *AuthorizationError", err)
+			t.Errorf("err is %v (%T); want non-nil *AuthorizationError", err, err)
 		}
-	}
-}
-
-func TestWaitAuthorizationClientError(t *testing.T) {
-	const code = http.StatusBadRequest
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(code)
-	}))
-	defer ts.Close()
-
-	ch := make(chan error, 1)
-	go func() {
-		var client Client
-		_, err := client.WaitAuthorization(context.Background(), ts.URL)
-		ch <- err
-	}()
-
-	select {
-	case <-time.After(3 * time.Second):
-		t.Fatal("WaitAuthz took too long to return")
-	case err := <-ch:
+	})
+	t.Run("non-retriable error", func(t *testing.T) {
+		const code = http.StatusBadRequest
+		_, err := runWaitAuthorization(context.Background(), t, func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(code)
+		})
 		res, ok := err.(*Error)
 		if !ok {
 			t.Fatalf("err is %v (%T); want a non-nil *Error", err, err)
@@ -574,34 +522,60 @@ func TestWaitAuthorizationClientError(t *testing.T) {
 		if res.StatusCode != code {
 			t.Errorf("res.StatusCode = %d; want %d", res.StatusCode, code)
 		}
+	})
+	for _, code := range []int{http.StatusTooManyRequests, http.StatusInternalServerError} {
+		t.Run(fmt.Sprintf("retriable %d error", code), func(t *testing.T) {
+			var count int
+			authz, err := runWaitAuthorization(context.Background(), t, func(w http.ResponseWriter, r *http.Request) {
+				count++
+				w.Header().Set("Retry-After", "0")
+				if count > 1 {
+					fmt.Fprintf(w, `{"status":"valid"}`)
+					return
+				}
+				w.WriteHeader(code)
+			})
+			if err != nil {
+				t.Fatalf("non-nil error: %v", err)
+			}
+			if authz == nil {
+				t.Fatal("authz is nil")
+			}
+		})
 	}
-}
-
-func TestWaitAuthorizationCancel(t *testing.T) {
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.Header().Set("Retry-After", "60")
-		fmt.Fprintf(w, `{"status":"pending"}`)
-	}))
-	defer ts.Close()
-
-	res := make(chan error)
-	defer close(res)
-	go func() {
-		var client Client
+	t.Run("context cancel", func(t *testing.T) {
 		ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
 		defer cancel()
-		_, err := client.WaitAuthorization(ctx, ts.URL)
-		res <- err
-	}()
-
-	select {
-	case <-time.After(time.Second):
-		t.Fatal("WaitAuthz took too long to return")
-	case err := <-res:
+		_, err := runWaitAuthorization(ctx, t, func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Retry-After", "60")
+			fmt.Fprintf(w, `{"status":"pending"}`)
+		})
 		if err == nil {
 			t.Error("err is nil")
 		}
+	})
+}
+func runWaitAuthorization(ctx context.Context, t *testing.T, h http.HandlerFunc) (*Authorization, error) {
+	t.Helper()
+	ts := httptest.NewServer(h)
+	defer ts.Close()
+	type res struct {
+		authz *Authorization
+		err   error
 	}
+	ch := make(chan res, 1)
+	go func() {
+		var client Client
+		a, err := client.WaitAuthorization(ctx, ts.URL)
+		ch <- res{a, err}
+	}()
+	select {
+	case <-time.After(3 * time.Second):
+		t.Fatal("WaitAuthorization took too long to return")
+	case v := <-ch:
+		return v.authz, v.err
+	}
+	panic("runWaitAuthorization: out of select")
 }
 
 func TestRevokeAuthorization(t *testing.T) {
@@ -628,7 +602,7 @@ func TestRevokeAuthorization(t *testing.T) {
 				t.Errorf("req.Delete is false")
 			}
 		case "/2":
-			w.WriteHeader(http.StatusInternalServerError)
+			w.WriteHeader(http.StatusBadRequest)
 		}
 	}))
 	defer ts.Close()
@@ -849,7 +823,7 @@ func TestFetchCertRetry(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if count < 1 {
 			w.Header().Set("Retry-After", "0")
-			w.WriteHeader(http.StatusAccepted)
+			w.WriteHeader(http.StatusTooManyRequests)
 			count++
 			return
 		}
@@ -1096,44 +1070,6 @@ func TestNonce_postJWS(t *testing.T) {
 	}
 }
 
-func TestRetryPostJWS(t *testing.T) {
-	var count int
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		count++
-		w.Header().Set("Replay-Nonce", fmt.Sprintf("nonce%d", count))
-		if r.Method == "HEAD" {
-			// We expect the client to do 2 head requests to fetch
-			// nonces, one to start and another after getting badNonce
-			return
-		}
-
-		head, err := decodeJWSHead(r)
-		if err != nil {
-			t.Errorf("decodeJWSHead: %v", err)
-		} else if head.Nonce == "" {
-			t.Error("head.Nonce is empty")
-		} else if head.Nonce == "nonce1" {
-			// return a badNonce error to force the call to retry
-			w.WriteHeader(http.StatusBadRequest)
-			w.Write([]byte(`{"type":"urn:ietf:params:acme:error:badNonce"}`))
-			return
-		}
-		// Make client.Authorize happy; we're not testing its result.
-		w.WriteHeader(http.StatusCreated)
-		w.Write([]byte(`{"status":"valid"}`))
-	}))
-	defer ts.Close()
-
-	client := Client{Key: testKey, dir: &Directory{AuthzURL: ts.URL}}
-	// This call will fail with badNonce, causing a retry
-	if _, err := client.Authorize(context.Background(), "example.com"); err != nil {
-		t.Errorf("client.Authorize 1: %v", err)
-	}
-	if count != 4 {
-		t.Errorf("total requests count: %d; want 4", count)
-	}
-}
-
 func TestLinkHeader(t *testing.T) {
 	h := http.Header{"Link": {
 		`<https://example.com/acme/new-authz>;rel="next"`,
@@ -1157,37 +1093,6 @@ func TestLinkHeader(t *testing.T) {
 	}
 }
 
-func TestErrorResponse(t *testing.T) {
-	s := `{
-		"status": 400,
-		"type": "urn:acme:error:xxx",
-		"detail": "text"
-	}`
-	res := &http.Response{
-		StatusCode: 400,
-		Status:     "400 Bad Request",
-		Body:       ioutil.NopCloser(strings.NewReader(s)),
-		Header:     http.Header{"X-Foo": {"bar"}},
-	}
-	err := responseError(res)
-	v, ok := err.(*Error)
-	if !ok {
-		t.Fatalf("err = %+v (%T); want *Error type", err, err)
-	}
-	if v.StatusCode != 400 {
-		t.Errorf("v.StatusCode = %v; want 400", v.StatusCode)
-	}
-	if v.ProblemType != "urn:acme:error:xxx" {
-		t.Errorf("v.ProblemType = %q; want urn:acme:error:xxx", v.ProblemType)
-	}
-	if v.Detail != "text" {
-		t.Errorf("v.Detail = %q; want text", v.Detail)
-	}
-	if !reflect.DeepEqual(v.Header, res.Header) {
-		t.Errorf("v.Header = %+v; want %+v", v.Header, res.Header)
-	}
-}
-
 func TestTLSSNI01ChallengeCert(t *testing.T) {
 	const (
 		token = "evaGxfADs6pSRb2LAv9IZf17Dt3juxGJ-PCt92wr-oA"
@@ -1353,28 +1258,3 @@ func TestDNS01ChallengeRecord(t *testing.T) {
 		t.Errorf("val = %q; want %q", val, value)
 	}
 }
-
-func TestBackoff(t *testing.T) {
-	tt := []struct{ min, max time.Duration }{
-		{time.Second, 2 * time.Second},
-		{2 * time.Second, 3 * time.Second},
-		{4 * time.Second, 5 * time.Second},
-		{8 * time.Second, 9 * time.Second},
-	}
-	for i, test := range tt {
-		d := backoff(i, time.Minute)
-		if d < test.min || test.max < d {
-			t.Errorf("%d: d = %v; want between %v and %v", i, d, test.min, test.max)
-		}
-	}
-
-	min, max := time.Second, 2*time.Second
-	if d := backoff(-1, time.Minute); d < min || max < d {
-		t.Errorf("d = %v; want between %v and %v", d, min, max)
-	}
-
-	bound := 10 * time.Second
-	if d := backoff(100, bound); d != bound {
-		t.Errorf("d = %v; want %v", d, bound)
-	}
-}

acme/http.go

@@ -0,0 +1,276 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package acme
+
+import (
+	"bytes"
+	"context"
+	"crypto"
+	"crypto/rand"
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"math/big"
+	"net/http"
+	"strconv"
+	"strings"
+	"time"
+)
+
+// retryTimer encapsulates common logic for retrying unsuccessful requests.
+// It is not safe for concurrent use.
+type retryTimer struct {
+	// backoffFn provides backoff delay sequence for retries.
+	// See Client.RetryBackoff doc comment.
+	backoffFn func(n int, r *http.Request, res *http.Response) time.Duration
+	// n is the current retry attempt.
+	n int
+}
+
+func (t *retryTimer) inc() {
+	t.n++
+}
+
+// backoff pauses the current goroutine as described in Client.RetryBackoff.
+func (t *retryTimer) backoff(ctx context.Context, r *http.Request, res *http.Response) error {
+	d := t.backoffFn(t.n, r, res)
+	if d <= 0 {
+		return fmt.Errorf("acme: no more retries for %s; tried %d time(s)", r.URL, t.n)
+	}
+	wakeup := time.NewTimer(d)
+	defer wakeup.Stop()
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	case <-wakeup.C:
+		return nil
+	}
+}
+
+func (c *Client) retryTimer() *retryTimer {
+	f := c.RetryBackoff
+	if f == nil {
+		f = defaultBackoff
+	}
+	return &retryTimer{backoffFn: f}
+}
+
+// defaultBackoff provides default Client.RetryBackoff implementation
+// using a truncated exponential backoff algorithm,
+// as described in Client.RetryBackoff.
+//
+// The n argument is always bounded between 1 and 30.
+// The returned value is always greater than 0.
+func defaultBackoff(n int, r *http.Request, res *http.Response) time.Duration {
+	const max = 10 * time.Second
+	var jitter time.Duration
+	if x, err := rand.Int(rand.Reader, big.NewInt(1000)); err == nil {
+		// Set the minimum to 1ms to avoid a case where
+		// an invalid Retry-After value is parsed into 0 below,
+		// resulting in the 0 returned value which would unintentionally
+		// stop the retries.
+		jitter = (1 + time.Duration(x.Int64())) * time.Millisecond
+	}
+	if v, ok := res.Header["Retry-After"]; ok {
+		return retryAfter(v[0]) + jitter
+	}
+
+	if n < 1 {
+		n = 1
+	}
+	if n > 30 {
+		n = 30
+	}
+	d := time.Duration(1<<uint(n-1))*time.Second + jitter
+	if d > max {
+		return max
+	}
+	return d
+}
+
+// retryAfter parses a Retry-After HTTP header value,
+// trying to convert v into an int (seconds) or use http.ParseTime otherwise.
+// It returns zero value if v cannot be parsed.
+func retryAfter(v string) time.Duration {
+	if i, err := strconv.Atoi(v); err == nil {
+		return time.Duration(i) * time.Second
+	}
+	t, err := http.ParseTime(v)
+	if err != nil {
+		return 0
+	}
+	return t.Sub(timeNow())
+}
+
+// resOkay is a function that reports whether the provided response is okay.
+// It is expected to keep the response body unread.
+type resOkay func(*http.Response) bool
+
+// wantStatus returns a function which reports whether the code
+// matches the status code of a response.
+func wantStatus(codes ...int) resOkay {
+	return func(res *http.Response) bool {
+		for _, code := range codes {
+			if code == res.StatusCode {
+				return true
+			}
+		}
+		return false
+	}
+}
+
+// get issues an unsigned GET request to the specified URL.
+// It returns a non-error value only when ok reports true.
+//
+// get retries unsuccessful attempts according to c.RetryBackoff
+// until the context is done or a non-retriable error is received.
+func (c *Client) get(ctx context.Context, url string, ok resOkay) (*http.Response, error) {
+	retry := c.retryTimer()
+	for {
+		req, err := http.NewRequest("GET", url, nil)
+		if err != nil {
+			return nil, err
+		}
+		res, err := c.doNoRetry(ctx, req)
+		switch {
+		case err != nil:
+			return nil, err
+		case ok(res):
+			return res, nil
+		case isRetriable(res.StatusCode):
+			res.Body.Close()
+			retry.inc()
+			if err := retry.backoff(ctx, req, res); err != nil {
+				return nil, err
+			}
+		default:
+			defer res.Body.Close()
+			return nil, responseError(res)
+		}
+	}
+}
+
+// post issues a signed POST request in JWS format using the provided key
+// to the specified URL.
+// It returns a non-error value only when ok reports true.
+//
+// post retries unsuccessful attempts according to c.RetryBackoff
+// until the context is done or a non-retriable error is received.
+// It uses postNoRetry to make individual requests.
+func (c *Client) post(ctx context.Context, key crypto.Signer, url string, body interface{}, ok resOkay) (*http.Response, error) {
+	retry := c.retryTimer()
+	for {
+		res, req, err := c.postNoRetry(ctx, key, url, body)
+		if err != nil {
+			return nil, err
+		}
+		if ok(res) {
+			return res, nil
+		}
+		err = responseError(res)
+		res.Body.Close()
+		switch {
+		// Check for bad nonce before isRetriable because it may have been returned
+		// with an unretriable response code such as 400 Bad Request.
+		case isBadNonce(err):
+			// Consider any previously stored nonce values to be invalid.
+			c.clearNonces()
+		case !isRetriable(res.StatusCode):
+			return nil, err
+		}
+		retry.inc()
+		if err := retry.backoff(ctx, req, res); err != nil {
+			return nil, err
+		}
+	}
+}
+
+// postNoRetry signs the body with the given key and POSTs it to the provided url.
+// The body argument must be JSON-serializable.
+// It is used by c.post to retry unsuccessful attempts.
+func (c *Client) postNoRetry(ctx context.Context, key crypto.Signer, url string, body interface{}) (*http.Response, *http.Request, error) {
+	nonce, err := c.popNonce(ctx, url)
+	if err != nil {
+		return nil, nil, err
+	}
+	b, err := jwsEncodeJSON(body, key, nonce)
+	if err != nil {
+		return nil, nil, err
+	}
+	req, err := http.NewRequest("POST", url, bytes.NewReader(b))
+	if err != nil {
+		return nil, nil, err
+	}
+	req.Header.Set("Content-Type", "application/jose+json")
+	res, err := c.doNoRetry(ctx, req)
+	if err != nil {
+		return nil, nil, err
+	}
+	c.addNonce(res.Header)
+	return res, req, nil
+}
+
+// doNoRetry issues a request req, replacing its context (if any) with ctx.
+func (c *Client) doNoRetry(ctx context.Context, req *http.Request) (*http.Response, error) {
+	res, err := c.httpClient().Do(req.WithContext(ctx))
+	if err != nil {
+		select {
+		case <-ctx.Done():
+			// Prefer the unadorned context error.
+			// (The acme package had tests assuming this, previously from ctxhttp's
+			// behavior, predating net/http supporting contexts natively)
+			// TODO(bradfitz): reconsider this in the future. But for now this
+			// requires no test updates.
+			return nil, ctx.Err()
+		default:
+			return nil, err
+		}
+	}
+	return res, nil
+}
+
+func (c *Client) httpClient() *http.Client {
+	if c.HTTPClient != nil {
+		return c.HTTPClient
+	}
+	return http.DefaultClient
+}
+
+// isBadNonce reports whether err is an ACME "badnonce" error.
+func isBadNonce(err error) bool {
+	// According to the spec badNonce is urn:ietf:params:acme:error:badNonce.
+	// However, ACME servers in the wild return their versions of the error.
+	// See https://tools.ietf.org/html/draft-ietf-acme-acme-02#section-5.4
+	// and https://github.com/letsencrypt/boulder/blob/0e07eacb/docs/acme-divergences.md#section-66.
+	ae, ok := err.(*Error)
+	return ok && strings.HasSuffix(strings.ToLower(ae.ProblemType), ":badnonce")
+}
+
+// isRetriable reports whether a request can be retried
+// based on the response status code.
+//
+// Note that a "bad nonce" error is returned with a non-retriable 400 Bad Request code.
+// Callers should parse the response and check with isBadNonce.
+func isRetriable(code int) bool {
+	return code <= 399 || code >= 500 || code == http.StatusTooManyRequests
+}
+
+// responseError creates an error of Error type from resp.
+func responseError(resp *http.Response) error {
+	// don't care if ReadAll returns an error:
+	// json.Unmarshal will fail in that case anyway
+	b, _ := ioutil.ReadAll(resp.Body)
+	e := &wireError{Status: resp.StatusCode}
+	if err := json.Unmarshal(b, e); err != nil {
+		// this is not a regular error response:
+		// populate detail with anything we received,
+		// e.Status will already contain HTTP response code value
+		e.Detail = string(b)
+		if e.Detail == "" {
+			e.Detail = resp.Status
+		}
+	}
+	return e.error(resp.Header)
+}

acme/http_test.go

@@ -0,0 +1,158 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package acme
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"reflect"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestDefaultBackoff(t *testing.T) {
+	tt := []struct {
+		nretry     int
+		retryAfter string        // Retry-After header
+		out        time.Duration // expected min; max = min + jitter
+	}{
+		{-1, "", time.Second},       // verify the lower bound is 1
+		{0, "", time.Second},        // verify the lower bound is 1
+		{100, "", 10 * time.Second}, // verify the ceiling
+		{1, "3600", time.Hour},      // verify the header value is used
+		{1, "", 1 * time.Second},
+		{2, "", 2 * time.Second},
+		{3, "", 4 * time.Second},
+		{4, "", 8 * time.Second},
+	}
+	for i, test := range tt {
+		r := httptest.NewRequest("GET", "/", nil)
+		resp := &http.Response{Header: http.Header{}}
+		if test.retryAfter != "" {
+			resp.Header.Set("Retry-After", test.retryAfter)
+		}
+		d := defaultBackoff(test.nretry, r, resp)
+		max := test.out + time.Second // + max jitter
+		if d < test.out || max < d {
+			t.Errorf("%d: defaultBackoff(%v) = %v; want between %v and %v", i, test.nretry, d, test.out, max)
+		}
+	}
+}
+
+func TestErrorResponse(t *testing.T) {
+	s := `{
+		"status": 400,
+		"type": "urn:acme:error:xxx",
+		"detail": "text"
+	}`
+	res := &http.Response{
+		StatusCode: 400,
+		Status:     "400 Bad Request",
+		Body:       ioutil.NopCloser(strings.NewReader(s)),
+		Header:     http.Header{"X-Foo": {"bar"}},
+	}
+	err := responseError(res)
+	v, ok := err.(*Error)
+	if !ok {
+		t.Fatalf("err = %+v (%T); want *Error type", err, err)
+	}
+	if v.StatusCode != 400 {
+		t.Errorf("v.StatusCode = %v; want 400", v.StatusCode)
+	}
+	if v.ProblemType != "urn:acme:error:xxx" {
+		t.Errorf("v.ProblemType = %q; want urn:acme:error:xxx", v.ProblemType)
+	}
+	if v.Detail != "text" {
+		t.Errorf("v.Detail = %q; want text", v.Detail)
+	}
+	if !reflect.DeepEqual(v.Header, res.Header) {
+		t.Errorf("v.Header = %+v; want %+v", v.Header, res.Header)
+	}
+}
+
+func TestPostWithRetries(t *testing.T) {
+	var count int
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		count++
+		w.Header().Set("Replay-Nonce", fmt.Sprintf("nonce%d", count))
+		if r.Method == "HEAD" {
+			// We expect the client to do 2 head requests to fetch
+			// nonces, one to start and another after getting badNonce
+			return
+		}
+
+		head, err := decodeJWSHead(r)
+		if err != nil {
+			t.Errorf("decodeJWSHead: %v", err)
+		} else if head.Nonce == "" {
+			t.Error("head.Nonce is empty")
+		} else if head.Nonce == "nonce1" {
+			// return a badNonce error to force the call to retry
+			w.WriteHeader(http.StatusBadRequest)
+			w.Write([]byte(`{"type":"urn:ietf:params:acme:error:badNonce"}`))
+			return
+		}
+		// Make client.Authorize happy; we're not testing its result.
+		w.WriteHeader(http.StatusCreated)
+		w.Write([]byte(`{"status":"valid"}`))
+	}))
+	defer ts.Close()
+
+	client := &Client{Key: testKey, dir: &Directory{AuthzURL: ts.URL}}
+	// This call will fail with badNonce, causing a retry
+	if _, err := client.Authorize(context.Background(), "example.com"); err != nil {
+		t.Errorf("client.Authorize 1: %v", err)
+	}
+	if count != 4 {
+		t.Errorf("total requests count: %d; want 4", count)
+	}
+}
+
+func TestRetryBackoffArgs(t *testing.T) {
+	const resCode = http.StatusInternalServerError
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Replay-Nonce", "test-nonce")
+		w.WriteHeader(resCode)
+	}))
+	defer ts.Close()
+
+	// Canceled in backoff.
+	ctx, cancel := context.WithCancel(context.Background())
+
+	var nretry int
+	backoff := func(n int, r *http.Request, res *http.Response) time.Duration {
+		nretry++
+		if n != nretry {
+			t.Errorf("n = %d; want %d", n, nretry)
+		}
+		if nretry == 3 {
+			cancel()
+		}
+
+		if r == nil {
+			t.Error("r is nil")
+		}
+		if res.StatusCode != resCode {
+			t.Errorf("res.StatusCode = %d; want %d", res.StatusCode, resCode)
+		}
+		return time.Millisecond
+	}
+
+	client := &Client{
+		Key:          testKey,
+		RetryBackoff: backoff,
+		dir:          &Directory{AuthzURL: ts.URL},
+	}
+	if _, err := client.Authorize(ctx, "example.com"); err == nil {
+		t.Error("err is nil")
+	}
+	if nretry != 3 {
+		t.Errorf("nretry = %d; want 3", nretry)
+	}
+}

acme/types.go

@@ -104,7 +104,7 @@ func RateLimit(err error) (time.Duration, bool) {
 	if e.Header == nil {
 		return 0, true
 	}
-	return retryAfter(e.Header.Get("Retry-After"), 0), true
+	return retryAfter(e.Header.Get("Retry-After")), true
 }
 
 // Account is a user account. It is associated with a private key.