Make POSTing KeyAuthorization optional, V2 don't echo it. (letsencryp…

…t#3526)

This commit updates the RA to make the notion of submitting
a KeyAuthorization value as part of the ra.UpdateAuthorization call
optional. If set, the value is enforced against expected and an error is
returned if the provided authorization isn't correct. If it isn't set
the RA populates the field with the computed authorization for the VA to
enforce against the value it sees in challenges. This retains the legacy
behaviour of the V1 API. The V2 API will never unmarshal a provided
key authorization.

The ACMEv2/WFEv2 prepChallengeForDisplay function is updated to strip
the ProvidedKeyAuthorization field before sending the challenge object
back to a client. ACMEv1/WFEv1 continue to return the KeyAuthorization
in challenges to avoid breaking clients that are relying on this legacy
behaviour.

For deployability ease this commit retains the name of the
core.Challenge.ProvidedKeyAuthorization field even though it should
be called core.Challenge.ComputedKeyAuthorization now that it isn't
set based on the client's provided key authz. This will be easier as
a follow-up change.

Resolves letsencrypt#3514

core/objects.go

@@ -233,14 +233,14 @@ type Challenge struct {
 	URL string `json:"url,omitempty"`
 
 	// Used by http-01, tls-sni-01, and dns-01 challenges
-	Token string `json:"token,omitempty"` // Used by http-00, tls-sni-00, and dns-00 challenges
-
-	// The KeyAuthorization provided by the client to start validation of
-	// the challenge. Set during
-	//
-	//   POST /acme/authz/:authzid/:challid
-	//
-	// Used by http-01, tls-sni-01, and dns-01 challenges
+	Token string `json:"token,omitempty"`
+
+	// The expected KeyAuthorization for validation of the challenge. Populated by
+	// the RA prior to passing the challenge to the VA. For legacy reasons this
+	// field is called "ProvidedKeyAuthorization" because it was initially set by
+	// the content of the challenge update POST from the client. It is no longer
+	// set that way and should be renamed to "KeyAuthorization".
+	// TODO(@cpu): Rename `ProvidedKeyAuthorization` to `KeyAuthorization`.
 	ProvidedKeyAuthorization string `json:"keyAuthorization,omitempty"`
 
 	// Contains information about URLs used or redirected to and IPs resolved and

ra/ra.go

@@ -1401,7 +1401,11 @@ func mergeUpdate(r *core.Registration, input core.Registration) bool {
 }
 
 // UpdateAuthorization updates an authorization with new values.
-func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, base core.Authorization, challengeIndex int, response core.Challenge) (core.Authorization, error) {
+func (ra *RegistrationAuthorityImpl) UpdateAuthorization(
+	ctx context.Context,
+	base core.Authorization,
+	challengeIndex int,
+	response core.Challenge) (core.Authorization, error) {
 	// Refuse to update expired authorizations
 	if base.Expires == nil || base.Expires.Before(ra.clk.Now()) {
 		return core.Authorization{}, berrors.MalformedError("expired authorization")
@@ -1457,18 +1461,30 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba
 		return core.Authorization{}, berrors.InternalServerError(err.Error())
 	}
 
-	// Recompute the key authorization field provided by the client and
-	// check it against the value provided
+	// Compute the key authorization field based on the registration key
 	expectedKeyAuthorization, err := ch.ExpectedKeyAuthorization(reg.Key)
 	if err != nil {
 		return core.Authorization{}, berrors.InternalServerError("could not compute expected key authorization value")
 	}
-	if expectedKeyAuthorization != response.ProvidedKeyAuthorization {
+
+	// NOTE(@cpu): Historically challenge update required the client to send
+	// a JSON POST body that included a computed KeyAuthorization. The RA would
+	// check this provided authorization against its own computation of the key
+	// authorization and err if they did not match. New ACME specification does
+	// not require this - the client does not need to send the key authorization.
+	// To support this for ACMEv2 we only enforce the provided key authorization
+	// matches expected if the update included it.
+	if response.ProvidedKeyAuthorization != "" && expectedKeyAuthorization != response.ProvidedKeyAuthorization {
 		return core.Authorization{}, berrors.MalformedError("provided key authorization was incorrect")
 	}
 
-	// Copy information over that the client is allowed to supply
-	ch.ProvidedKeyAuthorization = response.ProvidedKeyAuthorization
+	// Populate the ProvidedKeyAuthorization such that the VA can confirm the
+	// expected vs actual without needing the registration key. Historically this
+	// was done with the value from the challenge response and so the field name
+	// is called "ProvidedKeyAuthorization", in reality this is just
+	// "KeyAuthorization".
+	// TODO(@cpu): Rename ProvidedKeyAuthorization to KeyAuthorization
+	ch.ProvidedKeyAuthorization = expectedKeyAuthorization
 
 	// Double check before sending to VA
 	if cErr := ch.CheckConsistencyForValidation(); cErr != nil {

ra/ra_test.go

@@ -855,6 +855,7 @@ func TestUpdateAuthorization(t *testing.T) {
 	authz, err := ra.NewAuthorization(ctx, AuthzRequest, Registration.ID)
 	test.AssertNotError(t, err, "NewAuthorization failed")
 
+	// Create a challenge response with a key authorization
 	response, err := makeResponse(authz.Challenges[ResponseIndex])
 	test.AssertNotError(t, err, "Unable to construct response to challenge")
 	authz, err = ra.UpdateAuthorization(ctx, authz, ResponseIndex, response)
@@ -877,6 +878,32 @@ func TestUpdateAuthorization(t *testing.T) {
 
 	// Verify that the responses are reflected
 	test.Assert(t, len(vaAuthz.Challenges) > 0, "Authz passed to VA has no challenges")
+
+	// Create another authorization
+	authz, err = ra.NewAuthorization(ctx, AuthzRequest, Registration.ID)
+	test.AssertNotError(t, err, "NewAuthorization failed")
+
+	// Update it with an empty challenge, no key authorization
+	// This should work as well based on modern key authorization semantics
+	authz, err = ra.UpdateAuthorization(ctx, authz, ResponseIndex, core.Challenge{})
+	test.AssertNotError(t, err, "UpdateAuthorization failed")
+	select {
+	case a := <-va.argument:
+		vaAuthz = a
+	case <-time.After(time.Second):
+		t.Fatal("Timed out waiting for DummyValidationAuthority.PerformValidation to complete")
+	}
+
+	// Verify that returned authz same as DB
+	dbAuthz, err = sa.GetAuthorization(ctx, authz.ID)
+	test.AssertNotError(t, err, "Could not fetch authorization from database")
+	assertAuthzEqual(t, authz, dbAuthz)
+
+	// Verify that the VA got the authz, and it's the same as the others
+	assertAuthzEqual(t, authz, vaAuthz)
+
+	// Verify that the responses are reflected
+	test.Assert(t, len(vaAuthz.Challenges) > 0, "Authz passed to VA has no challenges")
 }
 
 func TestUpdateAuthorizationExpired(t *testing.T) {

va/va_test.go

@@ -905,7 +905,7 @@ func TestDNSValidationNotSane(t *testing.T) {
 	chal1.Token = "yfCBb-bRTLz8Wd1C0lTUQK3qlKj3-t2tYGwx5Hj7r_"
 
 	chal2 := core.DNSChallenge01()
-	chal2.ProvidedKeyAuthorization = ""
+	chal2.ProvidedKeyAuthorization = "a"
 
 	var authz = core.Authorization{
 		ID:             core.NewToken(),

wfe2/wfe.go

@@ -847,6 +847,9 @@ func (wfe *WebFrontEndImpl) prepChallengeForDisplay(request *http.Request, authz
 	challenge.URI = ""
 	challenge.ID = 0
 
+	// ACMEv2 never sends the KeyAuthorization back in a challenge object.
+	challenge.ProvidedKeyAuthorization = ""
+
 	// Historically the Type field of a problem was always prefixed with a static
 	// error namespace. To support the V2 API and migrating to the correct IETF
 	// namespace we now prefix the Type with the correct namespace at runtime when
@@ -921,14 +924,6 @@ func (wfe *WebFrontEndImpl) postChallenge(
 		wfe.sendError(response, logEvent, prob, nil)
 		return
 	}
-	// Any version of the agreement is acceptable here. Version match is enforced in
-	// wfe.Account when agreeing the first time. Agreement updates happen
-	// by mailing subscribers and don't require an account update.
-	if currAcct.Agreement == "" {
-		wfe.sendError(response, logEvent,
-			probs.Unauthorized("Account must agree to subscriber agreement before any further actions"), nil)
-		return
-	}
 
 	// Check that the account ID matching the key used matches
 	// the account ID on the authz object
@@ -941,14 +936,21 @@ func (wfe *WebFrontEndImpl) postChallenge(
 		return
 	}
 
-	var challengeUpdate core.Challenge
+	// NOTE(@cpu): Historically a challenge update needed to include
+	// a KeyAuthorization field. This is no longer the case, since both sides can
+	// calculate the key authorization as needed. We unmarshal here only to check
+	// that the POST body is valid JSON. Any data/fields included are ignored to
+	// be kind to ACMEv2 implementations that still send a key authorization.
+	var challengeUpdate struct{}
 	if err := json.Unmarshal(body, &challengeUpdate); err != nil {
 		wfe.sendError(response, logEvent, probs.Malformed("Error unmarshaling challenge response"), err)
 		return
 	}
 
-	// Ask the RA to update this authorization
-	updatedAuthorization, err := wfe.RA.UpdateAuthorization(ctx, authz, challengeIndex, challengeUpdate)
+	// Ask the RA to update this authorization. Send an empty `core.Challenge{}`
+	// as the challenge update because we do not care about the KeyAuthorization
+	// (if any) sent in the challengeUpdate.
+	updatedAuthorization, err := wfe.RA.UpdateAuthorization(ctx, authz, challengeIndex, core.Challenge{})
 	if err != nil {
 		wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "Unable to update challenge"), err)
 		return

wfe2/wfe_test.go

@@ -2487,6 +2487,7 @@ func TestPrepAuthzForDisplay(t *testing.T) {
 			{
 				ID:   12345,
 				Type: "dns",
+				ProvidedKeyAuthorization: "	🔑",
 			},
 		},
 		Combinations: [][]int{{1, 2, 3}, {4}, {5, 6}},
@@ -2510,4 +2511,7 @@ func TestPrepAuthzForDisplay(t *testing.T) {
 	chal := authz.Challenges[0]
 	test.AssertEquals(t, chal.URL, "http://localhost/acme/challenge/12345/12345")
 	test.AssertEquals(t, chal.URI, "")
+	// We also expect the ProvidedKeyAuthorization is not echoed back in the
+	// challenge
+	test.AssertEquals(t, chal.ProvidedKeyAuthorization, "")
 }