Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sa: ClearEmail method needs to observe LockCol #7716

Open
jsha opened this issue Sep 23, 2024 · 4 comments · May be fixed by #7827
Open

sa: ClearEmail method needs to observe LockCol #7716

jsha opened this issue Sep 23, 2024 · 4 comments · May be fixed by #7827
Assignees
Labels
starter Ideal issues for folks getting familiar with Boulder

Comments

@jsha
Copy link
Contributor

jsha commented Sep 23, 2024

borp has a "VersionCol" feature for optimistic locking. We mostly don't use it, but we still have it turned on for the registrations table:

boulder/sa/database.go

Lines 267 to 270 in 1fcf0ee

regTable := dbMap.AddTableWithName(regModel{}, "registrations").SetKeys(true, "ID")
regTable.SetVersionCol("LockCol")
regTable.ColMap("Key").SetNotNull(true)

This means that when a registration is read from the database, it gets the current LockCol. When we call Update for that registration, the generated SQL gets a WHERE LockCol = ? clause that specifies the LockCol previously read from the database. And the Update method automatically bumps the LockCol.

We call Update on registration objects in two places:

Since LockCol is a database-internal concept, we don't propagate it in registrationModelToPb; the protobuf version of a registration doesn't include a LockCol, and shouldn't.

In UpdateRegistration, we get around this by reading the current registration, then combining its current LockCol with the data received in the update request protobuf (req *corepb.Registration) to get a regModel with the correct LockCol, then calling Update on that.

However, ClearEmail doesn't do that, meaning that using admin clear-email may fail to clear emails when LockCol > 1. That happens when the user has previously updated their own account using the ACME logs.

Options for fixing:

  • In ClearEmail, instead of calling registrationModelToPb, simply parse and modify the contacts field of the fetched registration. Since the registration is staying internal to the SA, converting it to a protobuf is a conceptual mistake.
  • In ClearEmail, rather than parsing the contacts field to remove one specific address, clear the whole thing. Accounts can have multiple email addresses, but this is rare, and if a user has asked us to clear one email from their account it's reasonable to clear all of them.
  • Remove use of LockCol for registrations. This would require modifications to UpdateRegistration and ClearEmail to replace their Update calls with calls that only touch modified fields (which, can be: contacts; status; or jwk / jwk_sha256 (this one only via the key rotation API).
@aarongable
Copy link
Contributor

Honestly, I support option (3). Although the use of LockCol can prevent certain kinds of data races, I think that our current mechanism of setting every field of the registration row upon any update is fundamentally flawed. We should only be able to update the three columns (contacts, status, jwk) which are actually mutable, and our gRPC messages and SA implementations should reflect this.

@jcjones
Copy link
Contributor

jcjones commented Sep 23, 2024

In the moderate future, we can plan to remove the column entirely.

@aarongable aarongable added the starter Ideal issues for folks getting familiar with Boulder label Sep 24, 2024
@aarongable
Copy link
Contributor

aarongable commented Sep 24, 2024

We only do three kinds of account updates:

  • deactivation;
  • changing contacts; and
  • changing JWK

The first two go through the WFE's "POST to acct URL" endpoint, and can be easily distinguished just by looking to see if the POSTed object has "status: deactivated" in it. If it does, we should go through a dedicated gRPC deactivation codepath (the SA already has a method for this, but we never call it for some reason); if it does not, we should go through a dedicated gRPC contact-change codepath (this does not exist yet).

The last one starts at a different WFE endpoint, but then currently gets merged into the same generic "update account" gRPC codepath. It needs to be split out into its own gRPC key-change codepath as well.

There's some question of how to handle an account update request that both changes the contacts and deactivates the account. We believe this should be solved by simply always deleting the contacts field when an account is deactivated: we won't be sending them any more emails anyway, we don't need that information anymore, and it's unlikely that a deactivated account wants to hear from us.

@aarongable
Copy link
Contributor

All that said, there's a very easy Step 0 fix here: just change the ClearEmail method to just do an UPDATE registrations SET contact = ? WHERE id = ?, rather than a tx.Update(), to avoid the LockCol issue entirely.

@jprenken jprenken self-assigned this Sep 24, 2024
jprenken added a commit that referenced this issue Sep 27, 2024
Change ClearEmail to use a direct `UPDATE` query instead of
`tx.Update()`, in order to avoid `LockCol` issues.

Part of #7716
@aarongable aarongable added this to the Sprint 2024-10-01 milestone Oct 1, 2024
jprenken added a commit that referenced this issue Oct 2, 2024
…methods in RA & SA

Clear contact field in DeactivateRegistration

Part of #7716
Part of #5554
jprenken added a commit that referenced this issue Nov 6, 2024
…methods in RA & SA (#7735)

Introduce separate UpdateRegistrationContact & UpdateRegistrationKey
methods in RA & SA

Clear contact field during DeactivateRegistration

Part of #7716
Part of #5554
@jsha jsha modified the milestones: Sprint 2024-11-05, 2024-11-12 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
starter Ideal issues for folks getting familiar with Boulder
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants