fix(pam): atomic recording secret mint - #8043
Open
bernie-g wants to merge 4 commits into
Open
Conversation
The mint was an unguarded read-decide-write: two concurrent credential fetches both saw a null session key, both minted, and the later write won. The earlier caller kept a session key and upload token the row no longer matched, so its chunk uploads 400ed forever and anything already uploaded was undecryptable at playback. Guard the update on the key still being null and adopt the winner's secrets when the claim is lost. Verified against Postgres: six concurrent fetches previously issued up to six distinct session keys, now always one.
Four branches in verifyGatewayUploadToken threw the same opaque message, so a malformed token and a token that simply does not match were indistinguishable in logs despite pointing at different causes. Give each branch a distinct error name and keep the client-facing message identical, which leaves the response shape and the router's audit-log trigger unchanged. Also drop the unreachable catch: Buffer.from discards invalid base64 rather than throwing, so a bad encoding shows up as a short decode.
|
💬 Discussion in Slack: #pr-review-infisical-8043-fix-pam-atomic-recording-secret-mint Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
2 tasks
Contributor
 The PR should not merge until the losing claim path reads the winner's key consistently rather than potentially failing under read-replica lag. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8bbc9ed40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The lost-claim path re-read the session through findById, which goes to db.replicaNode(). Replication lag could return the pre-claim row with a null key, so a recoverable race answered with a 500 instead of the winner's session key. Fold the claim into one primary-side statement that sets the secrets only when the row has none and returns whichever key is stored afterwards, so the loser adopts the winner's key with no follow-up read. A missing row is now a 404 rather than a 500, since the only way to get there is the session being deleted concurrently. Also drop the ticket references from the new comments, per the repo's comment rules.
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
1 participant
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.
Context
Recording secrets were minted with an unguarded read-decide-write on
encryptedSessionKey, so two concurrent credential fetches for one session both minted and the later write won, leaving the earlier caller with a session key and upload token the row no longer matched. That gateway's chunk uploads then 400ed forever, and anything it had already uploaded became undecryptable at playback.Fixes PAM-463, which accounted for 99.75% of Infisical Cloud US platform errors. The gateway-side half is in Infisical/cli#392.
Steps to verify the change
Race concurrent credential fetches at one live, never-minted session and check that every response carries the same session key, exactly one carries an upload token, and the stored hash is
sha256of that token.Six concurrent fetches through the real stack, before and after:
Also verified the guarded
UPDATEagainst Postgres directly: eight concurrent claims on one row yield exactly one winner over five runs, where the unguarded version yields eight winners.The second commit only renames error variants, so the four
verifyGatewayUploadTokenbranches are distinguishable in logs. The client-facing message is deliberately unchanged, which keeps the response shape and the router'sPAM_SESSION_UPLOAD_TOKEN_INVALIDaudit trigger working. After deploy, group chunk-route errors byerrorName like /PamUploadToken/: allPamUploadTokenHashMismatchconfirms the race.Type
Checklist
type(scope): short description(scope is optional, e.g.,fix: prevent crash on syncorfix(api): handle null response).