Repository navigation
Safe naming and authN checks - #155
Open
GauravPawar101 wants to merge 3 commits into
Open
GauravPawar101 wants to merge 3 commits into
GauravPawar101 wants to merge 3 commits into
Conversation
Contributor
Author
|
@ekzhang I was not able to update description due to some reason. Just did it, lmk if any issues or discrepancies |
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
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.
Summary
Two hardening fixes to the session WebSocket endpoint, plus incidental build/CI fixes.
{name}path parameter is now validated before it is used. Session names are interpolated into an upstream URL when this server proxies a connection to a mesh peer (proxy_redirect→format!("ws://{host}/api/s/{name}")) and are also used as a storage key in the mesh (mesh.get_owner(name)). The value arrived from the wire as an unvalidatedString, so a percent-encoded%2F,%2E%2E,%20, etc. decoded into a name containing path metacharacters that changed the meaning of the proxied URL and the key namespace.handle_socketcalledsession.sync_now()before reading the client's auth message, so any anonymous socket connection allocated a UID from the persisted counter and woke the storage sync task — forcing a snapshot + write to Redis.No change for legitimate clients: session names are generated server-side as
rand_alphanumeric(10)(grpc.rs:51), which is always within the new validator's alphabet.Changes
SafeName— a validated identifier newtypeNew module
crates/sshx-core/src/name.rs, re-exported fromsshx_core(pub use name::{InvalidName, SafeName}):rand_alphanumeric, soFromStraccepts only ASCII alphanumerics. Everything that could change the meaning of an interpolated URL path —/,.,%,\,?,#, whitespace, control bytes, NUL, non-ASCII — is rejected outright rather than escaped.MAX_LEN = 64bytes. Far above the length of any generated name (10, and up to 22 for the server secret); the bound exists only to cap work on attacker-controlled input.SafeNamedoc comment states the property callers rely on — interpolating aSafeNameinto a URL path is always correct, so callers never need to escape it. This is what makesproxy_redirectsafe without changing that function.Deserializeis hand-written to go throughfrom_strvia a visitor, so the whitelist applies to every deserializer, not just explicitparsecalls. This is the load-bearing part: it is what letsaxum::extract::Path<SafeName>reject a bad name on its own, because axum deserializes path parameters — a malformed name fails extraction with400 Bad Requestbefore the handler, the mesh lookup, or the upstream proxy is ever reached.InvalidName { Empty, TooLong, NotAlphanumeric }implementsDisplayandstd::error::Error, so logs carry the reason instead of a bare failure.Display,Deref<Target = str>, andas_str()keep it usable wherever a&strwas used before.Authenticated-before-persist ordering
In
crates/sshx-server/src/web/socket.rs:get_session_wsextractsPath<SafeName>instead ofPath<String>.session.sync_now()moves from before theHellosend / auth read to after the client has been authenticated anduser_scopehas registered the user. An unauthenticated or wrongly-authenticated connection now returns early viaWsServer::InvalidAuthwithout ever marking the session for sync. As a side benefit, the snapshot that does get written now includes the user that was just allocated, rather than a bare counter increment.Integration test
test_session_name_rejects_path_traversalincrates/sshx-server/tests/simple.rsperforms real WebSocket handshakes against a freshTestServer. Real handshakes are used deliberately: a plainGETcannot distinguish the two outcomes, sinceWebSocketUpgradealso rejects a non-upgrade request with400.aB3xY9zQ1mK) must still complete its handshake.400:a%2Fb,a%2F..%2F..%2Fevil,%2E%2E%2Fadmin,%2E%2E,a%20b,a+b. These stay percent-encoded on the wire (thehttpcrate does not normalize paths) and are decoded to/,., and space by the server before validation, which is exactly the case that has to be refused.Unit tests
generated_names_are_always_safesamplesrand_alphanumericat several lengths and asserts every result parses asSafeName. This is a guard on the invariant the whole design rests on: if the generator's alphabet ever widens, the test fails instead of the validator silently becoming the thing that's wrong. The remaining tests cover acceptance, the list of rejected metacharacters, and the exact length boundary.Compatibility
The only newly rejected inputs are ones that could never have been valid session names, so no legitimate client is affected. The observable difference for a malformed name is
400at extraction instead of an upgrade followed by a4404close frame.