fix(server): let the SSH server decide the web terminal's public key access - #7049
Open
otavio wants to merge 3 commits into
Open
fix(server): let the SSH server decide the web terminal's public key access#7049otavio wants to merge 3 commits into
otavio wants to merge 3 commits into
Conversation
publicKeyAuth.Offer had no test, and it decides two separate things. The first is the agent version floor. Agents 0.5.x and earlier do not validate the public key request and may panic, so the server refuses public key authentication against them unless the instance sets SHELLHUB_ALLOW_PUBLIC_KEY_ACCESS_BELLOW_0_6_0. Customers still run those agents, and nothing held the floor in place: AllowPublickeyAccessBelow060, ErrUnsuportedPublicKeyAuth and ErrInvalidVersion appeared in no test file. Cover it from both sides, along with the opt-in that lifts it, the "latest" version that skips the comparison, and the v-prefixed spelling semver accepts. The two parse failures are pinned as they behave rather than as they arguably should: an agent whose version does not parse, including one reporting no version at all, is refused with ErrInvalidVersion while the opt-in is off. That is worth knowing before anyone changes it, because it means a version-less agent cannot use a public key on a default instance. The second is whether the key may act as the login on the device, which is the lookup and the two evaluations. Cover each way it can refuse — either evaluation answering false, either failing outright, and the lookup missing — because the commits that follow move the only other copy of that decision, and a decision with no test is a decision that can be moved wrongly. A refused version case asserts through the service mock rather than the error alone. mockery fails a MockService built with no expectations if anything calls it, so those cases prove the floor returned before reaching the store, not merely that some error came back. The opt-in is a package-level global installed by Configure, so the helper captures and restores the whole config around each case.
…fers publicKeyAuth.Offer skipped the public key lookup and both key evaluations whenever the offered key's fingerprint matched the magic key's. Nothing can present that key. magickey.GetReference is a process-local RSA key generated once at startup with sync.OnceValue. Its public half is never persisted, never published and never sent to a client, so no caller can offer it and the branch has been dead rather than dangerous. It is removed for what it hides, not for what it risks. It dates from 692d763 (2022), when the web terminal's GetAuth ran the key evaluations itself and then dialled localhost:2222 signing with the magic key. The bypass is what let that loopback connection through without evaluating the key a second time. The web terminal stopped presenting the magic key long ago — it now signs with the browser's key or with the user's registered key — so the bypass lost its only caller while the duplicated evaluation it existed for stayed behind. That duplication is removed next. With the branch gone the evaluations are unconditional, which is what every caller already got, and Offer ends in an explicit nil. The trailing `return err` it replaces could only ever return the already-checked error from building the magic key, so it was nil on every path that reached it — a value that read like a result and was not one. The agent version floor above is untouched and still returns before any of this, so agents 0.5.x and earlier are refused exactly as before.
…access getAuth evaluated the offered key itself — GetPublicKey, EvaluateKeyUsername, EvaluateKeyFilter and the refusal — before dialling the SSH server over the loopback. The server then evaluated the same key again on that connection, so every browser session using a registered key paid for the decision twice and two copies of it had to agree. They did not. The copies disagreed on the sentinel, and worse, on the access mode. The server picks between publicKeyAuth and ResolveKeyAuth on the namespace's SSH access mode; getAuth knew nothing of it and applied the legacy key filters even for an identity-mode namespace, where the server applies none. The browser could be refused a key the server would have admitted. Fetch the key and stop there. The lookup stays because the Signer needs the key material to present, and the tenant it is bounded by comes from the device — that is a read, not a decision. Branch one already works this way: a browser-held key is presented with no evaluation at all, and the server is what admits or refuses it. The registered-key branch now matches, so there is one decision, in the place that knows the access mode. Moving the decision moves where its refusal surfaces, and that had to be carried across or the browser would have been told the wrong thing. newSession discards getAuth's error and answers ErrGetAuth, so a key the namespace refused already reached the console as "the selected public key could not be used" rather than as ErrForbiddenPublicKey — which nothing returned, and which the console has therefore been mapping for nothing. With the decision in the server the refusal arrives as a failed handshake instead, and answering ErrAuthentication there would have told a public key user their password was wrong, offering a reconnect that cannot help. A handshake that fails on a public key credential now answers ErrForbiddenPublicKey, which is what the console's unused entry was always written for: "this public key is not authorized for this device". ErrEvaluatePublicKey goes: nothing returns it once the decision moves, and a sentinel that cannot be returned reads like a case that can happen. ErrForbiddenPublicKey stays in exitLogLevel's warn list, where it is now reachable for the first time.
Member
Author
|
/review |
Code Review CompleteThe automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment |
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.
What
The web terminal evaluated an offered public key itself and then dialled the SSH server, which
evaluated the same key again. One decision now lives in one place — the SSH server — and the web
terminal only fetches the key material it has to present.
Why
The two copies had drifted. They disagreed on the error sentinel, and more importantly on the
namespace's SSH access mode: the server chooses between
publicKeyAuthandResolveKeyAuthbasedon it, while
getAuthknew nothing about it and applied the legacy key filters even in anidentity-mode namespace, where the server applies none. A browser session could be refused a key
the server would have admitted.
Not a privilege escalation — the duplicate was stricter than the server, never laxer.
Found by an architecture review of the SSH path; there is no issue for it.
Changes
server/ssh/web/session.go—getAuthkeepsGetDeviceandGetPublicKey, because theSignerneeds the key material to present and the device supplies the tenant that bounds thelookup. The two
Evaluate*calls and the refusal go. This is how the browser-held-key branchabove it already worked: present the key, let the server admit or refuse it.
server/ssh/web/session.go— a handshake that fails on a public-key credential now answersErrForbiddenPublicKeyinstead ofErrAuthentication. Without this the change would have told apublic-key user their password was wrong and offered a reconnect that cannot help. The console
has an entry for
ErrForbiddenPublicKey— "this public key is not authorized for this device" —that nothing has ever been able to trigger, because
newSessiondiscardsgetAuth's error andanswers
ErrGetAuth. That mapping becomes reachable for the first time; no UI change needed.server/ssh/session/auther.go— removes the magic-key bypass, which skipped the lookup andboth evaluations when the offered key matched
magickey.GetReference(). That key is aprocess-local RSA key generated at startup; its public half never leaves the process, so nothing
can present it. It dates from 2022, when the web terminal signed the loopback dial with it — the
bypass existed because the web terminal had already evaluated the key. It lost its caller long
ago while the duplication it excused stayed. Also removes a trailing
return errthat could onlyever be nil.
server/ssh/session/auther_test.go— new.Offerhad no test and decides two things.Testing
The agent version floor is the thing to check. Agents 0.5.x and earlier do not validate the
public key request and may panic, so the server refuses public-key auth against them unless
SHELLHUB_ALLOW_PUBLIC_KEY_ACCESS_BELLOW_0_6_0is set. Customers still run those agents. That rulehad no test at all before this PR.
The test commit is deliberately first and touches no production code, so it can be checked against
master:Both were verified in a worktree. The floor block in
Offeris byte-identical tomaster.Two behaviours worth a reviewer's eye:
version does not parse — including one reporting no version at all — is refused with
ErrInvalidVersionwhile the opt-in is off. That means a version-less agent cannot use a publickey on a default instance. Preserved deliberately; say so if it should change.
MockServicewith no expectations, so they fail if the floorever lets a call reach the store rather than merely returning some error.
Not done here, and deliberately: a nil-guard on
Device.Info(DeviceToModelalways populates it,so it is unreachable and the guard would be speculative), and carrying the server's refusal reason
across as an SSH banner. The latter is worth doing — it would give plain SSH clients the explanation
they get nothing of today — but a banner in
Offerwould fire on every key a client merely queries,so it belongs in
Evaluateand is its own change.Scope: legacy access mode only
Worth knowing before reading the diff.
resolveKeyAuthpicks betweenpublicKeyAuthandResolveKeyAuthon the namespace'sssh_access_mode, and the column defaults toidentity. Anamespace created today therefore never reaches
publicKeyAuth.Offer— the function this PRchanges — and the web terminal's registered-key branch is likewise a legacy-mode path.
So the blast radius is narrower than the diff suggests: namespaces still on legacy mode.
Identity-mode namespaces are untouched, which is also why
getAuthapplying legacy key filtersregardless of mode was wrong rather than merely redundant.
Verified against a running instance
Beyond the unit tests, the change was exercised end-to-end on a live
v0.27.0-rc.8community devstack (real agent, real SSH client, real WebSocket bridge), with a namespace switched to legacy
mode. Three registered keys with different filters: unrestricted,
username=nobodyxyz,hostname=no-such-host.SSH — unrestricted key connects and runs a command; username-filtered and device-filtered keys
are both refused, server logging
refused the offered public key error="failed to evaluate the provided public key". That is the evaluation running unconditionally with the magic-key branchgone.
Web terminal — completing the real signature exchange over
/ws/ssh, the unrestricted keyreaches
{"kind":5}(session established). The unauthorized key is refused, and the refusal is thereason the third commit exists. Same fixtures, the fix toggled:
failed to authenticate to devicefailed to use the public key for this actionA wrong password still returns
failed to authenticate to device, so the discrimination is notover-broad.
Old-agent floor — with the device reporting
0.5.9, the key that connects above is refused,server logging
connections using public keys are not permitted when the agent version is 0.5.x or earlier. The hard constraint holds on a live server, not only in the unit tests.