From 8e03fc1ab6a36bdb9e7dee75a213c30f0249d0c1 Mon Sep 17 00:00:00 2001
From: Alex Vaghin <ddos@google.com>
Date: Tue, 25 Apr 2017 11:58:20 +0200
Subject: [PATCH] acme/autocert: remove failed state entries

This change makes the Manager try creating a certificate
again, after a previously unsuccessful attempt.

The implementation is based on a timer, to prevent hitting
an ACME CA with too high QPS when under a heavy load.
The timer is hardcoded to 1 minute.

Fixes golang/go#17740.
Change-Id: I46a49201cf423be3360633a89209d7b2bccc1d76
Reviewed-on: https://go-review.googlesource.com/41694
Run-TryBot: Alex Vaghin <ddos@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
---
 acme/autocert/autocert.go      | 33 ++++++++++++++++++++--
 acme/autocert/autocert_test.go | 50 ++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index 679ff4d..2b5d068 100644
--- a/acme/autocert/autocert.go
+++ b/acme/autocert/autocert.go
@@ -33,6 +33,12 @@ import (
 	"golang.org/x/crypto/acme"
 )
 
+// createCertRetryAfter is how much time to wait before removing a failed state
+// entry due to an unsuccessful createCert call.
+// This is a variable instead of a const for testing.
+// TODO: Consider making it configurable or an exp backoff?
+var createCertRetryAfter = time.Minute
+
 // pseudoRand is safe for concurrent use.
 var pseudoRand *lockedMathRand
 
@@ -363,6 +369,23 @@ func (m *Manager) createCert(ctx context.Context, domain string) (*tls.Certifica
 
 	der, leaf, err := m.authorizedCert(ctx, state.key, domain)
 	if err != nil {
+		// Remove the failed state after some time,
+		// making the manager call createCert again on the following TLS hello.
+		time.AfterFunc(createCertRetryAfter, func() {
+			defer testDidRemoveState(domain)
+			m.stateMu.Lock()
+			defer m.stateMu.Unlock()
+			// Verify the state hasn't changed and it's still invalid
+			// before deleting.
+			s, ok := m.state[domain]
+			if !ok {
+				return
+			}
+			if _, err := validCert(domain, s.cert, s.key); err == nil {
+				return
+			}
+			delete(m.state, domain)
+		})
 		return nil, err
 	}
 	state.cert = der
@@ -411,7 +434,6 @@ func (m *Manager) certState(domain string) (*certState, error) {
 // authorizedCert starts domain ownership verification process and requests a new cert upon success.
 // The key argument is the certificate private key.
 func (m *Manager) authorizedCert(ctx context.Context, key crypto.Signer, domain string) (der [][]byte, leaf *x509.Certificate, err error) {
-	// TODO: make m.verify retry or retry m.verify calls here
 	if err := m.verify(ctx, domain); err != nil {
 		return nil, nil, err
 	}
@@ -782,5 +804,10 @@ func (r *lockedMathRand) int63n(max int64) int64 {
 	return n
 }
 
-// for easier testing
-var timeNow = time.Now
+// For easier testing.
+var (
+	timeNow = time.Now
+
+	// Called when a state is removed.
+	testDidRemoveState = func(domain string) {}
+)
diff --git a/acme/autocert/autocert_test.go b/acme/autocert/autocert_test.go
index 3166c1f..4bb4640 100644
--- a/acme/autocert/autocert_test.go
+++ b/acme/autocert/autocert_test.go
@@ -210,6 +210,56 @@ func TestGetCertificate_expiredCache(t *testing.T) {
 	testGetCertificate(t, man, "example.org", hello)
 }
 
+func TestGetCertificate_failedAttempt(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadRequest)
+	}))
+	defer ts.Close()
+
+	const example = "example.org"
+	d := createCertRetryAfter
+	f := testDidRemoveState
+	defer func() {
+		createCertRetryAfter = d
+		testDidRemoveState = f
+	}()
+	createCertRetryAfter = 0
+	done := make(chan struct{})
+	testDidRemoveState = func(domain string) {
+		if domain != example {
+			t.Errorf("testDidRemoveState: domain = %q; want %q", domain, example)
+		}
+		close(done)
+	}
+
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	man := &Manager{
+		Prompt: AcceptTOS,
+		Client: &acme.Client{
+			Key:          key,
+			DirectoryURL: ts.URL,
+		},
+	}
+	defer man.stopRenew()
+	hello := &tls.ClientHelloInfo{ServerName: example}
+	if _, err := man.GetCertificate(hello); err == nil {
+		t.Error("GetCertificate: err is nil")
+	}
+	select {
+	case <-time.After(5 * time.Second):
+		t.Errorf("took too long to remove the %q state", example)
+	case <-done:
+		man.stateMu.Lock()
+		defer man.stateMu.Unlock()
+		if v, exist := man.state[example]; exist {
+			t.Errorf("state exists for %q: %+v", example, v)
+		}
+	}
+}
+
 // startACMEServerStub runs an ACME server
 // The domain argument is the expected domain name of a certificate request.
 func startACMEServerStub(t *testing.T, man *Manager, domain string) (url string, finish func()) {