letsencrypt / boulder

An ACME-based certificate authority, written in Go.
Mozilla Public License 2.0
5.22k stars 608 forks source link

Add feature flag to disable pending authz reuse #7836

Open aarongable opened 1 week ago

aarongable commented 1 week ago

Pending authz reuse is a nice-to-have feature because it allows us to create fewer rows in the authz database table when creating new orders. However, stats show that less than 2% of authorizations that we attach to new orders are reused pending authzs. And as we move towards using a more streamlined database schema to store our orders, authorizations, and validation attempts, disabling pending authz reuse will greatly simplify our database schema and code.

CPS Compliance Review: our CPS does not speak to whether or not we reuse pending authorizations for new orders.

Part of https://github.com/letsencrypt/boulder/issues/7715

github-actions[bot] commented 1 week ago

@aarongable, this PR adds one or more new feature flags: NoPendingAuthzReuse. 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:".

jsha commented 1 week ago

One good strategy for unittest coverage might be: locally, comment out the flag check such that the new code (GetValidAuthorizationsRequest) runs unconditionally, run the RA test and see what fails, then fix it / adjust it to run with and without the flag.

aarongable commented 1 week ago

Yeah, I already did that, and the result is that precisely one test fails -- one that is expecting authz reuse and no longer gets it. We don't actually have any tests currently that cover the distinction between pending and valid authz reuse. So I'm attempting a larger rewrite and unification of the NewOrder tests to see if that can put us in a better state.