Skip to content

ra/sa: render the attempted challenge as "processing" - #9096

Open
bhaskargurram-ai wants to merge 2 commits into
letsencrypt:mainfrom
bhaskargurram-ai:feat/challenge-processing-status
Open

bhaskargurram-ai wants to merge 2 commits into
letsencrypt:mainfrom
bhaskargurram-ai:feat/challenge-processing-status

Conversation

@bhaskargurram-ai

Copy link
Copy Markdown

RFC 8555 §7.1.6 says a challenge moves from "pending" to "processing" once the
client responds to it. Boulder never rendered that state: challenges are stored
as part of the authorization rather than as separate objects, so a challenge
stayed "pending" until the authz was finalized as "valid" or "invalid".

#8838 added SetAuthzProcessing, which sets authz2.beganProcessing before
validation is dispatched, but that column has no effect on rendered objects.
Following @aarongable's suggestion in #9032, this extends it so the RA also
tells the SA which challenge is being validated, and the SA renders that
challenge as "processing" while the authz stays "pending":

  • SetAuthzProcessing now takes a dedicated SetAuthzProcessingRequest
    carrying the authz ID and the attempted challenge type. Field 1 is unchanged
    from AuthorizationID2, so the RPC stays wire-compatible in both directions
    during a deploy; the new field is optional and an RA that doesn't send it
    gets the old behaviour.
  • The SA writes the challenge type into the attempted column in the same
    UPDATE that sets beganProcessing.
  • modelToAuthzPB renders an attempted challenge on a still-pending authz as
    "processing" (with no validation results). The authz status stays "pending",
    since RFC 8555 has no "processing" status for authorizations. The
    combination is done in the SA rather than the WFE because beganProcessing
    isn't part of the authz model/proto, and attempted != NULL on a pending
    authz is only ever produced by SetAuthzProcessing.
  • The RA rejects a second attempt at a challenge that is already "processing"
    with the same "conflict" error the SA returns, so clients that re-POST keep
    getting the 409 introduced by ra/sa: Prevent parallel validation attempts #8838 (and TestParallelValidationConflict
    keeps passing) instead of a "malformed" 400 from CheckPending.
  • test/inmem/sa now forwards SetAuthzProcessing; it didn't before, so RA
    tests couldn't exercise the flag.

Everything is still gated behind the existing SetAuthzProcessing feature
flag. No config, schema, or flag changes.

Open questions for reviewers:

  • Once a challenge is attempted, sibling challenges are hidden (existing
    behaviour). While processing they're shown the same way; happy to render
    them as "pending" instead if you'd rather follow §7.1.4 literally.
  • The conflict error text is duplicated between RA and SA; can hoist to a
    shared constant if preferred.

Tests: new TestAuthzModelProcessing, extended TestSetAuthzProcessing,
TestPerformValidationSetsChallengeProcessing,
TestPerformValidationChallengeProcessing, and WFE challenge/authz handler
cases asserting the rendered "processing" status. Protos regenerated with the
pinned toolchain (protoc 3.20.1, protoc-gen-go v1.36.5, protoc-gen-go-grpc
v1.5.1).

Fixes #9032

RFC 8555 Section 7.1.6 says a challenge transitions from "pending" to
"processing" once the client responds to it. Boulder never rendered that
state: because challenges are stored as part of the authorization rather
than as separate objects, a challenge stayed "pending" until the
authorization was finalized as "valid" or "invalid".

SetAuthzProcessing (added in letsencrypt#8838) already marks the authz's
beganProcessing column before dispatching validation, but that column has
no effect on the rendered objects. Extend it so that the RA also tells the
SA which challenge is being validated:

- Replace the SetAuthzProcessing request type with a dedicated
  SetAuthzProcessingRequest carrying the authz ID and the attempted
  challenge type. Field 1 is unchanged, so the RPC remains wire-compatible
  with the old AuthorizationID2 request in both directions.
- Have the SA write the attempted challenge type into the authz2
  "attempted" column in the same UPDATE that sets beganProcessing. The
  field is optional, so an RA which predates it still works.
- When modelToAuthzPB sees an attempted challenge on an authz which is
  still pending, render that challenge with status "processing" (and no
  validation results) while leaving the authz status "pending", since
  RFC 8555 has no "processing" status for authorizations.
- Have the RA reject a second attempt at a challenge which is already
  processing with the same "conflict" error the SA returns, rather than
  letting CheckPending turn it into a "malformed" error.
- Forward SetAuthzProcessing in the in-memory SA used by the RA tests,
  which previously did not implement it.

All of the new behavior remains gated behind the existing
SetAuthzProcessing feature flag, so nothing changes until the flag is
enabled in the RA.

Fixes letsencrypt#9032
Comment thread sa/sa.go
Comment thread sa/model.go Outdated
Comment thread sa/model.go Outdated
Comment thread sa/model.go Outdated
Address review feedback: when the authz has an attempted challenge,
populate that single challenge directly instead of checking every
challenge type in the bitmap loop. Move the "processing" check and the
validated time into populateAttemptedFields, and add a TODO to require
the attempted challenge type once the SetAuthzProcessing flag is
removed.
@bhaskargurram-ai

Copy link
Copy Markdown
Author

@aarongable Thanks, done in d96e0d4. The attempted challenge is now built directly outside the loop, the processing check and the validated time moved into populateAttemptedFields, and I added the TODO next to the optional check in SetAuthzProcessing. I also set a finalized status on the two models in TestPopulateAttemptedFieldsBadJSON, since a zero-value model is now treated as pending and returns early.

Comment thread sa/model.go
Comment on lines +839 to +840
if am.Attempted != nil {
if (am.Challenges>>*am.Attempted)&1 == 1 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either combine these into a single conditional, or remove the second conditional (it should be impossible for am.Attempted to indicate a value that isn't set in the am.Challenges bitmask, but I haven't looked closely to check how firmly we hold that invariant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Challenges never transition to the "processing" state

2 participants