sa: ClearEmail method needs to observe LockCol

[jsha]

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:

• UpdateRegistration
• ClearEmail

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]

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]

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

[aarongable]

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]

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.