letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Empty reg update should return existing reg data (#2067) 

For #2001 an optimization was added to the WFE to avoid invoking the RA's `UpdateRegistration` method when a trivial (e.g. `{"resource:"reg"}`) update is received. Instead the WFE returned the trivial update back to the client immediately.

This is contrary to the ACME spec which indicates:
>  Servers SHOULD NOT respond to GET requests for registration resources as these requests are not authenticated. If a client wishes to query the server for information about its account (e.g., to examine the   “contact” or “certificates” fields), then it SHOULD do so by sending  a POST request with an empty update. That is, it should send a JWS whose payload is trivial ({“resource”:”reg”}).

The optimization regression was captured in issue #2066 when it broke at least one client implementation.

This removes the empty reg update optimization and passes all POST's to the RA. The RA will in turn fetch the existing registration to return to the client. The second half of the #2001 optimizations remains in place, no DB UPDATE's will be performed if the new registration content doesn't differ from the existing registration content (as determine by the return of `registration.MergeUpdate`).

Since the WFE optimization is no longer in place the `FailureRegistrationAuthority` mock isn't required and is removed. Similarly `TestEmptyRegistration` in `wfe_test.go` is changed from testing for the optimization to testing for the ACME described "get registration info" behaviour.

This fixes #2066
This commit is contained in:
Daniel McCarney 2016-07-21 09:05:56 -04:00 committed by GitHub
parent 5da7728cc5
commit cc137507de
3 changed files with 25 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
mocks/mocks.go
View File

@ -113,8 +113,11 @@ func (sa *StorageAuthority) GetRegistrationByKey(_ context.Context, jwk jose.Jso
panic(err)
}
contactURL, _ := core.ParseAcmeURL("mailto:person@mail.com")
contacts := []*core.AcmeURL{contactURL}
if core.KeyDigestEquals(jwk, test1KeyPublic) {
return core.Registration{ID: 1, Key: jwk, Agreement: agreementURL}, nil
return core.Registration{ID: 1, Key: jwk, Agreement: agreementURL, Contact: &contacts}, nil
}
if core.KeyDigestEquals(jwk, test2KeyPublic) {

19
wfe/wfe.go
View File

@ -1116,20 +1116,11 @@ func (wfe *WebFrontEndImpl) Registration(ctx context.Context, logEvent *requestE
// serialize the update as JSON to send via AMQP to the RA.
update.Key = currReg.Key
// If the registration doesn't have an agreement set, or any contacts (e.g. it
// is the trivial update `{"resource":"reg"}` then do not send it to the RA
// for update, there is nothing to save/update.
var updatedReg core.Registration
if len(update.Agreement) > 0 || update.Contact != nil {
// Ask the RA to update this authorization.
updatedReg, err = wfe.RA.UpdateRegistration(ctx, currReg, update)
if err != nil {
logEvent.AddError("unable to update registration: %s", err)
wfe.sendError(response, logEvent, core.ProblemDetailsForError(err, "Unable to update registration"), err)
return
}
} else {
updatedReg = update // Return the empty update as-is
updatedReg, err := wfe.RA.UpdateRegistration(ctx, currReg, update)
if err != nil {
logEvent.AddError("unable to update registration: %s", err)
wfe.sendError(response, logEvent, core.ProblemDetailsForError(err, "Unable to update registration"), err)
return
}
jsonReply, err := marshalIndent(updatedReg)

56
wfe/wfe_test.go
View File

@ -901,42 +901,19 @@ func TestNewECDSARegistration(t *testing.T) {
test.AssertEquals(t, responseWriter.Code, 409)
}
type FailureRegistrationAuthority struct{}
func (ra *FailureRegistrationAuthority) NewRegistration(ctx context.Context, reg core.Registration) (core.Registration, error) {
return core.Registration{}, fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) NewAuthorization(ctx context.Context, authz core.Authorization, regID int64) (core.Authorization, error) {
return core.Authorization{}, fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) NewCertificate(ctx context.Context, req core.CertificateRequest, regID int64) (core.Certificate, error) {
return core.Certificate{}, fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) UpdateRegistration(ctx context.Context, reg core.Registration, updated core.Registration) (core.Registration, error) {
return core.Registration{}, fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) UpdateAuthorization(ctx context.Context, authz core.Authorization, foo int, challenge core.Challenge) (core.Authorization, error) {
return core.Authorization{}, fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) RevokeCertificateWithReg(ctx context.Context, cert x509.Certificate, reason core.RevocationCode, reg int64) error {
return fmt.Errorf("FailureRegistrationAuthority failed by design")
}
func (ra *FailureRegistrationAuthority) AdministrativelyRevokeCertificate(ctx context.Context, cert x509.Certificate, reason core.RevocationCode, user string) error {
return fmt.Errorf("FailureRegistrationAuthority failed by design")
}
// Test that the WFE handling of the "empty update" POST is correct. The ACME
// spec describes how when clients wish to query the server for information
// about a registration an empty registration update should be sent, and
// a populated reg object will be returned.
func TestEmptyRegistration(t *testing.T) {
wfe, _ := setupWFE(t)
_, err := wfe.Handler()
test.AssertNotError(t, err, "Problem setting up HTTP handlers")
responseWriter := httptest.NewRecorder()
// 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, err := jose.LoadPrivateKey([]byte(test1KeyPrivatePEM))
test.AssertNotError(t, err, "Failed to load key")
rsaKey, ok := key.(*rsa.PrivateKey)
@ -945,12 +922,6 @@ func TestEmptyRegistration(t *testing.T) {
test.AssertNotError(t, err, "Failed to make signer")
signer.SetNonceSource(wfe.nonceService)
// Use the FailureRegistrationAuthority to ensure that if any method of the RA
// is invoked for this Registration post that the test fails. We explicitly
// want to test that the WFE returns for empty reg updates without consulting
// the RA or SA.
wfe.RA = &FailureRegistrationAuthority{}
emptyReg := `{"resource":"reg"}`
emptyBody, err := signer.Sign([]byte(emptyReg))
test.AssertNotError(t, err, "Unable to sign emptyBody")
@ -962,11 +933,16 @@ func TestEmptyRegistration(t *testing.T) {
responseWriter,
makePostRequestWithPath("1", emptyBody.FullSerialize()))
// There should be no error, particularly from the FailureRegistrationAuthority
// There should be no error
test.AssertNotContains(t, responseWriter.Body.String(), "urn:acme:error")
// There shouldn't be a Contact or Agreement in the response
test.AssertNotContains(t, responseWriter.Body.String(), "Contact")
test.AssertNotContains(t, responseWriter.Body.String(), "Agreement")
// We should get back a populated Registration
var reg core.Registration
err = json.Unmarshal([]byte(responseWriter.Body.String()), &reg)
test.AssertNotError(t, err, "Couldn't unmarshal returned registration object")
test.Assert(t, len(*reg.Contact) >= 1, "No contact field in registration")
test.AssertEquals(t, (*reg.Contact)[0].String(), "mailto:person@mail.com")
test.AssertEquals(t, reg.Agreement, "http://example.invalid/terms")
responseWriter.Body.Reset()
}