Skip to content

feat: Set SSH env vars: SSH_CLIENT, SSH_CONNECTION and SSH_TTY #3622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Aug 23, 2022
Merged

feat: Set SSH env vars: SSH_CLIENT, SSH_CONNECTION and SSH_TTY #3622

merged 7 commits into from
Aug 23, 2022

Conversation

mafredri
Copy link
Member

With out current webrtc connections, the result is not ideal:

# env | grep ^SSH_
SSH_CONNECTION=peer/unknown-addr 0 peer/unknown-addr 0
SSH_PTY=/dev/pts/0
SSH_CLIENT=peer/unknown-addr 0 0

It seems like it would be possible to extract some sensible information from the peer negotiation process.

selected candidate pair changed	{"local": {"foundation": "", "priority": 0, "address": "172.17.0.2", "protocol": 1, "port": 33555, "type": 1, "component": 1, "relatedAddress": "", "relatedPort": 0, "tcpType": ""}, "remote": {"foundation": "", "priority": 0, "address": "192.168.32.120", "protocol": 1, "port": 56627, "type": 1, "component": 1, "relatedAddress": "", "relatedPort": 0, "tcpType": ""}}

But since we're moving towards WireGuard, I don't think it's worth it.

Using WireGuard though (coder config-ssh --wireguard), we actually get some more sensible output:

env | grep ^SSH_
SSH_CONNECTION=[66b0:b5ee:5a5f:62ec:3f95:e1ff:526c:38e4] 23171 [6a58:41de:9543:ec22:2692:f500:4768:861b] 12212
SSH_PTY=/dev/pts/0
SSH_CLIENT=[66b0:b5ee:5a5f:62ec:3f95:e1ff:526c:38e4] 23171 12212

Fixes #2339

We may want to re-consider the pty package we currently have as well, it's a bit weird we set SSH_TTY in it now, without being more explicit about it being tied to SSH. Relevant for #3473 as well.

@mafredri mafredri self-assigned this Aug 22, 2022
@mafredri mafredri requested a review from a team August 22, 2022 15:38
deansheather
deansheather reviewed Aug 22, 2022
View reviewed changes
Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just hardcode these values instead? They won't be useful to any programs that read them as the IP address is not an actual routable IP address (as the networking is happening in process and not through the host).

If we're just trying to expose these variables so software can detect if it's running in an SSH connection then hardcoding them seems easier and not prone to random format changes in the future

@mafredri
Copy link
Member Author

mafredri commented Aug 22, 2022 •
edited
Loading

Can we just hardcode these values instead? They won't be useful to any programs that read them as the IP address is not an actual routable IP address (as the networking is happening in process and not through the host).

If we're just trying to expose these variables so software can detect if it's running in an SSH connection then hardcoding them seems easier and not prone to random format changes in the future

I was thinking that might not necessarily be the case with WireGuard/Tailscale.

I guess we could hardcode them, but what would we hardcode them to?

@spikecurtis
Copy link
Contributor

Hardcode to 0.0.0.0? It's a reserved address for "this host" and is not a valid destination, so no one will try to dial it (or will fail if they do)

@mafredri
Copy link
Member Author

mafredri commented Aug 22, 2022 •
edited
Loading

Typically it'd be:

SSH_CONNECTION=[client_ip] [client_port] [server_ip] 22

@spikecurtis Are you suggesting 0.0.0.0 for both client and server (semantically it'd only be correct for the server)? So essentially:

SSH_CONNECTION=0.0.0.0 0 0.0.0.0 0

A bit weird, but I don't have strong feelings on what the value should be (unless we can make it right in the wg-case, then I think we should do it).

@spikecurtis
Copy link
Contributor

I don't think we can make it "right" in the Wireguard case because unlike a typical Wireguard VPN, our use of it is implemented entire in-process (agent), so anything reading the IP addresses would be unable to route to them.

@mafredri
Copy link
Member Author

I don't think we can make it "right" in the Wireguard case because unlike a typical Wireguard VPN, our use of it is implemented entire in-process (agent), so anything reading the IP addresses would be unable to route to them.

That makes sense, yeah. I do think there's a chance for this to change, depending on what we want to do with wg/tailscale. For instance, direct workspace->workspace communication in a tailnet would be pretty cool.

I'll go with the hard-coded values for now, though. 👍🏻

@mafredri mafredri force-pushed the mafredri/ssh-environment-variables branch from 1f5afe3 to 24f8e6a Compare August 23, 2022 11:16
@mafredri mafredri force-pushed the mafredri/ssh-environment-variables branch from 24f8e6a to f7c91cc Compare August 23, 2022 11:49
@mafredri mafredri requested a review from deansheather August 23, 2022 16:46
@mafredri mafredri merged commit e44f7ad into main Aug 23, 2022
@mafredri mafredri deleted the mafredri/ssh-environment-variables branch August 23, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deansheather deansheather deansheather approved these changes

Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set standard SSH environment variables in the workspace
3 participants
@mafredri @spikecurtis @deansheather