From c1aea7b8fe03ba0810c76415c1795e7c7bbd0a1d Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 18 Dec 2024 08:50:37 -0800 Subject: [PATCH 1/3] Fix error when deactivating an account --- ra/ra.go | 5 ++++ sa/sa.go | 2 ++ test/integration/account_test.go | 49 ++++++++++++++++++++++++++++++++ wfe2/wfe.go | 7 ++++- 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 test/integration/account_test.go diff --git a/ra/ra.go b/ra/ra.go index 255d923b9f1..bf882e5a834 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -2442,6 +2442,9 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, if reg == nil || reg.Id == 0 { return nil, errIncompleteGRPCRequest } + // TODO(#5554): Remove this check: this is only enforcing that the WFE has + // told us the correct status. The SA will enforce that the current status is + // valid during its database update. if reg.Status != string(core.StatusValid) { return nil, berrors.MalformedError("only valid registrations can be deactivated") } @@ -2449,6 +2452,8 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, if err != nil { return nil, err } + + // TODO(#5554): Return the updated account object. return &emptypb.Empty{}, nil } diff --git a/sa/sa.go b/sa/sa.go index 7e706149651..c7be24ae5ef 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -549,6 +549,8 @@ func (ssa *SQLStorageAuthority) DeactivateRegistration(ctx context.Context, req if err != nil { return nil, err } + + // TODO(#5554): Return the updated account object. return &emptypb.Empty{}, nil } diff --git a/test/integration/account_test.go b/test/integration/account_test.go new file mode 100644 index 00000000000..fbde24db5b6 --- /dev/null +++ b/test/integration/account_test.go @@ -0,0 +1,49 @@ +//go:build integration + +package integration + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "testing" + + "github.com/eggsampler/acme/v3" + + "github.com/letsencrypt/boulder/core" +) + +// TestAccountDeactivate tests that account deactivation works. It does not test +// that we reject requests for other account statuses, because eggsampler/acme +// wisely does not allow us to construct such malformed requests. +func TestAccountDeactivate(t *testing.T) { + t.Parallel() + + c, err := acme.NewClient("http://boulder.service.consul:4001/directory") + if err != nil { + t.Fatalf("failed to connect to acme directory: %s", err) + } + + acctKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("failed to generate account key: %s", err) + } + + account, err := c.NewAccount(acctKey, false, true, "mailto:hello@blackhole.net") + if err != nil { + t.Fatalf("failed to create initial account: %s", err) + } + + got, err := c.DeactivateAccount(account) + if err != nil { + t.Errorf("unexpected error while deactivating account: %s", err) + } + + if got.Status != string(core.StatusDeactivated) { + t.Errorf("account deactivation should have set status to %q, instead got %q", core.StatusDeactivated, got.Status) + } + + if len(got.Contact) != 0 { + t.Errorf("account deactivation should have cleared contacts field, instead got %+v", got.Contact) + } +} diff --git a/wfe2/wfe.go b/wfe2/wfe.go index ab175d6b10c..dd2b10248e2 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -1465,12 +1465,17 @@ func (wfe *WebFrontEndImpl) updateAccount( // their contacts, the deactivation will take place and return before an // update would be performed. Deactivation deletes the contacts field. if accountUpdateRequest.Status == core.StatusDeactivated { - _, err = wfe.ra.DeactivateRegistration(ctx, &corepb.Registration{Id: currAcct.ID, Status: string(core.StatusDeactivated)}) + // TODO(#5554): Remove the need to pass Status here: we wouldn't have reached + // this point unless the requesting account was valid. + _, err = wfe.ra.DeactivateRegistration(ctx, &corepb.Registration{Id: currAcct.ID, Status: string(currAcct.Status)}) if err != nil { return nil, web.ProblemDetailsForError(err, "Unable to deactivate account") } + // TODO(#5554): Have DeactivateRegistration return the updated account + // object, so we don't have to modify it ourselves. currAcct.Status = core.StatusDeactivated + currAcct.Contact = nil return currAcct, nil } From 759107f32728398c8b2020320f87aeaea28d968e Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 18 Dec 2024 08:54:52 -0800 Subject: [PATCH 2/3] Fix unittest --- wfe2/wfe_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 1b0a90db1be..c8229f58a7f 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -2756,9 +2756,6 @@ func TestDeactivateAccount(t *testing.T) { "n": "yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ", "e": "AQAB" }, - "contact": [ - "mailto:person@mail.com" - ], "status": "deactivated" }`) @@ -2775,9 +2772,6 @@ func TestDeactivateAccount(t *testing.T) { "n": "yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ", "e": "AQAB" }, - "contact": [ - "mailto:person@mail.com" - ], "status": "deactivated" }`) From c2390d33abba01723028222b26943fe17a2078b0 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 18 Dec 2024 09:16:40 -0800 Subject: [PATCH 3/3] work around eggsampler bug with todo --- test/integration/account_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/account_test.go b/test/integration/account_test.go index fbde24db5b6..162ee545720 100644 --- a/test/integration/account_test.go +++ b/test/integration/account_test.go @@ -43,7 +43,8 @@ func TestAccountDeactivate(t *testing.T) { t.Errorf("account deactivation should have set status to %q, instead got %q", core.StatusDeactivated, got.Status) } - if len(got.Contact) != 0 { - t.Errorf("account deactivation should have cleared contacts field, instead got %+v", got.Contact) - } + // TODO(#5554): Check that the contacts have been cleared. We can't do this + // today because eggsampler/acme unmarshals the WFE's response into the same + // account object as it used to make the request, and a wholly missing + // contacts field doesn't overwrite whatever eggsampler was holding in memory. }