Adds support for SSH ProxyCommand to be able to use "Bastion" servers #236
Conversation
vo-va
force-pushed
the
add-ProxyCommand-support
branch
from
ac09b23 to
163f4e4
Compare
|
looks like some minor linter issues |
|
I've tried to use linter before pushing a new commit, but I think I do not fully understand why it was complaining about Shadows declaration inside the if block and not complaining about err that is declared for agent forwarding. Idea is reporting it, so I am not sure it was fully fixed. Looks like name return of sshClient() is affecting it. |
|
I can't even allow this pipeline to run anymore. GH supposed to request permission to activate the pipeline, but it shows nothing. Anyway, don't worry about it. I'll review the code today (hopefully) regardless of those linter quirks. |
There was a problem hiding this comment.
Thanks for the PR. I have indicated a few issues I have with this PR. Another thing I would like to see is more tests to ensure things work as expected, end-to-end. At least a main-level test (main_test.go), but generally as much testing as needed to be absolutely certain it works and doesn't break anything existing.
| Port int `yaml:"port" toml:"port"` | ||
| User string `yaml:"user" toml:"user"` | ||
| Tags []string `yaml:"tags" toml:"tags"` | ||
| ProxyCommand []string `yaml:"proxy_command" toml:"proxy_command"` |
There was a problem hiding this comment.
I'm not sure if having []string here is clear and user-friendly. I would expect a single string with all the arguments.
| @@ -22,6 +23,31 @@ type Connector struct { | |||
| logs Logs | |||
| } | |||
|
|
|||
| // SubstituteProxyCommand updates variables with values associated with the target host. | |||
| // SSH ProxyCommand can use placeholders such as %h, %p, and %r (%r - username), they have to be replaced with the actual values. | |||
| func SubstituteProxyCommand(username, address string, proxyCommand []string) ([]string, error) { | |||
There was a problem hiding this comment.
Is this one supposed to be public? Unless I missed something, I don't see why the end user would hit it directly.
| func (c *Connector) Connect(ctx context.Context, hostAddr, hostName, user string) (*Remote, error) { | ||
| log.Printf("[DEBUG] connect to %q (%s), user %q", hostAddr, hostName, user) | ||
| client, err := c.sshClient(ctx, hostAddr, user) | ||
| func (c *Connector) Connect(ctx context.Context, hostAddr, hostName, user string, proxyCommand []string) (*Remote, error) { |
There was a problem hiding this comment.
I don't like changing the connect method to add an optional argument. I would rather expect another WithProxy method, similar to WithAgent.
There was a problem hiding this comment.
To be sure that I am not making changes in the wrong direction, I want to ask next. If the optional parameter ProxyCommand will be configured with a method similar to .WithAgent(), the code that establishes a connection through ProxyCommand will be a separate method, but at some place I will use if … else or just if … return, if … return to choose between a normal network connection and a proxy connection, will it be acceptable?
| log.Printf("[DEBUG] connect to %q (%s), user %q", hostAddr, hostName, user) | ||
| client, err := c.sshClient(ctx, hostAddr, user) | ||
| func (c *Connector) Connect(ctx context.Context, hostAddr, hostName, user string, proxyCommand []string) (*Remote, error) { | ||
| log.Printf("[DEBUG] connect to %q (%s), user %q, proxy command: %s", hostAddr, hostName, user, proxyCommand) |
There was a problem hiding this comment.
As proxy is very optional, I won't add it to the default debug message(s). Probably anything regarding the proxy should be shown in debug only if this proxy is active.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create client connection to %s: %v", host, err) | ||
|
|
||
| if len(proxyCommand) == 0 { |
There was a problem hiding this comment.
Looks complicated to me; it probably deserves to be extracted to a separate function.
There was a problem hiding this comment.
UPD: nevermind, checking WithAgentForwarding()
May I ask about this place https://github.com/umputun/spot/blob/master/pkg/runner/runner.go#L187 I guess there will be a 2nd function Connect.ConnectWithProxy(...), but what approach should I use to choose between normal connection and proxy? Will if ... else be good enough here?
|
Just in case, I am working on addressing review comments. It is not abandoned. |
Adds support for this https://man.openbsd.org/ssh_config.5#ProxyCommand
Discussion #231
Documentation not updated yet. I will update it if implementation is ok and will not require changes. Please check comments in connector_test.go.
I was running tests on a local computer, some of which related to vaults were failing for me. If they fail on GitHub, I will figure this out later.