Improve EAB JWS validation and increase test coverage

acme/api/account.go

@@ -9,8 +9,7 @@ import (
 	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/api"
 	"github.com/smallstep/certificates/logging"
-
-	squarejose "gopkg.in/square/go-jose.v2"
+	"go.step.sm/crypto/jose"
 )
 
 // ExternalAccountBinding represents the ACME externalAccountBinding JWS
@@ -94,12 +93,6 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	eak, err := h.validateExternalAccountBinding(ctx, &nar)
-	if err != nil {
-		api.WriteError(w, err)
-		return
-	}
-
 	prov, err := acmeProvisionerFromContext(ctx)
 	if err != nil {
 		api.WriteError(w, err)
@@ -107,7 +100,7 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 	}
 
 	httpStatus := http.StatusCreated
-	acc, err := accountFromContext(r.Context())
+	acc, err := accountFromContext(ctx)
 	if err != nil {
 		acmeErr, ok := err.(*acme.Error)
 		if !ok || acmeErr.Status != http.StatusBadRequest {
@@ -122,12 +115,19 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 				"account does not exist"))
 			return
 		}
+
 		jwk, err := jwkFromContext(ctx)
 		if err != nil {
 			api.WriteError(w, err)
 			return
 		}
 
+		eak, err := h.validateExternalAccountBinding(ctx, &nar)
+		if err != nil {
+			api.WriteError(w, err)
+			return
+		}
+
 		acc = &acme.Account{
 			Key:     jwk,
 			Contact: nar.Contact,
@@ -137,6 +137,7 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 			api.WriteError(w, acme.WrapErrorISE(err, "error creating account"))
 			return
 		}
+
 		if eak != nil { // means that we have a (valid) External Account Binding key that should be bound, updated and sent in the response
 			err := eak.BindTo(acc)
 			if err != nil {
@@ -256,26 +257,27 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc
 		return nil, acme.NewError(acme.ErrorExternalAccountRequiredType, "no external account binding provided")
 	}
 
-	// TODO: extract the EAB in a similar manner as JWS, JWK, payload, etc? That would probably move a lot/all of
-	// the logic of this function into the middleware. Should not be too hard, because the middleware does know
-	// about the handler and thus about its dependencies.
 	eabJSONBytes, err := json.Marshal(nar.ExternalAccountBinding)
 	if err != nil {
 		return nil, acme.WrapErrorISE(err, "error marshaling externalAccountBinding into bytes")
 	}
 
-	eabJWS, err := squarejose.ParseSigned(string(eabJSONBytes))
+	eabJWS, err := jose.ParseJWS(string(eabJSONBytes))
 	if err != nil {
 		return nil, acme.WrapErrorISE(err, "error parsing externalAccountBinding jws")
 	}
 
-	// TODO: implement strategy pattern to allow for different ways of verification (i.e. webhook call) based on configuration
+	// TODO(hs): implement strategy pattern to allow for different ways of verification (i.e. webhook call) based on configuration?
+
+	keyID, acmeErr := validateEABJWS(ctx, eabJWS)
+	if acmeErr != nil {
+		return nil, acmeErr
+	}
 
-	keyID := eabJWS.Signatures[0].Protected.KeyID
 	externalAccountKey, err := h.db.GetExternalAccountKey(ctx, acmeProv.Name, keyID)
 	if err != nil {
 		if _, ok := err.(*acme.Error); ok {
-			return nil, err
+			return nil, acme.WrapError(acme.ErrorUnauthorizedType, err, "the field 'kid' references an unknown key")
 		}
 		return nil, acme.WrapErrorISE(err, "error retrieving external account key")
 	}
@@ -294,14 +296,14 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc
 		return nil, err
 	}
 
-	var payloadJWK *squarejose.JSONWebKey
+	var payloadJWK *jose.JSONWebKey
 	err = json.Unmarshal(payload, &payloadJWK)
 	if err != nil {
 		return nil, acme.WrapError(acme.ErrorMalformedType, err, "error unmarshaling payload into jwk")
 	}
 
 	if !keysAreEqual(jwk, payloadJWK) {
-		return nil, acme.NewError(acme.ErrorMalformedType, "keys in jws and eab payload do not match") // TODO: decide ACME error type to use
+		return nil, acme.NewError(acme.ErrorUnauthorizedType, "keys in jws and eab payload do not match")
 	}
 
 	return externalAccountKey, nil
@@ -309,7 +311,7 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc
 
 // keysAreEqual performs an equality check on two JWKs by comparing
 // the (base64 encoding) of the Key IDs.
-func keysAreEqual(x, y *squarejose.JSONWebKey) bool {
+func keysAreEqual(x, y *jose.JSONWebKey) bool {
 	if x == nil || y == nil {
 		return false
 	}
@@ -320,3 +322,62 @@ func keysAreEqual(x, y *squarejose.JSONWebKey) bool {
 	}
 	return digestX == digestY
 }
+
+// validateEABJWS verifies the contents of the External Account Binding JWS.
+// The protected header of the JWS MUST meet the following criteria:
+// 	o 	The "alg" field MUST indicate a MAC-based algorithm
+//	o  	The "kid" field MUST contain the key identifier provided by the CA
+//	o  	The "nonce" field MUST NOT be present
+//	o  	The "url" field MUST be set to the same value as the outer JWS
+func validateEABJWS(ctx context.Context, jws *jose.JSONWebSignature) (string, *acme.Error) {
+
+	if jws == nil {
+		return "", acme.NewErrorISE("no JWS provided")
+	}
+
+	if len(jws.Signatures) != 1 {
+		return "", acme.NewError(acme.ErrorMalformedType, "JWS must have one signature")
+	}
+
+	header := jws.Signatures[0].Protected
+	algorithm := header.Algorithm
+	keyID := header.KeyID
+	nonce := header.Nonce
+
+	if !(algorithm == jose.HS256 || algorithm == jose.HS384 || algorithm == jose.HS512) {
+		return "", acme.NewError(acme.ErrorMalformedType, "'alg' field set to invalid algorithm '%s'", algorithm)
+	}
+
+	if keyID == "" {
+		return "", acme.NewError(acme.ErrorMalformedType, "'kid' field is required")
+	}
+
+	if nonce != "" {
+		return "", acme.NewError(acme.ErrorMalformedType, "'nonce' must not be present")
+	}
+
+	jwsURL, ok := header.ExtraHeaders["url"]
+	if !ok {
+		return "", acme.NewError(acme.ErrorMalformedType, "'url' field is required")
+	}
+
+	outerJWS, err := jwsFromContext(ctx)
+	if err != nil {
+		return "", acme.WrapErrorISE(err, "could not retrieve outer JWS from context")
+	}
+
+	if len(outerJWS.Signatures) != 1 {
+		return "", acme.NewError(acme.ErrorMalformedType, "outer JWS must have one signature")
+	}
+
+	outerJWSURL, ok := outerJWS.Signatures[0].Protected.ExtraHeaders["url"]
+	if !ok {
+		return "", acme.NewError(acme.ErrorMalformedType, "'url' field must be set in outer JWS")
+	}
+
+	if jwsURL != outerJWSURL {
+		return "", acme.NewError(acme.ErrorMalformedType, "'url' field is not the same value as the outer JWS")
+	}
+
+	return keyID, nil
+}