Fix session ID env var handling for joining sessions#59294
Conversation
|
Really happy to see this getting cleaned up. Thank you @Joerger! |
lib/srv/termhandlers.go
Outdated
| // TODO(Joerger): DELETE IN v19.0.0 - old clients set TELEPORT_SESSION for new | ||
| // sessions, but the client should only provide a session ID for join sessions. | ||
| // Therefore we ignore the client's provided session ID if there is no session | ||
| // to join and overwrite the TELEPORT_SESSION env var for the session. |
There was a problem hiding this comment.
@capnspacehook I gave this some thought and tested with a v17 client connecting to a server with this PR based off v18, and it seemed to work fine. I don't think the client provided session ID needs to be respected now that the Node will always set it itself (if a join session isn't found). WDYT?
There was a problem hiding this comment.
Isn't this the current behavior, just moved?
We might want to actually respect the DELETE IN and return the error in master and add the escape hatch for IsNotFound in the backport tho, or maybe bump the DELETE IN to v20 just to be extra accomodating to older clients.
There was a problem hiding this comment.
Isn't this the current behavior, just moved?
Ah, you're right. But yes it looks like v18.0.X clients still set the session ID for new sessions, so we should maintain this fallback until v20 at least.
| // TODO(Joerger): Rather than relying on the out-of-band env var params, we should | ||
| // provide session params upfront as extra data in the session channel request. |
There was a problem hiding this comment.
I don't know if we can do this without incurring an additional roundtrip of latency when talking to outdated servers (because we are going to have to use a separate channel type and fall back to the real ssh one on failure). I wish x/crypto/ssh had a way to set and read negotiated extensions at handshake time. 😔
There was a problem hiding this comment.
It can be provided as extra data in the "session" channel request - I have this in a draft PR
There was a problem hiding this comment.
Is that going to be gracefully ignored by openssh sshd and other ssh server implementations?
There was a problem hiding this comment.
Yes, I tested that this works with and Agentless node. The session params would only be needed or consumed when starting (or joining) a Teleport session with a Teleport client.
There was a problem hiding this comment.
I still don't fully buy it, I'd be ok with it if the client only sent that if the server version string included Teleport, we shouldn't violate the SSH spec otherwise - because other server implementations that are also going to be noncompliant might interpret that "free" extra data field in a different way.
There was a problem hiding this comment.
I don't think we actually have to worry about the client connecting to a non-Teleport server. Isn't tsh ssh only ever connecting to either 1) a Teleport Node or 2) a Proxy Forwarding server?
I've added the version check to that PR for now anyways.
lib/srv/termhandlers.go
Outdated
| // TODO(Joerger): DELETE IN v19.0.0 - old clients set TELEPORT_SESSION for new | ||
| // sessions, but the client should only provide a session ID for join sessions. | ||
| // Therefore we ignore the client's provided session ID if there is no session | ||
| // to join and overwrite the TELEPORT_SESSION env var for the session. |
There was a problem hiding this comment.
Isn't this the current behavior, just moved?
We might want to actually respect the DELETE IN and return the error in master and add the escape hatch for IsNotFound in the backport tho, or maybe bump the DELETE IN to v20 just to be extra accomodating to older clients.
Joerger
force-pushed
the
joerger/fix-connection-session-id
branch
from
e20bf12 to
71928b7
Compare
* Refactor server session ID logic; Separate join and open session flows. * Remove unused client node session ID. * Fix backwards compatibility. * Fix tests and lint. * Update deprecation TODO. * Fix failing test caused by delayed association with join session. * fix session ID for git events --------- Co-authored-by: STeve (Xin) Huang <huangin52@gmail.com>
* Refactor server session ID logic; Separate join and open session flows. * Remove unused client node session ID. * Fix backwards compatibility. * Fix tests and lint. * Update deprecation TODO. * Fix failing test caused by delayed association with join session. * fix session ID for git events --------- Co-authored-by: STeve (Xin) Huang <huangin52@gmail.com>
5 participants
Changelog: Fix a bug where session IDs were tied to the client connection, resulting in issues when combined with multiplexed connection features (OpenSSH ControlPath/ControlMaster/ControlPersist).
Currently, the way the server manages the session ID is a bit of a mess. The session ID may be set for join sessions via env var reqs any time between the "session" channel request and the "shell" / "exec" channel request, leaving the server unable to definitively provide an accurate session ID.
This PR simplifies this logic by simply setting / retrieving the session ID when it is actually available, during the "shell" / "exec" channel request. Premature calls to
ServerContext.SessionIDwill simply return an empty string.#59206 goes beyond this fix and actually ensures the session ID is available during the "session" channel request, but it will not be backported.
Closes #59066