From a49355c7e3f8fe157a85be2f77e6e269a0f89602 Mon Sep 17 00:00:00 2001
From: Alex Vaghin <ddos@google.com>
Date: Wed, 20 Jun 2018 11:14:27 +0200
Subject: [PATCH] acme: consistently return original errors from retries
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The retry logic returns an "acme: no more retries for ..." error
in some cases, while *Error type in others.

This change makes retries always return the last error as received
from the CA server, if available. No change in returned values
of successful requests.

Change-Id: I3df2cb332a3e2739bba457c0ee50d7ca5bd836d9
Reviewed-on: https://go-review.googlesource.com/119975
Reviewed-by: Maciej Dębski <maciejd@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Alex Vaghin <ddos@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 acme/http.go      | 19 +++++++++------
 acme/http_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/acme/http.go b/acme/http.go
index 56ba53a..a43ce6a 100644
--- a/acme/http.go
+++ b/acme/http.go
@@ -140,10 +140,13 @@ func (c *Client) get(ctx context.Context, url string, ok resOkay) (*http.Respons
 		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
+			resErr := responseError(res)
+			res.Body.Close()
+			// Ignore the error value from retry.backoff
+			// and return the one from last retry, as received from the CA.
+			if retry.backoff(ctx, req, res) != nil {
+				return nil, resErr
 			}
 		default:
 			defer res.Body.Close()
@@ -169,20 +172,22 @@ func (c *Client) post(ctx context.Context, key crypto.Signer, url string, body i
 		if ok(res) {
 			return res, nil
 		}
-		err = responseError(res)
+		resErr := 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):
+		case isBadNonce(resErr):
 			// Consider any previously stored nonce values to be invalid.
 			c.clearNonces()
 		case !isRetriable(res.StatusCode):
-			return nil, err
+			return nil, resErr
 		}
 		retry.inc()
+		// Ignore the error value from retry.backoff
+		// and return the one from last retry, as received from the CA.
 		if err := retry.backoff(ctx, req, res); err != nil {
-			return nil, err
+			return nil, resErr
 		}
 	}
 }
diff --git a/acme/http_test.go b/acme/http_test.go
index c9ce1d6..15e401b 100644
--- a/acme/http_test.go
+++ b/acme/http_test.go
@@ -88,12 +88,14 @@ func TestPostWithRetries(t *testing.T) {
 		}
 
 		head, err := decodeJWSHead(r)
-		if err != nil {
+		switch {
+		case err != nil:
 			t.Errorf("decodeJWSHead: %v", err)
-		} else if head.Nonce == "" {
+		case head.Nonce == "":
 			t.Error("head.Nonce is empty")
-		} else if head.Nonce == "nonce1" {
-			// return a badNonce error to force the call to retry
+		case head.Nonce == "nonce1":
+			// Return a badNonce error to force the call to retry.
+			w.Header().Set("Retry-After", "0")
 			w.WriteHeader(http.StatusBadRequest)
 			w.Write([]byte(`{"type":"urn:ietf:params:acme:error:badNonce"}`))
 			return
@@ -114,6 +116,55 @@ func TestPostWithRetries(t *testing.T) {
 	}
 }
 
+func TestRetryErrorType(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Replay-Nonce", "nonce")
+		w.WriteHeader(http.StatusTooManyRequests)
+		w.Write([]byte(`{"type":"rateLimited"}`))
+	}))
+	defer ts.Close()
+
+	client := &Client{
+		Key: testKey,
+		RetryBackoff: func(n int, r *http.Request, res *http.Response) time.Duration {
+			// Do no retries.
+			return 0
+		},
+		dir: &Directory{AuthzURL: ts.URL},
+	}
+
+	t.Run("post", func(t *testing.T) {
+		testRetryErrorType(t, func() error {
+			_, err := client.Authorize(context.Background(), "example.com")
+			return err
+		})
+	})
+	t.Run("get", func(t *testing.T) {
+		testRetryErrorType(t, func() error {
+			_, err := client.GetAuthorization(context.Background(), ts.URL)
+			return err
+		})
+	})
+}
+
+func testRetryErrorType(t *testing.T, callClient func() error) {
+	t.Helper()
+	err := callClient()
+	if err == nil {
+		t.Fatal("client.Authorize returned nil error")
+	}
+	acmeErr, ok := err.(*Error)
+	if !ok {
+		t.Fatalf("err is %v (%T); want *Error", err, err)
+	}
+	if acmeErr.StatusCode != http.StatusTooManyRequests {
+		t.Errorf("acmeErr.StatusCode = %d; want %d", acmeErr.StatusCode, http.StatusTooManyRequests)
+	}
+	if acmeErr.ProblemType != "rateLimited" {
+		t.Errorf("acmeErr.ProblemType = %q; want 'rateLimited'", acmeErr.ProblemType)
+	}
+}
+
 func TestRetryBackoffArgs(t *testing.T) {
 	const resCode = http.StatusInternalServerError
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {