Skip to content

fix(pki): add per-order lock and processing status to ACME order finalize#31993

Open
goingforstudying-ctrl wants to merge 16 commits into
hashicorp:mainfrom
goingforstudying-ctrl:fix/acme-order-finalize-lock
Open

fix(pki): add per-order lock and processing status to ACME order finalize#31993
goingforstudying-ctrl wants to merge 16 commits into
hashicorp:mainfrom
goingforstudying-ctrl:fix/acme-order-finalize-lock

Conversation

@goingforstudying-ctrl

Copy link
Copy Markdown

I ran into GH-31987 while reading through the PKI ACME handler — two concurrent finalize requests on the same order can both pass the readiness gate and both issue distinct certificates, but the order can only track one serial number, so the other cert is orphaned from all order-keyed lookups.

The root cause is that acmeFinalizeOrderHandler does a plain Storage.Get → status check → issue → Storage.Put with no serialization between the read and the write.

This PR adds two layers of protection:

  1. Per-order striped lock (locksutil) around the entire handler body. This serializes concurrent finalize calls for the same order ID so only one can enter the critical section at a time.

  2. Intermediate "processing" status as defense-in-depth. After the lock is acquired and validations pass, the order is immediately saved with ACMEOrderProcessing status before any certificate is issued. If a second request somehow reaches the handler despite the lock (e.g. across cluster nodes on different lock stripes), the order.Status != ACMEOrderReady gate will reject it with ErrOrderNotReady.

The computeOrderStatus helper already treats ACMEOrderProcessing as a non-terminal status, so the intermediate save doesn't change the authorization-recheck behavior for any codepath that recomputes status after the fact.

I went with locksutil over a plain sync.Mutex map because it's the idiomatic pattern in this codebase (used by the database backend, approle, cert auth, and others). Downside is the 256-stripe hash means two different order IDs will occasionally contend on the same underlying RWMutex, but in practice the critical section is brief — just the CSR parsing + cert issuance — and the processing-status gate ensures correctness even when the lock doesn't perfectly isolate.

Not sure about the AcmeAccountStatusValid constant I used in the test setup — I picked it from the existing account status values, but let me know if there's a better constant for "active account" in test contexts.

Closes #31987

@goingforstudying-ctrl goingforstudying-ctrl requested a review from a team as a code owner June 14, 2026 06:30
@vercel

vercel Bot commented Jun 14, 2026

Copy link
Copy Markdown

@goingforstudying-ctrl is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Used to indicate a potential bug secret/pki labels Jun 14, 2026
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 14, 2026
@goingforstudying-ctrl goingforstudying-ctrl requested a review from a team as a code owner June 14, 2026 06:30
@hashicorp-cla-app

hashicorp-cla-app Bot commented Jun 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@vercel

vercel Bot commented Jun 16, 2026

Copy link
Copy Markdown

Deployment failed with the following error:

The `vercel.json` schema validation failed with the following message: should NOT have additional property `public`

Learn More: https://vercel.com/docs/concepts/projects/project-configuration

@vercel

vercel Bot commented Jun 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vault-ui Error Error Jul 2, 2026 8:36pm

Request Review

goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 16, 2026
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 3156c88 to 25d6e65 Compare June 16, 2026 22:06
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 327ae88 to 4b554a5 Compare June 17, 2026 16:34
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 17, 2026
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 17, 2026
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 4b554a5 to 781f872 Compare June 17, 2026 20:22
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 17, 2026
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 781f872 to 6f4e88f Compare June 17, 2026 22:00
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 6f4e88f to 88c5dd8 Compare June 18, 2026 15:15
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 18, 2026
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 18, 2026
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 88c5dd8 to a78aab1 Compare June 18, 2026 18:03
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 2d0229f to c96b950 Compare June 22, 2026 19:26
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from c96b950 to 137190c Compare June 22, 2026 20:09
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 22, 2026
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 22, 2026
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 24, 2026
@goingforstudying-ctrl goingforstudying-ctrl force-pushed the fix/acme-order-finalize-lock branch from 358c754 to ccb4b50 Compare June 24, 2026 00:41
goingforstudying-ctrl added a commit to goingforstudying-ctrl/vault that referenced this pull request Jun 24, 2026
goingforstudying-ctrl and others added 16 commits June 26, 2026 18:05
Prevent concurrent finalize requests on the same ACME order from
double-issuing certificates by:

1. Adding a per-order striped lock (locksutil) around the entire
   read → check → issue → save sequence.

2. Persisting an intermediate ACMEOrderProcessing status before
   certificate issuance as defense-in-depth, so that even if a
   request bypasses the lock (e.g. different lock stripe on
   another cluster node), the status gate at the top of the
   handler will reject it.

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

Labels

bug Used to indicate a potential bug secret/pki size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PKI ACME: acmeFinalizeOrderHandler lacks per-order locking — concurrent finalize double-issues and orphans a cert from order-keyed tracking

1 participant