fix(pam-access,device-auth): make single-use credentials actually single-use - #88
Merged
Merged
Conversation
This was referenced Sep 6, 2026
guimard
force-pushed
the
fix/atomic-consumption
branch
from
September 6, 2026 13:28
d30cab0 to
488b52d
Compare
…gle-use Two one-shot credentials were read and deleted in separate steps with a wide window between them, and LLNG sessions have no locking, so two concurrent requests could both succeed. pam-access (#53): /pam/verify read the PAMTOKEN session, ran ~145 lines of checks including a persistent-session load, and only then removed it. It now consumes the token immediately after the lookup — every exit path discarded it anyway, so no outcome changes — and refuses when the delete did not succeed. `noCache => 1` makes that delete re-read the backend, so a token another node already consumed is seen as gone. device-authorization (#68): the device_code delete was unconditional, so two token requests that both read `approved` both minted a full token set — and because the legitimate device's exchange also succeeded, nothing looked wrong. _deleteDeviceAuth now reports whether it won, and _generateTokens refuses to mint otherwise. Both consumed device sessions are also evicted cluster-wide with an `unlog` event. Store->remove only drops the cache of the node doing the write (its own `#TODO: remove cache on all LL::NG instances`), which had turned a millisecond race into a window as wide as the code TTL. This does not give the store a compare-and-swap it does not have; it narrows the window to two adjacent round-trips and makes the verdict conditional on the delete, which is as far as a plugin can go. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
guimard
force-pushed
the
fix/atomic-consumption
branch
from
September 6, 2026 14:21
488b52d to
a7e11f0
Compare
… detected The comments credited the delete. It is the re-tie inside Common::Session->remove that fails for the loser: the record is gone, tying dies, remove returns false. The delete itself is unconditional -- Store::File guards its unlink with `if (-e $file)` and no else, so deleting an absent record is a silent success. That makes `noCache => 1` the load-bearing part rather than a hardening detail: without it the re-tie would be served by the node-local cache and both callers would be told they won. Also state the assumption this rests on (the backend fails to retrieve a missing id instead of returning an empty record -- true of File, DBI and REST), restate that atomicity is not claimed, and note that on the exchange path the grant hook has already run when the delete happens, so a losing exchange has executed its side effects before being refused. Left as is deliberately: moving the delete ahead of the hook would change what the existing error paths do. No behaviour change. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
t/37 cleared the 300s submission lockout with Time::Fake->offset("+10m"), but
the device code only lives oidcServiceDeviceAuthorizationExpiration (600s),
counted from a request made in real time at the top of the file. The jump
therefore landed exactly on the code's expiry, with a margin equal to the real
seconds the file had spent getting there: zero on a fast machine, negative on
a loaded runner. There the submission was refused as expired, the approval
never happened, and four assertions failed downstream -- on either LLNG ref,
which is what gave it away as timing rather than version.
It passed as often as it did because the assertion could not tell the two
apart: qr/deviceApproved|success/ also matches the refusal page. Reproduced
deterministically by inserting real elapsed time before the offset, which
fails the same four assertions, and fixed by jumping +6m -- past the 300s
lockout with four minutes to spare, well inside the 600s code TTL -- and by
asserting the approval rather than a substring both pages share.
Pre-existing on main; it surfaced here because two runs in a row were unlucky.
Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
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.
Closes #53, closes #68.
Stacked on #87 (same files); merge that one first.
pam-access —
/pam/verify(#53)The one-time PAM token was read at step 3 and removed at step 7, with ~145
lines of checks and a
getPersistentSessionround-trip in between. Twoconcurrent calls — a login racing a sudo,
scp host1: host2:— both read thetoken before either deleted it, and both answered
valid: true.Consumption now happens immediately after the lookup. Every exit path below
already discarded the token, so no outcome changes; what changes is that the
window is two adjacent store round-trips instead of a whole request. The
delete passes
noCache => 1so it re-reads the backend rather than thenode-local cache, and a verify that did not win it answers invalid with a
PAM_AUTH_TOKEN_NOT_CONSUMEDaudit record.A store failure lands on the same path. For a one-time credential, refusing is
the right reading of "I cannot tell whether this was already used".
oidc-device-authorization — the device_code (#68)
_deleteDeviceAuthdeleted unconditionally, so two token requests holding thesame approved code (and, under PKCE, the same verifier) both minted a full
token set. The point is not that the attacker gains access they could not
otherwise get — they already hold the code — but that the legitimate device's
exchange also succeeds, so the theft leaves nothing to notice.
_deleteDeviceAuthnow returns whether it won the delete, and_generateTokensrefuses (
invalid_grant, audited asISSUER_OIDC_DEVICE_AUTH_DOUBLE_EXCHANGE)when it did not.
Cluster-wide cache eviction
The issue's "real amplifier":
Common::Apache::Session::Store->removeonlyevicts the cache of the node performing the write — its own
#TODO: remove cache on all LL::NG instances._deleteDeviceAuthdeletes its sessionsdirectly instead of going through
Portal::Main::Run'spublishEvent('unlog'),so a second node kept serving a deleted
device_authfrom cache for the wholecode TTL (600 s in the shipped demos). Both consumed sessions now publish the
event.
Not claimed
The store has no compare-and-swap and every standard backend installs
Apache::Session::Lock::Null. This narrows the window and makes the verdictconditional on the delete; it does not make consumption atomic. That would
need a core primitive.
Tests
pam-access769,oidc-device-authorization282,ssh-ca565,oidc-device-organization52 — all green. New coverage in t/01 and t/36drives the lost-delete deterministically, and asserts both sessions are
unlogged.
https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL