acme/autocert: improve authorizations cleanup

Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.

The change also improves related tests,
removing code paths diverging in testing.

Fixes golang/go#25581

Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>

acme/autocert/autocert.go

@@ -551,17 +551,15 @@ 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)
-		}
+// revokePendingAuthz revokes all authorizations idenfied by the elements of uri slice.
+// It ignores revocation errors.
+func (m *Manager) revokePendingAuthz(ctx context.Context, uri []string) {
+	client, err := m.acmeClient(ctx)
+	if err != nil {
+		return
 	}
-	if waitForRevocations {
-		f(ctx)
-	} else {
-		go f(context.Background())
+	for _, u := range uri {
+		client.RevokeAuthorization(ctx, u)
 	}
 }
 
@@ -577,9 +575,21 @@ 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.
+	// Keep track of pending authzs and revoke the ones that did not validate.
 	pendingAuthzs := make(map[string]bool)
-	defer m.revokePending(ctx, pendingAuthzs)
+	defer func() {
+		var uri []string
+		for k, pending := range pendingAuthzs {
+			if pending {
+				uri = append(uri, k)
+			}
+		}
+		if len(uri) > 0 {
+			// Use "detached" background context.
+			// The revocations need not happen in the current verification flow.
+			go m.revokePendingAuthz(context.Background(), uri)
+		}
+	}()
 
 	var nextTyp int // challengeType index of the next challenge type to try
 	for {
@@ -989,7 +999,4 @@ var (
 
 	// Called when a state is removed.
 	testDidRemoveState = func(domain string) {}
-
-	// make testing of revokePending synchronous
-	waitForRevocations = false
 )

acme/autocert/autocert_test.go

@@ -20,12 +20,10 @@ import (
 	"fmt"
 	"html/template"
 	"io"
-	"io/ioutil"
 	"math/big"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
-	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -532,14 +530,20 @@ 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
-	)
+func TestRevokeFailedAuthz(t *testing.T) {
+	// Prefill authorization URIs expected to be revoked.
+	// The challenges are selected in a specific order,
+	// each tried within a newly created authorization.
+	// This means each authorization URI corresponds to a different challenge type.
+	revokedAuthz := map[string]bool{
+		"/authz/0": false, // tls-sni-02
+		"/authz/1": false, // tls-sni-01
+		"/authz/2": false, // no viable challenge, but authz is created
+	}
 
-	// make cleanup revocations synchronous
-	waitForRevocations = true
+	var authzCount int          // num. of created authorizations
+	var revokeCount int         // num. of revoked authorizations
+	done := make(chan struct{}) // closed when revokeCount is 3
 
 	// ACME CA server stub, only the needed bits.
 	// TODO: Merge this with startACMEServerStub, making it a configurable CA for testing.
@@ -568,38 +572,39 @@ func TestRevokeFailedAuth(t *testing.T) {
 				t.Errorf("authzTmpl: %v", err)
 			}
 			authzCount++
-		// force error on Accept()
+		// tls-sni-02 challenge "accept" request.
 		case "/challenge/2":
+			// Refuse.
 			http.Error(w, "won't accept tls-sni-02 challenge", http.StatusBadRequest)
-		// accept; authorization will be expired (404; see /authz/1 below)
+		// tls-sni-01 challenge "accept" request.
 		case "/challenge/1":
+			// Accept but the authorization will be "expired".
 			w.Write([]byte("{}"))
-		// Authorization statuses.
+		// Authorization requests.
 		case "/authz/0", "/authz/1", "/authz/2":
+			// Revocation requests.
 			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)
+				var req struct{ Status string }
+				if err := decodePayload(&req, r.Body); err != nil {
+					t.Errorf("%s: decodePayload: %v", r.URL, err)
 				}
-				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")
+				switch req.Status {
+				case "deactivated":
+					revokedAuthz[r.URL.Path] = true
+					revokeCount++
+					if revokeCount >= 3 {
+						// Last authorization is revoked.
+						defer close(done)
+					}
+				default:
+					t.Errorf("%s: req.Status = %q; want 'deactivated'", r.URL, req.Status)
 				}
-				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)
-				}
+				return
 			}
+			// Authorization status requests.
+			// Simulate abandoned authorization, deleted by the CA.
+			w.WriteHeader(http.StatusNotFound)
 		default:
 			http.NotFound(w, r)
 			t.Errorf("unrecognized r.URL.Path: %s", r.URL.Path)
@@ -607,24 +612,25 @@ func TestRevokeFailedAuth(t *testing.T) {
 	}))
 	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,
-		},
+		Client: &acme.Client{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!")
+	// Should fail and revoke 3 authorizations.
+	// The first 2 are tsl-sni-02 and tls-sni-01 challenges.
+	// The third time an authorization is created but no viable challenge is found.
+	// See revokedAuthz above for more explanation.
+	if _, err := m.createCert(context.Background(), "example.org"); err == nil {
+		t.Errorf("m.createCert returned nil error")
+	}
+	select {
+	case <-time.After(3 * time.Second):
+		t.Error("revocations took too long")
+	case <-done:
+		// revokeCount is at least 3.
 	}
-	for i := range revoked {
-		if !revoked[i] {
-			t.Errorf("authorization attempt %d not revoked after error", i)
+	for uri, ok := range revokedAuthz {
+		if !ok {
+			t.Errorf("%q authorization was not revoked", uri)
 		}
 	}
 }