fix(ssh-ca): bound /ssh/sign with a rate limit and a per-user quota - #90
Conversation
guimard
left a comment
There was a problem hiding this comment.
Verified: updatePersistentSession (Run.pm:665) persists both session copies; respHeaders/spliceHdrs carry Retry-After; the $resign exemption matches _storeCertificate's replace-by-fingerprint semantics (a re-signature under a different label also replaces, so the quota can't be bypassed).
Two nits:
- With
disablePersistentStoragethe counter is never written and the rate limit is silently inactive — consistent with the rest of the plugin (certs aren't persisted either) but worth a README note. - "Charged before parsing" in the commit message isn't quite true —
_jsonBodyOrRejectparses first (cheap, harmless).
Also a conservative edge: re-signing an expired cert at the quota is refused although it wouldn't grow the active set (the expired record is replaced by fingerprint). Fine, but worth knowing.
|
Both nits documented in the README (commit 22b9da2), plus the edge you flagged:
On the commit message: correct, |
f3dcd00 to
13873dd
Compare
Every signature forks ssh-keygen twice and rewrites the WHOLE KRL as a
read-modify-write, and re-signing a key you already hold appends the
superseded serial — which the plugin allows on purpose. Nothing stopped a
shell loop: the cost per call grows with the KRL, and every appended serial is
loaded by every sshd on every backend, so an authenticated user could deny
service to the whole fleet.
Two bounds, both per user, both disabled by 0:
sshCaSignMaxPerHour (20) -> 429 with Retry-After, audited
SSH_CA_SIGN_RATE_LIMITED. Fixed hourly window
counted in the user's own session, which is
already shared across nodes.
sshCaMaxCertsPerUser (20) -> 409, audited SSH_CA_CERT_QUOTA_EXCEEDED. A
re-signature replaces a record and is never
counted, so a user at the quota can still
rotate a key.
The rate limit is charged before any parsing, policy check or fork, so an
abusive caller cannot make the portal do the expensive work.
The KRL itself is deliberately NOT capped: refusing to record a revocation
would be a silent fail-open. Bounding the inputs bounds it instead.
A non-numeric setting falls back to the default rather than to unlimited, so a
Manager typo cannot quietly remove a limit.
Closes #63
Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
…tability The plugin carries the most identity-critical code in the chain — it replaces the approving admin's session with a synthetic one and derives _deviceId, the value the whole bastion vouching chain keys on — and three things about it were pinned nowhere. _deviceId stability across a refresh. It is documented as "stable across refreshes" and PamAccess::_callerId returns it as the bastion identity, so a device whose id changed at refresh would silently lose its vouchers. Now exercised end to end through /pam/heartbeat (pam-access is pulled in as a test-only dependency), including two consecutive heartbeats. The AllowOffline=0 + ownership=organization combination: an access token is issued, no refresh token of either kind, and the identity swap still happens. The swap itself: the token points at the synthetic session and not the admin's, no admin attribute survives the wholesale session copy, userinfo answers as the device, and the device token keeps working after the approving admin's SSO session is removed — which is the entire point of organizational ownership. Plus the negative: an RP without ownership=organization gets no _deviceId. It also pins the constraint the plugin's own design comment leans on: the core /oauth2/token refresh grant answers invalid_grant for these tokens, which is why Open Bastion refreshes through /pam/heartbeat. 52 -> 125 assertions. Closes #71 Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
Both counters live in the user's session: with `disablePersistentStorage` neither limit applies. And the quota's re-signature exemption only covers *active* records, so re-signing an already-expired key at the quota is refused although it would replace rather than add. Neither is a behaviour change; both were unstated. Reported in review of #90. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
721387b to
356f20c
Compare
The comment claimed the limit runs "before any parsing". It does not: _jsonBodyOrReject decodes the body on the line above. What the gate really precedes is the expensive work -- key validation, the policy check, the ssh-keygen fork and the KRL rewrite -- which is what issue #63 is about. Say that, and say why the body decode is deliberately left in front: a malformed body must answer 400 whether or not the caller is over quota, or the limit would turn a client bug into a misleading 429. No behaviour change. Claude-Session: https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL
Closes #63. Stacked on #89 → #88 → #87.
The mechanism
/ssh/signexplicitly permits reusing a label for the same key, and eachre-signature calls
_updateKrlon the superseded serial. Nothing rate-limitedit and the core has no limiter for plugin REST routes.
Every call forks
ssh-keygentwice and rewrites the entire KRL as aread-modify-write, so the cost grows with the KRL: the amplification is
superlinear. Each appended serial is then loaded by every
sshdon everybackend. A shell loop from one authenticated user is a denial of service
against the whole fleet, not just the portal.
Chosen mechanism
I had flagged that this one needed a decision on the limiter. Going with the
option that adds no dependency and no new storage, since the alternatives
(CrowdSec, a dedicated bucket store) both cost more than the problem is worth
here:
A fixed hourly window counted in the user's own session. The session store
is already shared across nodes, so the count is cluster-wide for free. Two
racing signatures can under-count by one — the store has no atomic increment —
which is irrelevant for a limit whose job is to bound a loop, not to be exact.
Say the word if you'd rather have CrowdSec here and I'll swap it.
sshCaSignMaxPerHour20Retry-After,SSH_CA_SIGN_RATE_LIMITEDsshCaMaxCertsPerUser20SSH_CA_CERT_QUOTA_EXCEEDED0disables either. A non-numeric value falls back to the default rather thanto unlimited, so a Manager typo cannot quietly remove a limit.
The rate limit is charged first, before parsing, the key policy check or
any fork — an abusive caller must not be able to make the portal do the work.
A re-signature of a key the user already holds replaces its record, so it
never grows the set and is not counted against the quota: someone sitting at
the cap can still rotate. A new key needs a
/ssh/myrevokefirst.The KRL is deliberately not capped
Refusing to record a revocation because the file is large is a silent
fail-open — the certificate stays usable. Bounding the two inputs bounds the
KRL instead, without ever dropping a revocation.
Note on the issue's second point
The claim that
/ssh/certsdoes a full session scan holds only for backendswithout a native
searchOnExpr; theApache::Session::Browseable::*backendsimplement it as an indexed query. Nothing to do there.
Tests
New
t/04-SSHCA-limits.t, 32 assertions: the quota, the re-signatureexemption, freeing a slot by revoking, the 429 with its header and body, the
limit being per user, the window rolling over,
0disabling both, and amalformed setting falling back to the default rather than to unlimited.
ssh-ca 602 total, pam-access 769 — green.
https://claude.ai/code/session_01GfBG36HfzjGy8W9rJQbxBL