From eec23a3978adcfd26c29f4153eaa3e3d9b2cc53a Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 21 Dec 2020 17:57:36 +0100 Subject: [PATCH] acme: hardcode and remove ExternalAccountBinding.Algorithm HMAC-SHA256 is a perfectly fine MAC algorithm, and there is no need to ask the user to choose one. This does break compatibility with the previous API, but it had been live only for a weekend, so hopefully still in a window in which we can make changes with a limited blast radius. Updates golang/go#41430 Change-Id: I03741a545b25b9fcc147760cd20e9d7029844a6c Reviewed-on: https://go-review.googlesource.com/c/crypto/+/279453 Trust: Filippo Valsorda Run-TryBot: Filippo Valsorda TryBot-Result: Go Bot Reviewed-by: James Kasten Reviewed-by: Roland Shoemaker --- acme/jws.go | 57 ++++++++++++++++---------------------------- acme/jws_test.go | 53 ++++------------------------------------ acme/rfc8555.go | 5 +--- acme/rfc8555_test.go | 11 ++++----- acme/types.go | 5 +--- acme/types_test.go | 7 +++--- 6 files changed, 35 insertions(+), 103 deletions(-) diff --git a/acme/jws.go b/acme/jws.go index 04f509f..8c3ecce 100644 --- a/acme/jws.go +++ b/acme/jws.go @@ -11,27 +11,15 @@ import ( "crypto/rand" "crypto/rsa" "crypto/sha256" - "crypto/sha512" _ "crypto/sha512" // need for EC keys "encoding/asn1" "encoding/base64" "encoding/json" "errors" "fmt" - "hash" "math/big" ) -// MACAlgorithm represents a JWS MAC signature algorithm. -// See https://tools.ietf.org/html/rfc7518#section-3.1 for more details. -type MACAlgorithm string - -const ( - MACAlgorithmHS256 = MACAlgorithm("HS256") - MACAlgorithmHS384 = MACAlgorithm("HS384") - MACAlgorithmHS512 = MACAlgorithm("HS512") -) - // keyID is the account identity provided by a CA during registration. type keyID string @@ -101,24 +89,35 @@ func jwsEncodeJSON(claimset interface{}, key crypto.Signer, kid keyID, nonce, ur return json.Marshal(&enc) } -// jwsWithMAC creates and signs a JWS using the given key and algorithm. -// "rawProtected" and "rawPayload" should not be base64-URL-encoded. -func jwsWithMAC(key []byte, alg MACAlgorithm, rawProtected, rawPayload []byte) (*jsonWebSignature, error) { +// jwsWithMAC creates and signs a JWS using the given key and the HS256 +// algorithm. kid and url are included in the protected header. rawPayload +// should not be base64-URL-encoded. +func jwsWithMAC(key []byte, kid, url string, rawPayload []byte) (*jsonWebSignature, error) { if len(key) == 0 { return nil, errors.New("acme: cannot sign JWS with an empty MAC key") } + header := struct { + Algorithm string `json:"alg"` + KID string `json:"kid"` + URL string `json:"url,omitempty"` + }{ + // Only HMAC-SHA256 is supported. + Algorithm: "HS256", + KID: kid, + URL: url, + } + rawProtected, err := json.Marshal(header) + if err != nil { + return nil, err + } protected := base64.RawURLEncoding.EncodeToString(rawProtected) payload := base64.RawURLEncoding.EncodeToString(rawPayload) - // Only HMACs are currently supported. - hmac, err := newHMAC(key, alg) - if err != nil { + h := hmac.New(sha256.New, key) + if _, err := h.Write([]byte(protected + "." + payload)); err != nil { return nil, err } - if _, err := hmac.Write([]byte(protected + "." + payload)); err != nil { - return nil, err - } - mac := hmac.Sum(nil) + mac := h.Sum(nil) return &jsonWebSignature{ Protected: protected, @@ -218,20 +217,6 @@ func jwsHasher(pub crypto.PublicKey) (string, crypto.Hash) { return "", 0 } -// newHMAC returns an appropriate HMAC for the given MACAlgorithm. -func newHMAC(key []byte, alg MACAlgorithm) (hash.Hash, error) { - switch alg { - case MACAlgorithmHS256: - return hmac.New(sha256.New, key), nil - case MACAlgorithmHS384: - return hmac.New(sha512.New384, key), nil - case MACAlgorithmHS512: - return hmac.New(sha512.New, key), nil - default: - return nil, fmt.Errorf("acme: unsupported MAC algorithm: %v", alg) - } -} - // JWKThumbprint creates a JWK thumbprint out of pub // as specified in https://tools.ietf.org/html/rfc7638. func JWKThumbprint(pub crypto.PublicKey) (string, error) { diff --git a/acme/jws_test.go b/acme/jws_test.go index c6e3230..c8a1e8b 100644 --- a/acme/jws_test.go +++ b/acme/jws_test.go @@ -397,8 +397,6 @@ func TestJWSWithMAC(t *testing.T) { // Example from RFC 7520 Section 4.4.3. // https://tools.ietf.org/html/rfc7520#section-4.4.3 b64Key := "hJtXIZ2uSN5kbQfbtTNWbpdmhkV8FJG-Onbc6mxCcYg" - alg := MACAlgorithmHS256 - rawProtected := []byte(`{"alg":"HS256","kid":"018c0ae5-4d9b-471b-bfd6-eef314bc7037"}`) rawPayload := []byte("It\xe2\x80\x99s a dangerous business, Frodo, going out your " + "door. You step onto the road, and if you don't keep your feet, " + "there\xe2\x80\x99s no knowing where you might be swept off " + @@ -416,7 +414,7 @@ func TestJWSWithMAC(t *testing.T) { if err != nil { t.Fatalf("unable to decode key: %q", b64Key) } - got, err := jwsWithMAC(key, alg, rawProtected, rawPayload) + got, err := jwsWithMAC(key, "018c0ae5-4d9b-471b-bfd6-eef314bc7037", "", rawPayload) if err != nil { t.Fatalf("jwsWithMAC() = %q", err) } @@ -432,22 +430,9 @@ func TestJWSWithMAC(t *testing.T) { } func TestJWSWithMACError(t *testing.T) { - tt := []struct { - desc string - alg MACAlgorithm - key []byte - }{ - {"Unknown Algorithm", MACAlgorithm("UNKNOWN-ALG"), []byte("hmac-key")}, - {"Empty Key", MACAlgorithmHS256, nil}, - } - for _, tc := range tt { - tc := tc - t.Run(string(tc.desc), func(t *testing.T) { - p := "{}" - if _, err := jwsWithMAC(tc.key, tc.alg, []byte(p), []byte(p)); err == nil { - t.Errorf("jwsWithMAC(%v, %v, %s, %s) = success; want err", tc.key, tc.alg, p, p) - } - }) + p := "{}" + if _, err := jwsWithMAC(nil, "", "", []byte(p)); err == nil { + t.Errorf("jwsWithMAC(nil, ...) = success; want err") } } @@ -525,33 +510,3 @@ func TestJWKThumbprintErrUnsupportedKey(t *testing.T) { t.Errorf("err = %q; want %q", err, ErrUnsupportedKey) } } - -func TestNewHMAC(t *testing.T) { - tt := []struct { - alg MACAlgorithm - wantSize int - }{ - {MACAlgorithmHS256, 32}, - {MACAlgorithmHS384, 48}, - {MACAlgorithmHS512, 64}, - } - for _, tc := range tt { - tc := tc - t.Run(string(tc.alg), func(t *testing.T) { - h, err := newHMAC([]byte("key"), tc.alg) - if err != nil { - t.Fatalf("newHMAC(%v) = %q", tc.alg, err) - } - gotSize := len(h.Sum(nil)) - if gotSize != tc.wantSize { - t.Errorf("HMAC produced signature with unexpected length; got %d want %d", gotSize, tc.wantSize) - } - }) - } -} - -func TestNewHMACError(t *testing.T) { - if h, err := newHMAC([]byte("key"), MACAlgorithm("UNKNOWN-ALG")); err == nil { - t.Errorf("newHMAC(UNKNOWN-ALG) = %T, nil; want error", h) - } -} diff --git a/acme/rfc8555.go b/acme/rfc8555.go index ceb239d..073cee5 100644 --- a/acme/rfc8555.go +++ b/acme/rfc8555.go @@ -5,7 +5,6 @@ package acme import ( - "bytes" "context" "crypto" "encoding/base64" @@ -93,9 +92,7 @@ func (c *Client) encodeExternalAccountBinding(eab *ExternalAccountBinding) (*jso if err != nil { return nil, err } - var rProtected bytes.Buffer - fmt.Fprintf(&rProtected, `{"alg":%q,"kid":%q,"url":%q}`, eab.Algorithm, eab.KID, c.dir.RegURL) - return jwsWithMAC(eab.Key, eab.Algorithm, rProtected.Bytes(), []byte(jwk)) + return jwsWithMAC(eab.Key, eab.KID, c.dir.RegURL, []byte(jwk)) } // updateRegRFC is equivalent to c.UpdateReg but for CAs implementing RFC 8555. diff --git a/acme/rfc8555_test.go b/acme/rfc8555_test.go index b985b3e..c35118e 100644 --- a/acme/rfc8555_test.go +++ b/acme/rfc8555_test.go @@ -351,9 +351,8 @@ func TestRFC_Register(t *testing.T) { func TestRFC_RegisterExternalAccountBinding(t *testing.T) { eab := &ExternalAccountBinding{ - KID: "kid-1", - Key: []byte("secret"), - Algorithm: MACAlgorithmHS256, + KID: "kid-1", + Key: []byte("secret"), } type protected struct { @@ -403,10 +402,10 @@ func TestRFC_RegisterExternalAccountBinding(t *testing.T) { if prot.KID != eab.KID { t.Errorf("j.ExternalAccountBinding.KID = %s; want %s", prot.KID, eab.KID) } - // Ensure same Algorithm. - if prot.Algorithm != string(eab.Algorithm) { + // Ensure expected Algorithm. + if prot.Algorithm != "HS256" { t.Errorf("j.ExternalAccountBinding.Alg = %s; want %s", - prot.Algorithm, eab.Algorithm) + prot.Algorithm, "HS256") } // Ensure same URL as outer JWS. diff --git a/acme/types.go b/acme/types.go index 4d89fed..e751bf5 100644 --- a/acme/types.go +++ b/acme/types.go @@ -217,13 +217,10 @@ type ExternalAccountBinding struct { // Key is the bytes of the symmetric key that the CA provides to identify // the account. Key must correspond to the KID. Key []byte - - // Algorithm used to sign the JWS. - Algorithm MACAlgorithm } func (e *ExternalAccountBinding) String() string { - return fmt.Sprintf("&{KID: %q, Key: redacted, Algorithm: %v}", e.KID, e.Algorithm) + return fmt.Sprintf("&{KID: %q, Key: redacted}", e.KID) } // Directory is ACME server discovery data. diff --git a/acme/types_test.go b/acme/types_test.go index 25fa8b2..9ed7753 100644 --- a/acme/types_test.go +++ b/acme/types_test.go @@ -13,12 +13,11 @@ import ( func TestExternalAccountBindingString(t *testing.T) { eab := ExternalAccountBinding{ - KID: "kid", - Key: []byte("key"), - Algorithm: MACAlgorithmHS256, + KID: "kid", + Key: []byte("key"), } got := eab.String() - want := `&{KID: "kid", Key: redacted, Algorithm: HS256}` + want := `&{KID: "kid", Key: redacted}` if got != want { t.Errorf("eab.String() = %q, want: %q", got, want) }