acme/autocert: revoke dangling pending authzs

We now keep track of pending authorization requests during verify() and
defer the asynchronous revocation of the ones that failed.
This should help avoid letsencrypt's "too many currently pending
authorizations" error.

Fixes golang/go#23426

Change-Id: Ibffb10f59733962d45e43b67fc42a2ec7c5faf51
Reviewed-on: https://go-review.googlesource.com/100078
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Leo Antunes <costela@gmail.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

Author: desdeel2d0m

acme/autocert/autocert.go

@@ -542,6 +542,20 @@ func (m *Manager) authorizedCert(ctx context.Context, key crypto.Signer, domain
 	return der, leaf, nil
 }
 
+// revokePending revokes all provided authorizations (passed as a map of URIs)
+func (m *Manager) revokePending(ctx context.Context, pendingAuthzURIs map[string]bool) {
+	f := func(ctx context.Context) {
+		for uri := range pendingAuthzURIs {
+			m.Client.RevokeAuthorization(ctx, uri)
+		}
+	}
+	if waitForRevocations {
+		f(ctx)
+	} else {
+		go f(context.Background())
+	}
+}
+
 // verify runs the identifier (domain) authorization flow
 // using each applicable ACME challenge type.
 func (m *Manager) verify(ctx context.Context, client *acme.Client, domain string) error {
@@ -554,6 +568,10 @@ func (m *Manager) verify(ctx context.Context, client *acme.Client, domain string
 	}
 	m.tokensMu.RUnlock()
 
+	// we keep track of pending authzs and revoke the ones that did not validate.
+	pendingAuthzs := make(map[string]bool)
+	defer m.revokePending(ctx, pendingAuthzs)
+
 	var nextTyp int // challengeType index of the next challenge type to try
 	for {
 		// Start domain authorization and get the challenge.
@@ -570,6 +588,8 @@ func (m *Manager) verify(ctx context.Context, client *acme.Client, domain string
 			return fmt.Errorf("acme/autocert: invalid authorization %q", authz.URI)
 		}
 
+		pendingAuthzs[authz.URI] = true
+
 		// Pick the next preferred challenge.
 		var chal *acme.Challenge
 		for chal == nil && nextTyp < len(challengeTypes) {
@@ -590,6 +610,7 @@ func (m *Manager) verify(ctx context.Context, client *acme.Client, domain string
 
 		// A challenge is fulfilled and accepted: wait for the CA to validate.
 		if _, err := client.WaitAuthorization(ctx, authz.URI); err == nil {
+			delete(pendingAuthzs, authz.URI)
 			return nil
 		}
 	}
@@ -959,4 +980,7 @@ var (
 
 	// Called when a state is removed.
 	testDidRemoveState = func(domain string) {}
+
+	// make testing of revokePending synchronous
+	waitForRevocations = false
 )

acme/autocert/autocert_test.go

@@ -19,10 +19,12 @@ import (
 	"fmt"
 	"html/template"
 	"io"
+	"io/ioutil"
 	"math/big"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -529,6 +531,103 @@ func TestVerifyHTTP01(t *testing.T) {
 	}
 }
 
+func TestRevokeFailedAuth(t *testing.T) {
+	var (
+		authzCount int     // num. of created authorizations
+		revoked    [3]bool // there will be three different authorization attempts; see below
+	)
+
+	// make cleanup revocations synchronous
+	waitForRevocations = true
+
+	// ACME CA server stub, only the needed bits.
+	// TODO: Merge this with startACMEServerStub, making it a configurable CA for testing.
+	var ca *httptest.Server
+	ca = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Replay-Nonce", "nonce")
+		if r.Method == "HEAD" {
+			// a nonce request
+			return
+		}
+
+		switch r.URL.Path {
+		// Discovery.
+		case "/":
+			if err := discoTmpl.Execute(w, ca.URL); err != nil {
+				t.Errorf("discoTmpl: %v", err)
+			}
+		// Client key registration.
+		case "/new-reg":
+			w.Write([]byte("{}"))
+		// New domain authorization.
+		case "/new-authz":
+			w.Header().Set("Location", fmt.Sprintf("%s/authz/%d", ca.URL, authzCount))
+			w.WriteHeader(http.StatusCreated)
+			if err := authzTmpl.Execute(w, ca.URL); err != nil {
+				t.Errorf("authzTmpl: %v", err)
+			}
+			authzCount++
+		// force error on Accept()
+		case "/challenge/2":
+			http.Error(w, "won't accept tls-sni-02 challenge", http.StatusBadRequest)
+		// accept; authorization will be expired (404; see /authz/1 below)
+		case "/challenge/1":
+			w.Write([]byte("{}"))
+		// Authorization statuses.
+		case "/authz/0", "/authz/1", "/authz/2":
+			if r.Method == "POST" {
+				body, err := ioutil.ReadAll(r.Body)
+				if err != nil {
+					t.Errorf("could not read request body")
+				}
+				if reflect.DeepEqual(body, []byte(`{"status": "deactivated"}`)) {
+					t.Errorf("unexpected POST to authz: '%s'", body)
+				}
+				i, err := strconv.ParseInt(r.URL.Path[len(r.URL.Path)-1:], 10, 64)
+				if err != nil {
+					t.Errorf("could not convert authz ID to int")
+				}
+				revoked[i] = true
+				w.Write([]byte(`{"status": "invalid"}`))
+			} else {
+				switch r.URL.Path {
+				case "/authz/0":
+					// ensure we get a spurious validation if error forcing did not work (see above)
+					w.Write([]byte(`{"status": "valid"}`))
+				case "/authz/1":
+					// force error during WaitAuthorization()
+					w.WriteHeader(http.StatusNotFound)
+				}
+			}
+		default:
+			http.NotFound(w, r)
+			t.Errorf("unrecognized r.URL.Path: %s", r.URL.Path)
+		}
+	}))
+	defer ca.Close()
+
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	m := &Manager{
+		Client: &acme.Client{
+			Key:          key,
+			DirectoryURL: ca.URL,
+		},
+	}
+
+	// should fail and revoke tsl-sni-02, tls-sni-01 and the third time after not finding any remaining challenges
+	if err := m.verify(context.Background(), m.Client, "example.org"); err == nil {
+		t.Errorf("m.verify should have failed!")
+	}
+	for i := range revoked {
+		if !revoked[i] {
+			t.Errorf("authorization attempt %d not revoked after error", i)
+		}
+	}
+}
+
 func TestHTTPHandlerDefaultFallback(t *testing.T) {
 	tt := []struct {
 		method, url  string