From 09d8a49bf219d4f1e06cfb7e7604e62228a607bb Mon Sep 17 00:00:00 2001
From: Pavel Forkert <fxposter@gmail.com>
Date: Sun, 19 Feb 2017 05:48:45 +0200
Subject: [PATCH] Reduce nonce locking (#340)

* [reduce-locking] Prepare for change

* [reduce-locking] Do not lock on http request

* [reduce-locking] Move getNonce and getNonceFromResponse from jws struct cause they do not need access to it

* [reduce-locking] Extract nonceManager

* [reduce-locking] Add test that tries to show locking on http requests problem
---
 acme/client_test.go | 34 +++++++++++++++++++
 acme/jws.go         | 82 ++++++++++++++++++++++++++-------------------
 2 files changed, 82 insertions(+), 34 deletions(-)

diff --git a/acme/client_test.go b/acme/client_test.go
index e309554f..c4e3d6a1 100644
--- a/acme/client_test.go
+++ b/acme/client_test.go
@@ -10,6 +10,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 )
 
 func TestNewClient(t *testing.T) {
@@ -118,6 +119,39 @@ func TestClientOptPort(t *testing.T) {
 	}
 }
 
+func TestNotHoldingLockWhileMakingHTTPRequests(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(250 * time.Millisecond)
+		w.Header().Add("Replay-Nonce", "12345")
+		w.Header().Add("Retry-After", "0")
+		writeJSONResponse(w, &challenge{Type: "http-01", Status: "Valid", URI: "http://example.com/", Token: "token"})
+	}))
+	defer ts.Close()
+
+	privKey, _ := rsa.GenerateKey(rand.Reader, 512)
+	j := &jws{privKey: privKey, directoryURL: ts.URL}
+	ch := make(chan bool)
+	resultCh := make(chan bool)
+	go func() {
+		j.Nonce()
+		ch <- true
+	}()
+	go func() {
+		j.Nonce()
+		ch <- true
+	}()
+	go func() {
+		<-ch
+		<-ch
+		resultCh <- true
+	}()
+	select {
+	case <-resultCh:
+	case <-time.After(400 * time.Millisecond):
+		t.Fatal("JWS is probably holding a lock while making HTTP request")
+	}
+}
+
 func TestValidate(t *testing.T) {
 	var statuses []string
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
diff --git a/acme/jws.go b/acme/jws.go
index 2a1fc244..4818f17e 100644
--- a/acme/jws.go
+++ b/acme/jws.go
@@ -16,8 +16,7 @@ import (
 type jws struct {
 	directoryURL string
 	privKey      crypto.PrivateKey
-	nonces       []string
-	sync.Mutex
+	nonces       nonceManager
 }
 
 func keyAsJWK(key interface{}) *jose.JsonWebKey {
@@ -46,9 +45,10 @@ func (j *jws) post(url string, content []byte) (*http.Response, error) {
 		return nil, err
 	}
 
-	j.Lock()
-	defer j.Unlock()
-	j.getNonceFromResponse(resp)
+	nonce, nonceErr := getNonceFromResponse(resp)
+	if nonceErr == nil {
+		j.nonces.Push(nonce)
+	}
 
 	return resp, err
 }
@@ -80,38 +80,52 @@ func (j *jws) signContent(content []byte) (*jose.JsonWebSignature, error) {
 	return signed, nil
 }
 
-func (j *jws) getNonceFromResponse(resp *http.Response) error {
+func (j *jws) Nonce() (string, error) {
+	if nonce, ok := j.nonces.Pop(); ok {
+		return nonce, nil
+	}
+
+	return getNonce(j.directoryURL)
+}
+
+type nonceManager struct {
+	nonces []string
+	sync.Mutex
+}
+
+func (n *nonceManager) Pop() (string, bool) {
+	n.Lock()
+	defer n.Unlock()
+
+	if len(n.nonces) == 0 {
+		return "", false
+	}
+
+	nonce := n.nonces[len(n.nonces)-1]
+	n.nonces = n.nonces[:len(n.nonces)-1]
+	return nonce, true
+}
+
+func (n *nonceManager) Push(nonce string) {
+	n.Lock()
+	defer n.Unlock()
+	n.nonces = append(n.nonces, nonce)
+}
+
+func getNonce(url string) (string, error) {
+	resp, err := httpHead(url)
+	if err != nil {
+		return "", err
+	}
+
+	return getNonceFromResponse(resp)
+}
+
+func getNonceFromResponse(resp *http.Response) (string, error) {
 	nonce := resp.Header.Get("Replay-Nonce")
 	if nonce == "" {
-		return fmt.Errorf("Server did not respond with a proper nonce header.")
+		return "", fmt.Errorf("Server did not respond with a proper nonce header.")
 	}
 
-	j.nonces = append(j.nonces, nonce)
-	return nil
-}
-
-func (j *jws) getNonce() error {
-	resp, err := httpHead(j.directoryURL)
-	if err != nil {
-		return err
-	}
-
-	return j.getNonceFromResponse(resp)
-}
-
-func (j *jws) Nonce() (string, error) {
-	j.Lock()
-	defer j.Unlock()
-	nonce := ""
-	if len(j.nonces) == 0 {
-		err := j.getNonce()
-		if err != nil {
-			return nonce, err
-		}
-	}
-	if len(j.nonces) == 0 {
-		return "", fmt.Errorf("Can't get nonce")
-	}
-	nonce, j.nonces = j.nonces[len(j.nonces)-1], j.nonces[:len(j.nonces)-1]
 	return nonce, nil
 }