feat(pam-access): verify X-Signature-256 / X-Timestamp / X-Nonce - #93
Conversation
guimard
left a comment
guimard
left a comment
There was a problem hiding this comment.
Found a blocker while reviewing: the nonce is claimed before the HMAC is verified. _claimNonce writes a PAMNONCE record into the shared session backend for any request whose headers parse and whose timestamp is inside the window — both attacker-controlled — so an unauthenticated caller can fill globalStorage with one record per request without knowing the secret. That's a storage-exhaustion vector against the exact shared store this PR leans on, and the PR body's own "checks in order of cost" argument points the other way: the HMAC (one SHA-256 over a short message) is cheaper than the two storage roundtrips _claimNonce makes.
Confirmed empirically: bad signature with nonce N → 403 invalid signature; then the correct signature with the same N → 403 Request nonce refused (N was burned by the bad-signature request).
Suggested fix: order timestamp → HMAC → nonce claim.
Minor: an unknown mode value (typo, e.g. optionnal) silently behaves as required — fail-closed is acceptable, but a one-line warning would help.
Everything else checks out: wire format pinned against the issue's worked example, constant-time compare, fail-closed on empty secret, hashStore => 0 consistent with the #37 lesson, nonce expiry arithmetic correct (reaped at t0 + window).
_claimNonce writes a PAMNONCE record into the shared session backend. It ran before the signature was checked, so every input it depended on — the header set and the timestamp — was attacker-controlled: an unauthenticated caller could create one storage record per request, against the very store this protection leans on, and could burn the nonce of a captured request before its legitimate retry arrived. Order is now timestamp -> HMAC -> nonce claim. The cost argument the original comment made points this way too: one SHA-256 over a short message is cheaper than the two storage roundtrips _claimNonce makes. Also warn once when pamAccessRequestSigningMode holds a value that is not off/optional/required. Falling through to 'required' is the right default, but a typo silently selecting the strictest mode is a production surprise. t/17 pins both: a forged signature no longer invalidates the nonce, and a misspelled mode still refuses unsigned requests. Reported in review of #93. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
|
You are right, and the PR body's own cost argument was the tell. Fixed in c8a5d39: the order is now timestamp → HMAC → nonce claim.
Minor also done: an unrecognised mode value still falls through to README updated to state the order and why the nonce claim is last. |
0988d98 to
d4a5a71
Compare
open-bastion's PAM/NSS client has been signing every call to the portal when
request_signing_secret is set, and nothing on this side ever read the headers.
The client signed, nothing checked: the replay-protection chain was a no-op
that cost bandwidth and bought nothing. This is the missing verifier, built
against the wire format settled by open-bastion#188.
message = <timestamp>.<nonce>.<method>.<path>.<body>
HMAC-SHA256, key = the raw bytes of the shared secret
Checks run in order of cost — timestamp, then nonce, then HMAC — so a stale
request never makes the portal hash anything. The nonce is single-use, cached
one session per nonce in the shared backend and expiring with the window, so
it holds across workers and nodes. The HMAC is compared in constant time and
computed over the raw body bytes, before anything decodes the JSON.
Three modes, because turning this on is a breaking change for any fleet where
some hosts have the secret and some do not:
off (default) headers ignored.
optional a signed request must verify; an unsigned one passes. Run this
while rolling the secret out.
required headers mandatory.
`optional` waives the requirement to sign, never the requirement to sign
correctly: a bad signature is refused in every mode but off. A configured mode
with an empty secret refuses everything rather than waving it through.
One gate for all six endpoints, via _checkCaller.
Closes #81
Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
_claimNonce writes a PAMNONCE record into the shared session backend. It ran before the signature was checked, so every input it depended on — the header set and the timestamp — was attacker-controlled: an unauthenticated caller could create one storage record per request, against the very store this protection leans on, and could burn the nonce of a captured request before its legitimate retry arrived. Order is now timestamp -> HMAC -> nonce claim. The cost argument the original comment made points this way too: one SHA-256 over a short message is cheaper than the two storage roundtrips _claimNonce makes. Also warn once when pamAccessRequestSigningMode holds a value that is not off/optional/required. Falling through to 'required' is the right default, but a typo silently selecting the strictest mode is a production surprise. t/17 pins both: a forged signature no longer invalidates the nonce, and a misspelled mode still refuses unsigned requests. Reported in review of #93. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
504b099 to
1d5079f
Compare
The rollout guide said "deploy optional, roll the secret out, switch to required". Following it breaks the fleet: the gate covers all six /pam/* endpoints and the Open Bastion client signs two of them. Checked in the client sources: add_signing_headers has exactly two call sites, /pam/verify (ob_client.c:726) and /pam/authorize (ob_client.c:1137). /pam/heartbeat says so in its own comment -- "no Authorization header, no request signing", it authenticates by the refresh_token in its body. /pam/bastion-cert sends Content-Type and Bearer only (ob-cert-daemon.c), and nothing calls /pam/userinfo. Heartbeat is the bad one: it is how every enrolled host renews its access token, so `required` breaks nothing at the moment it is switched on and takes the whole fleet down hours later, together, when the tokens still in hand expire. So: state what the client signs today, make "wait for the client to sign heartbeat and bastion-cert" an explicit step of the rollout rather than an assumption, and say that `optional` is a useful destination in its own right -- it already refuses a bad signature on the two endpoints that consume credentials. Also note the sizing that becomes relevant once heartbeat is signed: two store round-trips per signed request, one resident record per nonce per window, and heartbeat beats on a timer whether anyone logs in or not. No code change; the verifier was never the problem. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
1 participant
Closes #81 (linagora/open-bastion#188). Rebased onto
main; #94 follows.The PAM/NSS client signs some of its calls to the portal when
request_signing_secretis set, and nothing on this side read the headers —the replay-protection chain was a no-op that cost bandwidth and bought
nothing. This is the verifier, built against the wire format the issue
documents.
What it does
Four literal
.separators, always present; a bodyless request signs the emptystring.
<path>has no scheme, host or query.<body>is the raw bytes assent — the HMAC is computed in
_checkCaller, before anything decodes andre-encodes the JSON.
Checks run timestamp → HMAC → nonce:
pamAccessRequestSigningWindow(300 s), first — thecheapest check, and it bounds both how long a captured request stays
replayable and how big the nonce store has to be.
before verifying would let an unauthenticated caller fill the shared
session backend with one record per request, and burn the nonce of a
request it had captured before the legitimate retry arrived. One session
per nonce keyed on its digest, expiring with the window, so it holds across
workers and nodes.
One gate for all six endpoints, via
_checkCaller.Rollout —
requiredis not deployable yetThe gate covers all six
/pam/*endpoints. The client signs two:/pam/verifyand/pam/authorize(add_signing_headershas exactly those twocall sites,
ob_client.c:726and:1137)./pam/heartbeatsays so in its owncomment — "no Authorization header, no request signing", it authenticates by
the
refresh_tokenin its body./pam/bastion-certsends Bearer only(
ob-cert-daemon.c), and nothing calls/pam/userinfo.Heartbeat is the dangerous one: it is how every enrolled host renews its access
token, so
requiredwould break nothing at the moment it is switched on andtake the whole fleet down hours later, together, when the tokens still in hand
expire.
off(default)optionalrequiredSo the order is: deploy
optional, roll the secret out, wait for the clientto sign heartbeat and bastion-cert, then
required.optionalis a usefuldestination in its own right — it already refuses a bad signature on the two
endpoints that consume credentials.
A partially signed request is refused as malformed in both modes: it is not the
"old client that does not sign" case
optionalexists for. A configured modewith an empty secret refuses everything rather than waving it through, and an
unrecognised mode value is treated as
requiredwith a once-per-workerwarning.
Documented in the README, with the sizing that matters once heartbeat is
signed: two store round-trips per signed request, one resident record per nonce
per window, and heartbeat beats on a timer whether anyone logs in or not.
Known limit
Read-then-create on the nonce cache is not atomic — the LLNG store has no
insert-if-absent, the same wall #87/#88/#95 ran into. Two replays landing in
the same instant can both pass. That is a far smaller target than the 300 s
window this closes, and the comment says so rather than implying otherwise.
Tests
t/17-PamAccess-RequestSigning.t, 44 assertions, including the issue's workedexample verbatim: all three modes, the timestamp window in both directions,
nonce replay, body tampering, a signature for a different path not
transferring, four shapes of malformed header, the empty-secret refusal, the
unknown-mode fallback, and that a bad signature does not burn the nonce.
pam-access 959 total, ssh-ca 610, green.
https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL