acme: fix deadlock when Client.Key is nil

When methods that use POSTs are called on a acme.Client which has a
nil Key field it will cause a deadlock due to an infinite loop in
the code that looks up the account KID. This change adds a check for
the key being nil, and errors out if that is the case. Also adds a
test for this behavior.

Fixes golang/go#38790

Change-Id: I65ff6bbbade7ed2d85306895904a976089730bbf
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/233164
Trust: Roland Shoemaker <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: rolandshoemaker

acme/http.go

@@ -10,6 +10,7 @@ import (
 	"crypto"
 	"crypto/rand"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"math/big"
@@ -215,6 +216,9 @@ func (c *Client) post(ctx context.Context, key crypto.Signer, url string, body i
 func (c *Client) postNoRetry(ctx context.Context, key crypto.Signer, url string, body interface{}) (*http.Response, *http.Request, error) {
 	kid := noKeyID
 	if key == nil {
+		if c.Key == nil {
+			return nil, nil, errors.New("acme: Client.Key must be populated to make POST requests")
+		}
 		key = c.Key
 		kid = c.accountKID(ctx)
 	}

acme/http_test.go

@@ -238,3 +238,18 @@ func TestUserAgent(t *testing.T) {
 		}
 	}
 }
+
+func TestAccountKidLoop(t *testing.T) {
+	// if Client.postNoRetry is called with a nil key argument
+	// then Client.Key must be set, otherwise we fall into an
+	// infinite loop (which also causes a deadlock).
+	client := &Client{dir: &Directory{OrderURL: ":)"}}
+	_, _, err := client.postNoRetry(context.Background(), nil, "", nil)
+	if err == nil {
+		t.Fatal("Client.postNoRetry didn't fail with a nil key")
+	}
+	expected := "acme: Client.Key must be populated to make POST requests"
+	if err.Error() != expected {
+		t.Fatalf("Unexpected error returned: wanted %q, got %q", expected, err.Error())
+	}
+}