-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor authorization status polling #44
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the authorization status polling mechanism to improve reliability and reduce unnecessary retry attempts. The changes replace fixed retry counts with configurable timeouts and implement exponential backoff strategies.
Key changes:
- Replaces fixed retry counters with configurable timeout-based retry policies
- Implements exponential backoff with maximum interval limits for better server behavior
- Consolidates challenge handling logic by removing the separate
do_challenge
method
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
5f14ca2
to
47c95ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with one remark:
request retrying logic may be made more complicated then simple repetition with fixed interval and fixed overall times. It gives fast failure in case of permanent network errors but may lead to unnecessary failures if error conditions are temporary. But this may be postponed to adding fatal error checking in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks almost correct to me, but I'm not sure if ChallengeStatus::Valid can be the result of a request to challenge URL. I understood that only the polling to authz URL can result in a ChallengeStatus::Valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good.
Previously, we assumed that we should poll the challenge URL until completion, because all the servers we tested correctly responded to a POST-as-GET to the challenger URL. We also made an initial POST-as-GET request to fetch the status and skip the challenges in "valid" state. RFC8555 7.5.1 specifies that we should poll the authorization resource status instead. Additionally, the discussion around Errata ID 6317 implies that it is harmful to initiate multiple challenges, as a single failure invalidates the whole authorization. Given the above, we can simplify the code and proceed to poll the authorization status after initiating the first available challenge.
Previously, we limited polling for challenge and order status to a fixed number of tries. The updated algorithm will use increasing intervals and give up when the total wait timeout has elapsed.
47c95ab
to
5e648e4
Compare
I confirmed that EJBCA performs validation while processing the POST to challenge URL, and delays the HTTP response until it is able to resolve the status to "valid" or "invalid". That is a fully spec-compliant behavior. The other cases are pre-authorization and reuse of a previously validated authorization, but these are less likely to happen because we already filter valid authorizations a few lines above. |
Tested with:
Fixes: #29
Fixes: #37
Fixes: #40