From 0c658f202aca268a74d6d2334691925c570becbb Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 18 Dec 2024 10:06:08 -0800 Subject: [PATCH] Fix error when deactivating an account (#7899) The RA's DeactivateAccount method expects the account provided to it by the WFE to still have status Valid. The new WFE deactivation code was hardcoding the status to Deactivated. Fix the WFE to pass the account's current status instead. Add an integration test to confirm both the breakage and the fix. Also leave behind some TODOs to simplify this codepath further, and not require the status to be provided at all. Part of #5554 --- ra/ra.go | 5 ++++ sa/sa.go | 2 ++ test/integration/account_test.go | 50 ++++++++++++++++++++++++++++++++ wfe2/wfe.go | 7 ++++- wfe2/wfe_test.go | 6 ---- 5 files changed, 63 insertions(+), 7 deletions(-) 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..162ee545720 --- /dev/null +++ b/test/integration/account_test.go @@ -0,0 +1,50 @@ +//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) + } + + // 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. +} diff --git a/wfe2/wfe.go b/wfe2/wfe.go index ebfc3cd7257..19063b7cd2f 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -1466,12 +1466,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 } 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" }`)