From a4c6cb3142f211c99e4bf4cd769535b29a9b616f Mon Sep 17 00:00:00 2001 From: Alex Vaghin Date: Tue, 12 Feb 2019 18:56:05 +0100 Subject: [PATCH] acme: try to fetch nonce from directory first The change should reduce resource quota consumed by the client overall. Instead of sending HEAD to an ACME resource URL to get a new nonce, the Client will now try to fetch it from the Directory URL first and only then from the ACME resource URL if the former fails. This builds up on an abandoned https://golang.org/cl/34623, only this time with a fallback to the original behaviour. Change-Id: I6e75c0e524c4bc751f3a651b290c0ac2493e0628 Reviewed-on: https://go-review.googlesource.com/c/162057 Run-TryBot: Alex Vaghin TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- acme/acme.go | 23 ++++++++++----- acme/acme_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++--- acme/http_test.go | 6 +++- 3 files changed, 92 insertions(+), 12 deletions(-) diff --git a/acme/acme.go b/acme/acme.go index edd1c82b..00ee9555 100644 --- a/acme/acme.go +++ b/acme/acme.go @@ -128,11 +128,7 @@ func (c *Client) Discover(ctx context.Context) (Directory, error) { return *c.dir, nil } - dirURL := c.DirectoryURL - if dirURL == "" { - dirURL = LetsEncryptURL - } - res, err := c.get(ctx, dirURL, wantStatus(http.StatusOK)) + res, err := c.get(ctx, c.directoryURL(), wantStatus(http.StatusOK)) if err != nil { return Directory{}, err } @@ -165,6 +161,13 @@ func (c *Client) Discover(ctx context.Context) (Directory, error) { return *c.dir, nil } +func (c *Client) directoryURL() string { + if c.DirectoryURL != "" { + return c.DirectoryURL + } + return LetsEncryptURL +} + // CreateCert requests a new certificate using the Certificate Signing Request csr encoded in DER format. // The exp argument indicates the desired certificate validity duration. CA may issue a certificate // with a different duration. @@ -711,12 +714,18 @@ func (c *Client) doReg(ctx context.Context, url string, typ string, acct *Accoun } // popNonce returns a nonce value previously stored with c.addNonce -// or fetches a fresh one from the given URL. +// or fetches a fresh one from a URL by issuing a HEAD request. +// It first tries c.directoryURL() and then the provided url if the former fails. func (c *Client) popNonce(ctx context.Context, url string) (string, error) { c.noncesMu.Lock() defer c.noncesMu.Unlock() if len(c.nonces) == 0 { - return c.fetchNonce(ctx, url) + dirURL := c.directoryURL() + v, err := c.fetchNonce(ctx, dirURL) + if err != nil && url != dirURL { + v, err = c.fetchNonce(ctx, url) + } + return v, err } var nonce string for nonce = range c.nonces { diff --git a/acme/acme_test.go b/acme/acme_test.go index 29d7abba..99a4bf89 100644 --- a/acme/acme_test.go +++ b/acme/acme_test.go @@ -75,6 +75,7 @@ func TestDiscover(t *testing.T) { ) ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") + w.Header().Set("Replay-Nonce", "testnonce") fmt.Fprintf(w, `{ "new-reg": %q, "new-authz": %q, @@ -100,6 +101,9 @@ func TestDiscover(t *testing.T) { if dir.RevokeURL != revoke { t.Errorf("dir.RevokeURL = %q; want %q", dir.RevokeURL, revoke) } + if _, exist := c.nonces["testnonce"]; !exist { + t.Errorf("c.nonces = %q; want 'testnonce' in the map", c.nonces) + } } func TestRegister(t *testing.T) { @@ -147,7 +151,11 @@ func TestRegister(t *testing.T) { return false } - c := Client{Key: testKeyEC, dir: &Directory{RegURL: ts.URL}} + c := Client{ + Key: testKeyEC, + DirectoryURL: ts.URL, + dir: &Directory{RegURL: ts.URL}, + } a := &Account{Contact: contacts} var err error if a, err = c.Register(context.Background(), a, prompt); err != nil { @@ -351,7 +359,11 @@ func TestAuthorize(t *testing.T) { auth *Authorization err error ) - cl := Client{Key: testKeyEC, dir: &Directory{AuthzURL: ts.URL}} + cl := Client{ + Key: testKeyEC, + DirectoryURL: ts.URL, + dir: &Directory{AuthzURL: ts.URL}, + } switch test.typ { case "dns": auth, err = cl.Authorize(context.Background(), test.value) @@ -422,7 +434,11 @@ func TestAuthorizeValid(t *testing.T) { w.Write([]byte(`{"status":"valid"}`)) })) defer ts.Close() - client := Client{Key: testKey, dir: &Directory{AuthzURL: ts.URL}} + client := Client{ + Key: testKey, + DirectoryURL: ts.URL, + dir: &Directory{AuthzURL: ts.URL}, + } _, err := client.Authorize(context.Background(), "example.com") if err != nil { t.Errorf("err = %v", err) @@ -1037,6 +1053,53 @@ func TestNonce_fetchError(t *testing.T) { } } +func TestNonce_popWhenEmpty(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "HEAD" { + t.Errorf("r.Method = %q; want HEAD", r.Method) + } + switch r.URL.Path { + case "/dir-with-nonce": + w.Header().Set("Replay-Nonce", "dirnonce") + case "/new-nonce": + w.Header().Set("Replay-Nonce", "newnonce") + case "/dir-no-nonce", "/empty": + // No nonce in the header. + default: + t.Errorf("Unknown URL: %s", r.URL) + } + })) + defer ts.Close() + ctx := context.Background() + + tt := []struct { + dirURL, popURL, nonce string + wantOK bool + }{ + {ts.URL + "/dir-with-nonce", ts.URL + "/new-nonce", "dirnonce", true}, + {ts.URL + "/dir-no-nonce", ts.URL + "/new-nonce", "newnonce", true}, + {ts.URL + "/dir-no-nonce", ts.URL + "/empty", "", false}, + } + for _, test := range tt { + t.Run(fmt.Sprintf("nonce:%s wantOK:%v", test.nonce, test.wantOK), func(t *testing.T) { + c := Client{DirectoryURL: test.dirURL} + v, err := c.popNonce(ctx, test.popURL) + if !test.wantOK { + if err == nil { + t.Fatalf("c.popNonce(%q) returned nil error", test.popURL) + } + return + } + if err != nil { + t.Fatalf("c.popNonce(%q): %v", test.popURL, err) + } + if v != test.nonce { + t.Errorf("c.popNonce(%q) = %q; want %q", test.popURL, v, test.nonce) + } + }) + } +} + func TestNonce_postJWS(t *testing.T) { var count int seen := make(map[string]bool) @@ -1070,7 +1133,11 @@ func TestNonce_postJWS(t *testing.T) { })) defer ts.Close() - client := Client{Key: testKey, dir: &Directory{AuthzURL: ts.URL}} + client := Client{ + Key: testKey, + DirectoryURL: ts.URL, // nonces are fetched from here first + dir: &Directory{AuthzURL: ts.URL}, + } if _, err := client.Authorize(context.Background(), "example.com"); err != nil { t.Errorf("client.Authorize 1: %v", err) } diff --git a/acme/http_test.go b/acme/http_test.go index 15e401ba..2748995a 100644 --- a/acme/http_test.go +++ b/acme/http_test.go @@ -106,7 +106,11 @@ func TestPostWithRetries(t *testing.T) { })) defer ts.Close() - client := &Client{Key: testKey, dir: &Directory{AuthzURL: ts.URL}} + client := &Client{ + Key: testKey, + DirectoryURL: ts.URL, + 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)