-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
feat: Golang ssh #25
feat: Golang ssh #25
Conversation
* 'main' of https://github.com/mikew/nvrh: feat: Use loopback (#26)
return replaced | ||
} | ||
|
||
replaced = strings.ReplaceAll(replaced, "$HOME", userHomeDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add support for %HOME%
? I'm not sure what ssh actually supports here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh might need more:
IdentityFile
The IdentityFile parameter contains the path to the identity file with a user's RSA or DSA identity. The default path for protocol version 1 is ~/.ssh/identity, whereas protocol version 2 uses either ~/.ssh/id_rsa or~/.ssh/id_dsa.
The identity file path allows using the tilde (~) for a user's home directory, and the following substitutions:
%d is the local user's home directory.
%u is the local user's username.
%l is the local hostname.
%h is the remote hostname.
%r is the remote username.
} | ||
|
||
func (nc *NvrhContext) LocalSocketOrPort() string { | ||
if nc.ShouldUsePorts { | ||
// nvim-qt, at least on Windows (and might have something to do with | ||
// running in a VM) seems to prefer `127.0.0.1` to `0.0.0.0`, and I think | ||
// that's safe on other OSes. | ||
return fmt.Sprintf("127.0.0.1:%d", nc.PortNumber) | ||
return fmt.Sprintf("localhost:%d", nc.PortNumber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Windows environment isn't accessible at the moment, and I'd like to test this on there before merging this in.
if agentSigners, _ := getSignersForIdentityAgent(endpoint.GivenHost); agentSigners != nil { | ||
allSigners = append(allSigners, agentSigners...) | ||
} | ||
|
||
if identitySigner, _ := getSignerForIdentityFile(endpoint.GivenHost); identitySigner != nil { | ||
allSigners = append(allSigners, identitySigner) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should leave a comment why both pubkey and agent are in the one callback
return nil | ||
} | ||
|
||
func (c *NvrhInternalSshClient) TunnelSocket(tunnelInfo *ssh_tunnel_info.SshTunnelInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check some other implementations of this to see if I'm missing anything
if e.SshConfigHost != "" { | ||
return e.SshConfigHost | ||
} | ||
|
||
return e.GivenHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should comment why this one's a bit different than the rest when giving preference to config / given
Alright, I've let this cook for a month. I've been using it and it seems to work everywhere (and more importantly, doesn't have any regressions when not using the internal SSH path). I'm not psyched on how much code it is, and don't know how willing I am to maintain it, but it's got some other refactors that I think are worth it, and it shouldn't be to hard to strip out the internal SSH path if maintenance becomes a burden. |
I've kept the old SSH functionality, henceforth known as "binary SSH", in nvrh. In fact it's still the default
--ssh-path binary
will use the included OpenSSH on Windows, andssh
anywhere else--ssh-path internal
will use golang's SSH stuff added in this PR--ssh-path /some/path/to/ssh
will use binary SSH, calling out to/some/path/to/ssh
These PR has changes that will be more annoying for people that use password auth + binary ssh mode. nvrh now runs more commands after you end the session for cleanup. I really recommend:
Mostly putting this up to get it off my mind. It's nowhere near feature complete, and the amount of things we have to worry about is staggering (and really makes me appreciate just shelling out to things):And that's just to build a config to even begin connecting via SSH. On top of that we've got to pass a client around, and do port / socket tunnelling.Closes #24