ra/sa: Prevent parallel validation attempts#8838
Conversation
|
@aarongable, this PR adds one or more new feature flags: SetAuthzProcessing. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document. Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:". |
beautifulentropy
changed the title
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
This looks good to me. Notes:
This will increase database load, I think significantly. Every authz that we validate will now generate two writes to the authz2 table instead of 1. We can try it out and see if that's too much, but definitely something to keep an eye on.
This will change our metrics for the /acme/authz endpoint: we'll have fewer 200's and more 409s. We should make sure to note that in the SRE ticket enabling the flag.
For the question of whether to use the beganProcessing idiom (like orders), I wanted to double check why we did it that way in orders. Turns out it was because we decided to synthesize order status on read based on the authz statuses and expires information. There's no "status" field in the order model, but we did need to store the processing bit.
For authzs the story is different: we could set a "processing" status (rewriting it at render time since RFC 8555 doesn't have "processing" for authzs). But there's a different reason to like beganProcessing: since we have an index that includes status, and it's nice to not spend extra writes updating the index.
KEY `regID_identifier_status_expires_idx` (`registrationID`,`identifierType`,`identifierValue`,`status`,`expires`),
beautifulentropy
left a comment
beautifulentropy
left a comment
There was a problem hiding this comment.
Mostly just some stale comments/log lines, but also a question about doing this work inside of a transaction. Thanks for your work on this!
| if req.Id == 0 { | ||
| return nil, errIncompleteRequest | ||
| } | ||
| _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (any, error) { |
There was a problem hiding this comment.
I don't think we need a transaction here. We're performing just one update query, which I should be atomic on its own. The transaction itself adds two round trips to that. I notice that we also do the same in *SQLStorageAuthority.SetOrderProcessing(), so perhaps there's a quality that a transaction confers upon this query that I just don't understand.
| // We log NotFound at a lower level because this is largely due to a | ||
| // parallel-validation race: a different validation attempt has already | ||
| // updated this authz, so we failed to find a *pending* authz with the | ||
| // given ID to update. |
There was a problem hiding this comment.
In a post-SetAuthzProcessing world this comment will become stale. Arguably it's already stale as of this PR. We should consider updating it now or at least leaving a TODO to clean it up. FWIW, in the future, I think the only way we could still end up here is the client concurrently deactivating the authz while in the process of validating.
| // parallel-validation race: a different validation attempt has already | ||
| // updated this authz, so we failed to find a *pending* authz with the | ||
| // given ID to update. | ||
| ra.log.Info(ctx, "Failed to record validation (likely parallel validation race)", |
There was a problem hiding this comment.
Continuing the same train of thought as above, how about:
| ra.log.Info(ctx, "Failed to record validation (authz no longer pending)", |
| blog.Error(err), | ||
| ) | ||
| } else { | ||
| ra.log.AuditError(ctx, "Failed to record validation (likely parallel validation race)", err, |
There was a problem hiding this comment.
I'm struggling to see how we'd get here from a parallel validation race. The race should always resolve as NotFound. Here we should only see uncaught database errors, timeouts, etc.
| ra.log.AuditError(ctx, "Failed to record validation", err, |
4 participants
Add a new
beganProcessingcolumn to the authz2 table, which is a replica of the column of the same name in the orders table. Similarly add a new SA gRPC method which sets this column to true, or returns an error if it already has been set to true. The update is done in a transaction to ensure that multiple attempts to update the field don't all succeed. The error is specifically "conflict", so that the WFE will render it with HTTP status code 409.In the RA, call the new SA method before kicking off validation. If it returns an error, bail out. Lock this behavior behind a feature flag to ensure it is not enabled before the necessary database changes have been made.
Add an integration test which kicks off two validations simultaneously and ensures that exactly one of them fails with reason "conflict".
Fixes #3036
IN-12842 tracks the corresponding SRE-side database and config changes
CPS Compliance Review: Neither the BRs nor our CP/CPS have anything to say about attempting validation multiple times. We will be in compliance both before and after this feature flag is enabled.