Add Authz revocation upon Cert revocation, by feature flag.#8799
Add Authz revocation upon Cert revocation, by feature flag.#8799ezekiel wants to merge 15 commits into
Conversation
ezekiel
changed the title
|
@ezekiel, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
|
@ezekiel, this PR adds one or more new feature flags: RevokeAuthzsUponRevokeCert. 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:". |
|
CPS Compliance Review: Our CP/CPS does not directly discuss authorization revocation - there ARE important points about authorization re-use time frames, including in the Baseline Requirements 4.2.1. This flag does not modify authorization re-use time frames. After enabling this flag, authorizations may be revoked in a particular circumstance, which fully prevents their re-use regardless of their age. |
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
LGTM with a few small nits!
Because it can revoke multiple authorizations. Co-authored-by: Aaron Gable <aaron@letsencrypt.org>
Update retrybackoff usage of authz revocation tests to 1) retry more quickly in the test expecting change, but 2) retry over a longer period of time in the test expecting stability to try to extend beyond the custom context timeout of the RA function. Rename SA gRPC methods to be consistently plural, reflecting their capability.
…uthorization Resolve ra.go conflicts emerging from the slog revert.
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
Logging changes since last review LGTM.
|
Conversation from Jacob - we don't re-use "pending" authz, so don't need to include that in the revocation Converting to Draft PR for some updates. |
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Thanks for working on this! It should say "Fixes #8673" in the PR description.
I'm curious about the requirement that authzs not be revoked when an account revokes its own certificate. Is that a usability requirement, or a "save database work" requirement, or something else? I see in #8673 that @aarongable said "especially in the case where one ACME client is revoking a cert originally issued by a different ACME client".
Looking at the code, and https://letsencrypt.org/docs/revoking/, I see that it's more about the method of authenticating the revocation request: subscriber, control, key, or admin. Probably we don't want to revoke authorizations based on private key control. Probably we do want to revoke authorizations when an admin revokes a certificate. For user-facing consistency, it seems like a better rule might be "revocation methods other than key cause authz revocation." WDYT?
| // revoked certificate which are held by the RegID from cert metadata, | ||
| // confirmed above to be different than requester ID. | ||
| if requestAuthzRevocation { | ||
| go ra.revokeAuthorizations(ctx, cert, metadata.RegistrationID) |
There was a problem hiding this comment.
| go ra.revokeAuthorizations(ctx, cert, metadata.RegistrationID) | |
| go ra.drainWG.revokeAuthorizations(ctx, cert, metadata.RegistrationID) |
The RA has a couple of other "background tasks" that get detached from the RPC stack: validation and async finalize. For both of them we use the drainWG to make sure they finish (or time out) during a clean shutdown. We should do the same here.
Also, worth noting that we will have three separate timeouts that govern drainable work:
- ra.finalizeTimeout
- the VA's configured RPC timeout (x2, for CAA and DCV), plus the SA's configured RPC timeout
- here, 5s hardcoded
I think the solution should be:
- land this with a hardcoded timeout
- file a followup issue to cover all three drainable work sites with a consistent timeout, so we know how long to wait between
kill -TERMandkill -KILLfor a clean shutdown.
| // Aim to leverage the `regID_identifier_status_expires_idx` index on the | ||
| // Authz2 table |
There was a problem hiding this comment.
Nit: This comment suggests that we sometimes might not hit the index. I would simply say "Uses the regID_identifier_status_expires_idx index on the authz2 table."
| // waitForAuthzStatusStable uses an acme client to poll some (a slice of) | ||
| // authorizations to remain stable with a supplied status. It will repeatedly | ||
| // fetch the authorizations five times, accumulating ~5 seconds total wait time | ||
| // with backoff+jitter (which is intended to be _just_ longer than the | ||
| // revokeAuthorizations function custom context), before reporting success. | ||
| // Observed status change of ANY authorization is fatal, ending the loop. | ||
| func waitForAuthzStatusStable(t *testing.T, aClient *client, authzs []string, wantStatus core.AcmeStatus) { |
There was a problem hiding this comment.
This is a pretty expensive thing to add to our unittests, in terms of runtime! I'd like to avoid that. Fast tests are really valuable.
I see below that this is used once, checking that a certain authz doesn't change status. How about another approach:
- Test the "doesn't change status" case in a unittest instead of an integration test.
- In the unittest, explicitly call
ra.drainWG.Wait(). Once it returns, we'll know any work in the RA is done, without having to wait a long amount of time.
3 participants
No description provided.