diff --git a/wfe2/verify.go b/wfe2/verify.go index e86d8ccc8..79012967d 100644 --- a/wfe2/verify.go +++ b/wfe2/verify.go @@ -179,7 +179,8 @@ func (wfe *WebFrontEndImpl) validNonce(jws *jose.JSONWebSignature, logEvent *req // validPOSTURL checks the JWS' URL header against the expected URL based on the // HTTP request. This prevents a JWS intended for one endpoint being replayed -// against a different endpoint. +// against a different endpoint. If the URL isn't present, is invalid, or +// doesn't match the HTTP request a problem is returned. func (wfe *WebFrontEndImpl) validPOSTURL( request *http.Request, jws *jose.JSONWebSignature) *probs.ProblemDetails { @@ -214,24 +215,41 @@ func (wfe *WebFrontEndImpl) validPOSTURL( return nil } -// parseJWS extracts a JSONWebSignature from an HTTP POST request's body. If -// there is an error reading the JWS or it is unacceptable (e.g. too many/too -// few signatures, presence of unprotected headers) a problem is returned, -// otherwise the parsed *JSONWebSignature is returned. -func (wfe *WebFrontEndImpl) parseJWS(request *http.Request) (*jose.JSONWebSignature, *probs.ProblemDetails) { - // Verify that the POST request has the expected headers - if prob := wfe.validPOSTRequest(request); prob != nil { - return nil, prob +// matchJWSURLs checks two JWS' URL headers are equal. This is used during key +// rollover to check that the inner JWS URL matches the outer JWS URL. If the +// JWS URLs do not match a problem is returned. +func matchJWSURLs(outer, inner *jose.JSONWebSignature) *probs.ProblemDetails { + // Verify that the outer JWS has a non-empty URL header. This is strictly + // defensive since the expectation is that endpoints using `matchJWSURLs` + // have received at least one of their JWS from calling validPOSTForAccount(), + // which checks the outer JWS has the expected URL header before processing + // the inner JWS. + outerURL, ok := outer.Signatures[0].Header.ExtraHeaders[jose.HeaderKey("url")].(string) + if !ok || len(outerURL) == 0 { + return probs.Malformed("Outer JWS header parameter 'url' required") } - // Read the POST request body's bytes. validPOSTRequest has already checked - // that the body is non-nil - bodyBytes, err := ioutil.ReadAll(request.Body) - if err != nil { - wfe.stats.Inc("Errors.UnableToReadRequestBody", 1) - return nil, probs.ServerInternal("unable to read request body") + // Verify the inner JWS has a non-empty URL header. + innerURL, ok := inner.Signatures[0].Header.ExtraHeaders[jose.HeaderKey("url")].(string) + if !ok || len(innerURL) == 0 { + return probs.Malformed("Inner JWS header parameter 'url' required") } + // Verify that the outer URL matches the inner URL + if outerURL != innerURL { + return probs.Malformed(fmt.Sprintf( + "Outer JWS 'url' value %q does not match inner JWS 'url' value %q", + outerURL, innerURL)) + } + + return nil +} + +// parseJWS extracts a JSONWebSignature from a byte slice. If there is an error +// reading the JWS or it is unacceptable (e.g. too many/too few signatures, +// presence of unprotected headers) a problem is returned, otherwise the parsed +// *JSONWebSignature is returned. +func (wfe *WebFrontEndImpl) parseJWS(body []byte) (*jose.JSONWebSignature, *probs.ProblemDetails) { // Parse the raw JWS JSON to check that: // * the unprotected Header field is not being used. // * the "signatures" member isn't present, just "signature". @@ -242,7 +260,7 @@ func (wfe *WebFrontEndImpl) parseJWS(request *http.Request) (*jose.JSONWebSignat Header map[string]string Signatures []interface{} } - if err := json.Unmarshal(bodyBytes, &unprotected); err != nil { + if err := json.Unmarshal(body, &unprotected); err != nil { wfe.stats.Inc("Errors.JWSParseError", 1) return nil, probs.Malformed("Parse error reading JWS") } @@ -265,8 +283,8 @@ func (wfe *WebFrontEndImpl) parseJWS(request *http.Request) (*jose.JSONWebSignat // Parse the JWS using go-jose and enforce that the expected one non-empty // signature is present in the parsed JWS. - body := string(bodyBytes) - parsedJWS, err := jose.ParseSigned(body) + bodyStr := string(body) + parsedJWS, err := jose.ParseSigned(bodyStr) if err != nil { wfe.stats.Inc("Errors.JWSParseError", 1) return nil, probs.Malformed("Parse error reading JWS") @@ -287,6 +305,24 @@ func (wfe *WebFrontEndImpl) parseJWS(request *http.Request) (*jose.JSONWebSignat return parsedJWS, nil } +// parseJWSRequest extracts a JSONWebSignature from an HTTP POST request's body using parseJWS. +func (wfe *WebFrontEndImpl) parseJWSRequest(request *http.Request) (*jose.JSONWebSignature, *probs.ProblemDetails) { + // Verify that the POST request has the expected headers + if prob := wfe.validPOSTRequest(request); prob != nil { + return nil, prob + } + + // Read the POST request body's bytes. validPOSTRequest has already checked + // that the body is non-nil + bodyBytes, err := ioutil.ReadAll(request.Body) + if err != nil { + wfe.stats.Inc("Errors.UnableToReadRequestBody", 1) + return nil, probs.ServerInternal("unable to read request body") + } + + return wfe.parseJWS(bodyBytes) +} + // extractJWK extracts a JWK from a provided JWS or returns a problem. It // expects that the JWS is using the embedded JWK style of authentication and // does not contain an embedded Key ID. Callers should have acquired the @@ -332,8 +368,6 @@ func (wfe *WebFrontEndImpl) lookupJWK( return nil, nil, prob } - // lookupJWK is called after parseJWS() which defends against the - // incorrect number of signatures. header := jws.Signatures[0].Header accountURL := header.KeyID prefix := wfe.relativeEndpoint(request, regPath) @@ -438,31 +472,32 @@ func (wfe *WebFrontEndImpl) validJWSForKey( // specified by the JWS key ID. If the request is valid (e.g. the JWS is well // formed, verifies with the JWK stored for the specified key ID, specifies the // correct URL, and has a valid nonce) then `validPOSTForAccount` returns the -// validated JWS body and a pointer to the JWK's associated account. If any of -// these conditions are not met or an error occurs only a problem is returned. +// validated JWS body, the parsed JSONWebSignature, and a pointer to the JWK's +// associated account. If any of these conditions are not met or an error +// occurs only a problem is returned. func (wfe *WebFrontEndImpl) validPOSTForAccount( request *http.Request, ctx context.Context, - logEvent *requestEvent) ([]byte, *core.Registration, *probs.ProblemDetails) { + logEvent *requestEvent) ([]byte, *jose.JSONWebSignature, *core.Registration, *probs.ProblemDetails) { // Parse the JWS from the POST request - jws, prob := wfe.parseJWS(request) + jws, prob := wfe.parseJWSRequest(request) if prob != nil { - return nil, nil, prob + return nil, nil, nil, prob } // Lookup the account and JWK for the key ID that authenticated the JWS pubKey, account, prob := wfe.lookupJWK(jws, ctx, request, logEvent) if prob != nil { - return nil, nil, prob + return nil, nil, nil, prob } // Verify the JWS with the JWK from the SA payload, prob := wfe.validJWSForKey(jws, pubKey, request, logEvent) if prob != nil { - return nil, nil, prob + return nil, nil, nil, prob } - return payload, account, nil + return payload, jws, account, nil } // validSelfAuthenticatedPOST checks that a given POST request has a valid JWS @@ -481,7 +516,7 @@ func (wfe *WebFrontEndImpl) validSelfAuthenticatedPOST( logEvent *requestEvent) ([]byte, *jose.JSONWebKey, *probs.ProblemDetails) { // Parse the JWS from the POST request - jws, prob := wfe.parseJWS(request) + jws, prob := wfe.parseJWSRequest(request) if prob != nil { return nil, nil, prob } @@ -506,3 +541,82 @@ func (wfe *WebFrontEndImpl) validSelfAuthenticatedPOST( return payload, pubKey, nil } + +// rolloverRequest is a struct representing an ACME key rollover request +type rolloverRequest struct { + NewKey jose.JSONWebKey + Account string +} + +// validKeyRollover checks if the innerJWS is a valid key rollover operation +// given the outer JWS that carried it. It is assumed that the outerJWS has +// already been validated per the normal ACME process using `validPOSTForAccount`. +// It is *critical* this is the case since `validKeyRollover` does not check the +// outerJWS signature. This function checks that: +// 1) the inner JWS is valid and well formed +// 2) the inner JWS has the same "url" header as the outer JWS +// 3) the inner JWS is self-authenticated with an embedded JWK +// 4) the payload of the inner JWS is a valid JSON key change object +// 5) that the key specified in the key change object *also* verifies the inner JWS +// A *rolloverRequest object and is returned if successfully validated, +// otherwise a problem is returned. The caller is left to verify whether the +// new key is appropriate (e.g. isn't being used by another existing account) +// and that the account field of the rollover object matches the account that +// verified the outer JWS. +func (wfe *WebFrontEndImpl) validKeyRollover( + outerJWS *jose.JSONWebSignature, + innerJWS *jose.JSONWebSignature, + logEvent *requestEvent) (*rolloverRequest, *probs.ProblemDetails) { + + // Extract the embedded JWK from the inner JWS + jwk, prob := wfe.extractJWK(innerJWS) + if prob != nil { + return nil, prob + } + + // If the key doesn't meet the GoodKey policy return a problem immediately + if err := wfe.keyPolicy.GoodKey(jwk.Key); err != nil { + wfe.stats.Inc("Errors.InnerJWKRejectedByGoodKey", 1) + return nil, probs.Malformed(err.Error()) + } + + // Check that the public key and JWS algorithms match expected + if err := checkAlgorithm(jwk, innerJWS); err != nil { + return nil, probs.Malformed(err.Error()) + } + + // Verify the inner JWS signature with the public key from the embedded JWK. + // NOTE(@cpu): We do not use `wfe.validJWSForKey` here because the inner JWS + // of a key rollover operation is special (e.g. has no nonce, doesn't have an + // HTTP request to match the URL to) + innerPayload, err := innerJWS.Verify(jwk) + if err != nil { + wfe.stats.Inc("Errors.InnerJWSVerificationFailed", 1) + return nil, probs.Malformed("Inner JWS does not verify with embedded JWK") + } + // NOTE(@cpu): we do not stomp the requestEvent's payload here since that is set + // from the outerJWS in validPOSTForAccount and contains the inner JWS and inner + // payload already. + + // Verify that the outer and inner JWS protected URL headers match + if matchJWSURLs(outerJWS, innerJWS) != nil { + return nil, prob + } + + // Unmarshal the inner JWS' key roll over request + var req rolloverRequest + if json.Unmarshal(innerPayload, &req) != nil { + return nil, probs.Malformed( + "Inner JWS payload did not parse as JSON key rollover object") + } + + // Verify that the key roll over request's NewKey *also* validates the inner + // JWS. So far we've only checked that the JWK embedded in the inner JWS valides + // the JWS. + if _, err := innerJWS.Verify(req.NewKey); err != nil { + wfe.stats.Inc("Errors.InnerJWSVerificationFailed", 1) + return nil, probs.Malformed("Inner JWS does not verify with specified new key") + } + + return &req, nil +} diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go index 32576e98f..54bd08c33 100644 --- a/wfe2/verify_test.go +++ b/wfe2/verify_test.go @@ -78,7 +78,7 @@ func signRequestEmbed( // if no key is provided default to test1KeyPrivatePEM var publicKey interface{} if privateKey == nil { - signer := loadKeys(t, []byte(test1KeyPrivatePEM)) + signer := loadKey(t, []byte(test1KeyPrivatePEM)) privateKey = signer publicKey = signer.Public() } else { @@ -93,9 +93,11 @@ func signRequestEmbed( opts := &jose.SignerOptions{ NonceSource: nonceService, EmbedJWK: true, - ExtraHeaders: map[jose.HeaderKey]interface{}{ + } + if url != "" { + opts.ExtraHeaders = map[jose.HeaderKey]interface{}{ "url": url, - }, + } } signer, err := jose.NewSigner(signerKey, opts) @@ -124,13 +126,13 @@ func signRequestKeyID( nonceService jose.NonceSource) (*jose.JSONWebSignature, *jose.JSONWebKey, string) { // if no key is provided default to test1KeyPrivatePEM if privateKey == nil { - privateKey = loadKeys(t, []byte(test1KeyPrivatePEM)) + privateKey = loadKey(t, []byte(test1KeyPrivatePEM)) } jwk := &jose.JSONWebKey{ Key: privateKey, Algorithm: keyAlgForKey(t, privateKey), - KeyID: fmt.Sprintf("%d", keyID), + KeyID: fmt.Sprintf("http://localhost/acme/reg/%d", keyID), } signerKey := jose.SigningKey{ @@ -567,7 +569,7 @@ func signExtraHeaders( t *testing.T, headers map[jose.HeaderKey]interface{}, nonceService jose.NonceSource) (*jose.JSONWebSignature, string) { - privateKey := loadKeys(t, []byte(test1KeyPrivatePEM)) + privateKey := loadKey(t, []byte(test1KeyPrivatePEM)) signerKey := jose.SigningKey{ Key: privateKey, @@ -674,8 +676,8 @@ func TestValidPOSTURL(t *testing.T) { } func multiSigJWS(t *testing.T, nonceService jose.NonceSource) (*jose.JSONWebSignature, string) { - privateKeyA := loadKeys(t, []byte(test1KeyPrivatePEM)) - privateKeyB := loadKeys(t, []byte(test2KeyPrivatePEM)) + privateKeyA := loadKey(t, []byte(test1KeyPrivatePEM)) + privateKeyB := loadKey(t, []byte(test2KeyPrivatePEM)) signerKeyA := jose.SigningKey{ Key: privateKeyA, @@ -705,7 +707,7 @@ func multiSigJWS(t *testing.T, nonceService jose.NonceSource) (*jose.JSONWebSign return parsedJWS, body } -func TestParseJWS(t *testing.T) { +func TestParseJWSRequest(t *testing.T) { wfe, _ := setupWFE(t) _, tooManySigsJWSBody := multiSigJWS(t, wfe.nonceService) @@ -808,7 +810,7 @@ func TestParseJWS(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - _, prob := wfe.parseJWS(tc.Request) + _, prob := wfe.parseJWSRequest(tc.Request) if tc.ExpectedProblem == nil && prob != nil { t.Fatal(fmt.Sprintf("Expected nil problem, got %#v\n", prob)) } else { @@ -861,7 +863,7 @@ func TestExtractJWK(t *testing.T) { } func signRequestBadKeyID(t *testing.T, nonceService jose.NonceSource) (*jose.JSONWebSignature, string) { - privateKey := loadKeys(t, []byte(test1KeyPrivatePEM)) + privateKey := loadKey(t, []byte(test1KeyPrivatePEM)) jwk := &jose.JSONWebKey{ Key: privateKey, @@ -945,7 +947,7 @@ func TestLookupJWK(t *testing.T) { Request: makePostRequestWithPath("test-path", errorIDJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.ServerInternalProblem, - Detail: "Error retreiving account \"100\"", + Detail: "Error retreiving account \"http://localhost/acme/reg/100\"", HTTPStatus: http.StatusInternalServerError, }, }, @@ -955,7 +957,7 @@ func TestLookupJWK(t *testing.T) { Request: makePostRequestWithPath("test-path", missingIDJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.AccountDoesNotExistProblem, - Detail: "Account \"102\" not found", + Detail: "Account \"http://localhost/acme/reg/102\" not found", HTTPStatus: http.StatusBadRequest, }, }, @@ -1120,14 +1122,14 @@ func TestValidJWSForKey(t *testing.T) { func TestValidPOSTForAccount(t *testing.T) { wfe, _ := setupWFE(t) - _, _, validJWSBody := signRequestKeyID(t, 1, nil, "http://localhost/test", `{"test":"passed"}`, wfe.nonceService) + validJWS, _, validJWSBody := signRequestKeyID(t, 1, nil, "http://localhost/test", `{"test":"passed"}`, wfe.nonceService) validAccount, _ := wfe.SA.GetRegistration(context.Background(), 1) // ID 102 is mocked to return missing _, _, missingJWSBody := signRequestKeyID(t, 102, nil, "http://localhost/test", "{}", wfe.nonceService) // ID 3 is mocked to return deactivated - key3 := loadKeys(t, []byte(test3KeyPrivatePEM)) + key3 := loadKey(t, []byte(test3KeyPrivatePEM)) _, _, deactivatedJWSBody := signRequestKeyID(t, 3, key3, "http://localhost/test", "{}", wfe.nonceService) _, _, embeddedJWSBody := signRequestEmbed(t, nil, "http://localhost/test", `{"test":"passed"}`, wfe.nonceService) @@ -1138,6 +1140,7 @@ func TestValidPOSTForAccount(t *testing.T) { ExpectedProblem *probs.ProblemDetails ExpectedPayload string ExpectedAcct *core.Registration + ExpectedJWS *jose.JSONWebSignature }{ { Name: "Invalid JWS", @@ -1162,7 +1165,7 @@ func TestValidPOSTForAccount(t *testing.T) { Request: makePostRequestWithPath("test", missingJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.AccountDoesNotExistProblem, - Detail: "Account \"102\" not found", + Detail: "Account \"http://localhost/acme/reg/102\" not found", HTTPStatus: http.StatusBadRequest, }, }, @@ -1180,6 +1183,7 @@ func TestValidPOSTForAccount(t *testing.T) { Request: makePostRequestWithPath("test", validJWSBody), ExpectedPayload: `{"test":"passed"}`, ExpectedAcct: &validAccount, + ExpectedJWS: validJWS, }, } @@ -1187,7 +1191,7 @@ func TestValidPOSTForAccount(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { inputLogEvent := newRequestEvent() - outPayload, acct, prob := wfe.validPOSTForAccount(tc.Request, context.Background(), inputLogEvent) + outPayload, jws, acct, prob := wfe.validPOSTForAccount(tc.Request, context.Background(), inputLogEvent) if tc.ExpectedProblem == nil && prob != nil { t.Fatal(fmt.Sprintf("Expected nil problem, got %#v\n", prob)) @@ -1195,6 +1199,7 @@ func TestValidPOSTForAccount(t *testing.T) { test.AssertEquals(t, inputLogEvent.Payload, tc.ExpectedPayload) test.AssertEquals(t, string(outPayload), tc.ExpectedPayload) test.AssertMarshaledEquals(t, acct, tc.ExpectedAcct) + test.AssertMarshaledEquals(t, jws, tc.ExpectedJWS) } else { test.AssertMarshaledEquals(t, prob, tc.ExpectedProblem) } @@ -1234,7 +1239,7 @@ func TestValidPOSTForAccountSwappedKey(t *testing.T) { // Ensure that ValidPOSTForAccount produces an error since the // mockSADifferentStoredKey will return a different key than the one we used to // sign the request - _, _, prob := wfe.validPOSTForAccount(request, ctx, event) + _, _, _, prob := wfe.validPOSTForAccount(request, ctx, event) test.Assert(t, prob != nil, "No error returned for request signed by wrong key") test.AssertEquals(t, prob.Type, probs.MalformedProblem) test.AssertEquals(t, prob.Detail, "JWS verification error") @@ -1301,3 +1306,109 @@ func TestValidSelfAuthenticatedPOST(t *testing.T) { }) } } + +func TestMatchJWSURLs(t *testing.T) { + wfe, _ := setupWFE(t) + + noURLJWS, _, _ := signRequestEmbed(t, nil, "", "", wfe.nonceService) + urlAJWS, _, _ := signRequestEmbed(t, nil, "example.com", "", wfe.nonceService) + urlBJWS, _, _ := signRequestEmbed(t, nil, "example.org", "", wfe.nonceService) + + testCases := []struct { + Name string + Outer *jose.JSONWebSignature + Inner *jose.JSONWebSignature + ExpectedProblem *probs.ProblemDetails + }{ + { + Name: "Outer JWS without URL", + Outer: noURLJWS, + Inner: urlAJWS, + ExpectedProblem: &probs.ProblemDetails{ + Type: probs.MalformedProblem, + Detail: "Outer JWS header parameter 'url' required", + HTTPStatus: http.StatusBadRequest, + }, + }, + { + Name: "Inner JWS without URL", + Outer: urlAJWS, + Inner: noURLJWS, + ExpectedProblem: &probs.ProblemDetails{ + Type: probs.MalformedProblem, + Detail: "Inner JWS header parameter 'url' required", + HTTPStatus: http.StatusBadRequest, + }, + }, + { + Name: "Inner and outer JWS without URL", + Outer: noURLJWS, + Inner: noURLJWS, + ExpectedProblem: &probs.ProblemDetails{ + Type: probs.MalformedProblem, + // The Outer JWS is validated first + Detail: "Outer JWS header parameter 'url' required", + HTTPStatus: http.StatusBadRequest, + }, + }, + { + Name: "Mismatched inner and outer JWS URLs", + Outer: urlAJWS, + Inner: urlBJWS, + ExpectedProblem: &probs.ProblemDetails{ + Type: probs.MalformedProblem, + Detail: "Outer JWS 'url' value \"example.com\" does not match inner JWS 'url' value \"example.org\"", + HTTPStatus: http.StatusBadRequest, + }, + }, + { + Name: "Matching inner and outer JWS URLs", + Outer: urlAJWS, + Inner: urlAJWS, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + prob := matchJWSURLs(tc.Outer, tc.Inner) + if prob != nil && tc.ExpectedProblem == nil { + t.Errorf("matchJWSURLs failed. Expected no problem, got %#v", prob) + } else { + test.AssertMarshaledEquals(t, prob, tc.ExpectedProblem) + } + }) + } +} + +func TestValidKeyRollover(t *testing.T) { + + testCases := []struct { + Name string + OuterJWS *jose.JSONWebSignature + InnerJWS *jose.JSONWebSignature + ExpectedRollover *rolloverRequest + ExpectedProblem *probs.ProblemDetails + }{ + { + Name: "Inner JWK with broken signature", + }, + { + Name: "Mismatched outer/inner JWK URLs", + }, + { + Name: "Invalid inner JWS payload", + }, + { + Name: "Rollover key doesn't validate inner JWS", + }, + { + Name: "Good rollover request", + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + + }) + } +} diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 04537a365..424cca1d2 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -670,7 +670,7 @@ func (wfe *WebFrontEndImpl) postChallenge( authz core.Authorization, challengeIndex int, logEvent *requestEvent) { - body, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors @@ -735,7 +735,7 @@ func (wfe *WebFrontEndImpl) Registration( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - body, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors @@ -836,7 +836,7 @@ func (wfe *WebFrontEndImpl) deactivateAuthorization( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) bool { - body, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { wfe.sendError(response, logEvent, prob, nil) @@ -1053,12 +1053,82 @@ func (wfe *WebFrontEndImpl) setCORSHeaders(response http.ResponseWriter, request } // KeyRollover allows a user to change their signing key -func (wfe *WebFrontEndImpl) KeyRollover(ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - // KeyRollover is a NOP for WFEv2 at the present time. It needs unique JWS - // validation compared to other ACME v2 endpoints. - wfe.sendError(response, logEvent, - probs.ServerInternal("KeyRollover is not presently implemented for ACME v2"), nil) - return +func (wfe *WebFrontEndImpl) KeyRollover( + ctx context.Context, + logEvent *requestEvent, + response http.ResponseWriter, + request *http.Request) { + // Validate the outer JWS on the key rollover in standard fashion using + // validPOSTForAccount + outerBody, outerJWS, acct, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + addRequesterHeader(response, logEvent.Requester) + if prob != nil { + wfe.sendError(response, logEvent, prob, nil) + return + } + + // Parse the inner JWS from the validated outer JWS body + innerJWS, prob := wfe.parseJWS(outerBody) + if prob != nil { + wfe.sendError(response, logEvent, prob, nil) + return + } + + // Validate the inner JWS as a key rollover request for the outer JWS + rolloverRequest, prob := wfe.validKeyRollover(outerJWS, innerJWS, logEvent) + if prob != nil { + wfe.sendError(response, logEvent, prob, nil) + return + } + newKey := rolloverRequest.NewKey + + // Check that the rollover request's account URL matches the account URL used + // to validate the outer JWS + header := outerJWS.Signatures[0].Header + if rolloverRequest.Account != header.KeyID { + wfe.sendError(response, logEvent, probs.Malformed( + fmt.Sprintf("Inner key rollover request specified Account %q, but outer JWS has Key ID %q", + rolloverRequest.Account, header.KeyID)), nil) + return + } + + // Check that the new key isn't the same as the old key. This would fail as + // part of the subsequent `wfe.SA.GetRegistrationByKey` check since the new key + // will find the old registration if its equal to the old registration key. We + // check new key against old key explicitly to save an RPC round trip and a DB + // query for this easy rejection case + keysEqual, err := core.PublicKeysEqual(newKey.Key, acct.Key.Key) + if err != nil { + // This should not happen - both the old and new key have been validated by now + wfe.sendError(response, logEvent, probs.ServerInternal("Unable to compare new and old keys"), nil) + return + } + if keysEqual { + wfe.sendError(response, logEvent, probs.Malformed( + "New key specified by rollover request is the same as the old key"), nil) + return + } + + // Check that the new key isn't already being used for an existing account + if existingAcct, err := wfe.SA.GetRegistrationByKey(ctx, &newKey); err != nil { + response.Header().Set("Location", wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", regPath, existingAcct.ID))) + wfe.sendError(response, logEvent, probs.Conflict("New key is already in use for a different account"), err) + return + } + + // Update the account key to the new key + updatedAcct, err := wfe.RA.UpdateRegistration(ctx, *acct, core.Registration{Key: &newKey}) + if err != nil { + wfe.sendError(response, logEvent, + problemDetailsForError(err, "Unable to update account with new key"), err) + return + } + + err = wfe.writeJsonResponse(response, logEvent, http.StatusOK, updatedAcct) + if err != nil { + logEvent.AddError("failed to marshal updated account: %q", err) + wfe.sendError(response, logEvent, probs.ServerInternal("Failed to marshal updated account"), err) + } } func (wfe *WebFrontEndImpl) deactivateRegistration(ctx context.Context, reg core.Registration, response http.ResponseWriter, request *http.Request, logEvent *requestEvent) { @@ -1103,7 +1173,7 @@ type orderJSON struct { // NewOrder is used by clients to create a new order object from a CSR func (wfe *WebFrontEndImpl) NewOrder(ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - body, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 742b3ddd4..d986ffde2 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -283,9 +283,9 @@ func makeBody(s string) io.ReadCloser { return ioutil.NopCloser(strings.NewReader(s)) } -// loadKeys loads a private key from PEM/DER-encoded data and returns +// loadKey loads a private key from PEM/DER-encoded data and returns // a `crypto.Signer`. -func loadKeys(t *testing.T, keyBytes []byte) crypto.Signer { +func loadKey(t *testing.T, keyBytes []byte) crypto.Signer { // pem.Decode does not return an error as its 2nd arg, but instead the "rest" // that was leftover from parsing the PEM block. We only care if the decoded // PEM block was empty for this test function. @@ -972,7 +972,7 @@ func TestChallenge(t *testing.T) { func TestBadNonce(t *testing.T) { wfe, _ := setupWFE(t) - key := loadKeys(t, []byte(test2KeyPrivatePEM)) + key := loadKey(t, []byte(test2KeyPrivatePEM)) rsaKey, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") // NOTE: We deliberately do not set the NonceSource in the jose.SignerOptions @@ -997,7 +997,7 @@ func TestNewECDSARegistration(t *testing.T) { wfe, _ := setupWFE(t) // E1 always exists; E2 never exists - key := loadKeys(t, []byte(testE2KeyPrivatePEM)) + key := loadKey(t, []byte(testE2KeyPrivatePEM)) _, ok := key.(*ecdsa.PrivateKey) test.Assert(t, ok, "Couldn't load ECDSA key") @@ -1021,7 +1021,7 @@ func TestNewECDSARegistration(t *testing.T) { test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/reg/0") - key = loadKeys(t, []byte(testE1KeyPrivatePEM)) + key = loadKey(t, []byte(testE1KeyPrivatePEM)) _, ok = key.(*ecdsa.PrivateKey) test.Assert(t, ok, "Couldn't load ECDSA key") @@ -1049,7 +1049,7 @@ func TestEmptyRegistration(t *testing.T) { // Test Key 1 is mocked in the mock StorageAuthority used in setupWFE to // return a populated registration for GetRegistrationByKey when test key 1 is // used. - key := loadKeys(t, []byte(test1KeyPrivatePEM)) + key := loadKey(t, []byte(test1KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") @@ -1083,7 +1083,7 @@ func TestEmptyRegistration(t *testing.T) { func TestNewRegistration(t *testing.T) { wfe, _ := setupWFE(t) mux := wfe.Handler() - key := loadKeys(t, []byte(test2KeyPrivatePEM)) + key := loadKey(t, []byte(test2KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load test2 key") @@ -1168,7 +1168,7 @@ func TestNewRegistration(t *testing.T) { links := responseWriter.Header()["Link"] test.AssertEquals(t, contains(links, "<"+agreementURL+">;rel=\"terms-of-service\""), true) - key = loadKeys(t, []byte(test1KeyPrivatePEM)) + key = loadKey(t, []byte(test1KeyPrivatePEM)) _, ok = key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load test1 key") @@ -1269,7 +1269,7 @@ func TestRegistration(t *testing.T) { `{"type":"urn:acme:error:malformed","detail":"Parse error reading JWS","status":400}`) responseWriter.Body.Reset() - key := loadKeys(t, []byte(test2KeyPrivatePEM)) + key := loadKey(t, []byte(test2KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") @@ -1284,10 +1284,10 @@ func TestRegistration(t *testing.T) { wfe.Registration(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), - `{"type":"urn:ietf:params:acme:error:accountDoesNotExist","detail":"Account \"102\" not found","status":400}`) + `{"type":"urn:ietf:params:acme:error:accountDoesNotExist","detail":"Account \"http://localhost/acme/reg/102\" not found","status":400}`) responseWriter.Body.Reset() - key = loadKeys(t, []byte(test1KeyPrivatePEM)) + key = loadKey(t, []byte(test1KeyPrivatePEM)) _, ok = key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") @@ -1525,7 +1525,7 @@ func TestHeaderBoulderRequester(t *testing.T) { mux := wfe.Handler() responseWriter := httptest.NewRecorder() - key := loadKeys(t, []byte(test1KeyPrivatePEM)) + key := loadKey(t, []byte(test1KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Failed to load test 1 RSA key") @@ -1652,7 +1652,7 @@ func TestDeactivateRegistration(t *testing.T) { }`) responseWriter.Body.Reset() - key := loadKeys(t, []byte(test3KeyPrivatePEM)) + key := loadKey(t, []byte(test3KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load test3 RSA key") @@ -1751,3 +1751,100 @@ func TestNewOrder(t *testing.T) { }) } } + +func TestKeyRollover(t *testing.T) { + responseWriter := httptest.NewRecorder() + wfe, _ := setupWFE(t) + + newKey, err := rsa.GenerateKey(rand.Reader, 2048) + test.AssertNotError(t, err, "Error creating random 2048 RSA key") + + newJWK := &jose.JSONWebKey{ + Key: &newKey.PublicKey, + Algorithm: keyAlgForKey(t, newKey), + } + newJWKJSON, err := newJWK.MarshalJSON() + test.AssertNotError(t, err, "Error marshaling random JWK") + + wfe.KeyRollover(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("", "{}")) + test.AssertUnmarshaledEquals(t, + responseWriter.Body.String(), + `{ + "type": "urn:acme:error:malformed", + "detail": "Parse error reading JWS", + "status": 400 + }`) + + testCases := []struct { + Name string + Payload string + ExpectedResponse string + NewKey crypto.Signer + }{ + { + Name: "Missing account URL", + Payload: `{"newKey":` + string(newJWKJSON) + `}`, + ExpectedResponse: `{ + "type": "urn:acme:error:malformed", + "detail": "Inner key rollover request specified Account \"\", but outer JWS has Key ID \"http://localhost/acme/reg/1\"", + "status": 400 + }`, + NewKey: newKey, + }, + { + Name: "Missing new key from inner payload", + Payload: `{"account":"http://localhost/acme/reg/1"}`, + ExpectedResponse: `{ + "type": "urn:acme:error:malformed", + "detail": "Inner JWS does not verify with specified new key", + "status": 400 + }`, + }, + { + Name: "New key is the same as the old key", + Payload: `{"newKey":{"kty":"RSA","n":"yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ","e":"AQAB"},"account":"http://localhost/acme/reg/1"}`, + ExpectedResponse: `{ + "type": "urn:acme:error:malformed", + "detail": "New key specified by rollover request is the same as the old key", + "status": 400 + }`, + }, + { + Name: "Inner JWS signed by the wrong key", + Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/reg/1"}`, + ExpectedResponse: `{ + "type": "urn:acme:error:malformed", + "detail": "Inner JWS does not verify with specified new key", + "status": 400 + }`, + }, + { + Name: "Valid key rollover request", + Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/reg/1"}`, + ExpectedResponse: `{ + "id": 1, + "key": ` + string(newJWKJSON) + `, + "contact": [ + "mailto:person@mail.com" + ], + "agreement": "http://example.invalid/terms", + "initialIp": "", + "createdAt": "0001-01-01T00:00:00Z", + "Status": "valid" + }`, + NewKey: newKey, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + responseWriter.Body.Reset() + + _, _, inner := signRequestEmbed(t, tc.NewKey, "http://localhost/key-change", tc.Payload, wfe.nonceService) + _, _, outer := signRequestKeyID(t, 1, nil, "http://localhost/key-change", inner, wfe.nonceService) + + wfe.KeyRollover(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("key-change", outer)) + test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), tc.ExpectedResponse) + }) + } +}