secret/pki: lock ACME order finalize to prevent concurrent double-issuance#32004
Closed
Har1sh-k wants to merge 1 commit into
Closed
secret/pki: lock ACME order finalize to prevent concurrent double-issuance#32004Har1sh-k wants to merge 1 commit into
Har1sh-k wants to merge 1 commit into
Conversation
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/concepts/projects/project-configuration |
dosubot
Bot
added
the
size:M
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
…uance acmeFinalizeOrderHandler loaded an order, checked it was in the ready state, issued a certificate, and saved the order back, with no lock held across those steps. Two concurrent finalize requests for the same order could both pass the ready check and each issue a certificate. The order only records one serial number, so the last save wins and the other certificate is left orphaned from every order-keyed lookup, including /acme/order/<id>/cert and the account based revocation path that walks it. Take a per-order lock from a striped pool (locksutil) around the finalize critical section so requests for the same order run one at a time. Finalize is forwarded to the active node (ForwardPerformanceStandby), so all finalizes for an order run on a single node and an in-memory lock is sufficient. The order keeps its existing pending to valid lifecycle, with ready and processing still computed on read, so a failed issuance leaves the stored order untouched and the client can retry. Add regression tests that run concurrent finalizes for one order and assert a single certificate is issued and the rest are rejected, that a failed issuance leaves the order finalizable so a retry succeeds, and that the per-order lock is stable for a given order id. Fixes hashicorp#31987
Har1sh-k
force-pushed
the
fix/acme-finalize-per-order-lock
branch
from
June 19, 2026 18:09
5e871bf to
fd3da6d
Compare
Collaborator
Copy workflow completed!
|
Collaborator
Copy pull request failed!
|
Collaborator
|
Hi @Har1sh-k, thanks for the contribution! I really appreciate all the details you provided and the nice tests, they made it easy to review. I've merged the changes via a PR in our enterprise repo, as per our usual process. They won't be included in the next release because we missed the window on that, but they'll go out in the following one. |
eualin
pushed a commit
to eualin/vault
that referenced
this pull request
Jul 15, 2026
…ent double-issuance into main into ce/main (hashicorp#16550) * Copy hashicorp#32004 into main * secret/pki: lock ACME order finalize to prevent concurrent double-issuance acmeFinalizeOrderHandler loaded an order, checked it was in the ready state, issued a certificate, and saved the order back, with no lock held across those steps. Two concurrent finalize requests for the same order could both pass the ready check and each issue a certificate. The order only records one serial number, so the last save wins and the other certificate is left orphaned from every order-keyed lookup, including /acme/order/<id>/cert and the account based revocation path that walks it. Take a per-order lock from a striped pool (locksutil) around the finalize critical section so requests for the same order run one at a time. Finalize is forwarded to the active node (ForwardPerformanceStandby), so all finalizes for an order run on a single node and an in-memory lock is sufficient. The order keeps its existing pending to valid lifecycle, with ready and processing still computed on read, so a failed issuance leaves the stored order untouched and the client can retry. Add regression tests that run concurrent finalizes for one order and assert a single certificate is issued and the rest are rejected, that a failed issuance leaves the order finalizable so a retry succeeds, and that the per-order lock is stable for a given order id. Fixes hashicorp#31987 * Rename CL --------- Co-authored-by: Harish Kolla <hkolla03@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
acmeFinalizeOrderHandlerloads an order, checks that it is in thereadystate, issues a certificate, then saves the order back, without holding any lock across those steps. Two finalize requests for the same order that arrive at the same time can both pass thereadycheck and each issue a separate certificate. The order only stores a singleCertificateSerialNumber, so the last save wins and the other certificate is left orphaned from every order-keyed lookup, including/acme/order/<id>/certand the account-based revocation path that walks orders. The orphaned certificate is fully valid for the named identifiers and cannot be found or revoked through the order.RFC 8555 section 7.4 models finalize as a single transition (ready to processing to valid) that is meant to happen once per order.
Fix
Take a per-order lock from a striped
locksutilpool around the finalize critical section, so finalize requests for the same order run one at a time. The lock pool lives onacmeStateand is created inNewACMEState.Finalize is forwarded to the active node (
ForwardPerformanceStandbyis set on the path), so all finalizes for a given order run on one node and an in-memory lock is enough to serialize them. The order keeps its existing pending to valid lifecycle, withreadyandprocessingstill computed on read bycomputeOrderStatus, so a failed issuance leaves the stored order untouched and the client can retry.I looked at also persisting an intermediate
processingstatus on disk, but Vault only ever persists orders as pending then valid (readyandprocessingare computed), and writing them back caused two problems: an order could get stuck inprocessingif the node failed over mid-issuance, and reverting to a storedreadywould skip the authorization recheck thatcomputeOrderStatusdoes for non-final orders. The lock on its own fixes the race without changing the persisted state machine.Testing
Added tests in
builtin/logical/pki/path_acme_order_test.go, all passing under-race:TestACMEFinalizeOrder_ConcurrentRequestsIssueSingleCertruns several concurrent finalizes for one order and asserts exactly one succeeds, the rest are rejected withorderNotReady, and only one certificate is stored and referenced by the order.TestACMEFinalizeOrder_FailedIssuanceLeavesOrderRetryablemakes issuance fail once and confirms the order stays finalizable, then a retry succeeds.TestACMEFinalizeOrder_OrderLockIsStablePerOrderchecks the per-order lock is stable for a given order id.Closes #31987