expanded ssh_config parameters for qemu+ssh uri option #1059
Conversation
this allows different hosts (jump hosts) to have different identity files specified
as per commit from MaxMatti: dmacvicar@1152bdd
the hostKeyCallback makes use of the SSH port and fails if a custom ssh port is being used by the host
this value was chosen as the lowest RSA available by default on a debian build running OpenSSH_9.2 and works out of the box for most hosts tested by authority. Any older systems can specifically set their key algorithms in .ssh/config
memetb
changed the title
memetb
force-pushed
the
ssh-config-parameters
branch
from
082f086
to
dec8f23
Compare
| bastion, err = u.dialHost(proxy, sshcfg, depth + 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to bastion host '%v': %w", proxy, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
If the err != nil, it is silently ignored instead of returning. My suggestion: if err != nil, return, then only condition on proxy being non empty to execute the recursion.
There was a problem hiding this comment.
Unclear, can you rephrase please? Did you mean "if the error == nil" ?
Are you perhaps saying there can be a condition in which both bastion and err are nil?
There was a problem hiding this comment.
What I mean is that proxy, err = sshcfg.Get(target, "ProxyJump") can return error, and in that case you would continue with the flow, instead of returning the function. Yes you would skip dialing the proxy (assuming it returned empty string in addition to the error), but the flow would continue anyway.
Wouldn't it make more sense to return early if proxy, err = sshcfg.Get(target, "ProxyJump") returns an error. And then you execute the code that dials the proxy only testing for proxy != "" ?
There was a problem hiding this comment.
ProxyJump can be absent, in which case Get() (as opposed to GetStrict())returns empty string as per sshconfig docs. This isn't an error per-se as it simply means there's no proxy. This covers case 1 of the conditional on line 250, which is err == nil and proxy == "". In this case, the flow should proceed without proxy.
The second case is if the sshconfig file had a parse error (i.e. err != nil) and we don't care about proxy. In this case, it means that the sshconfig file couldn't be parsed. Expected behaviour in those circumstances (e.g. when using ssh from cli) is to simply honor the command itself and ignore any ssh_config directives altogether, which in this case corresponds to a flow-through without Proxy.
Wouldn't it make more sense to return early if proxy, err = sshcfg.Get(target, "ProxyJump") returns an error. And then you execute the code that dials the proxy only testing for proxy != "" ?
I think there's a case to be made that (maybe) the very first successful call to sshcfg.Get() guarantees that every subsequent call is successful. However, I'd not feel comfortable breaking the abstraction and depending on this behaviour: it would make the code very convoluted and introduce race conditions (i.e. if ssh_config gets touched during this setup).
Thoughts?
There was a problem hiding this comment.
@dmacvicar see above for my thoughts on this codepath.
replace could have resulted in weird behaviour such as "some~path" becoming incorrectly mangled
|
@dmacvicar made the changes you requested, 2 outstanding question. Let me know when you get a chance. Thanks. |
| bastion, err = u.dialHost(proxy, sshcfg, depth + 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to bastion host '%v': %w", proxy, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
What I mean is that proxy, err = sshcfg.Get(target, "ProxyJump") can return error, and in that case you would continue with the flow, instead of returning the function. Yes you would skip dialing the proxy (assuming it returned empty string in addition to the error), but the flow would continue anyway.
Wouldn't it make more sense to return early if proxy, err = sshcfg.Get(target, "ProxyJump") returns an error. And then you execute the code that dials the proxy only testing for proxy != "" ?
|
Thanks for your contribution @memetb , I will try to wrap it up in a release soon. |
dmacvicar
added
the
Important (Wanted)
|
You're welcome. Thanks for the very useful library. |
|
Do you see anything worth documenting here? |
* bump ssh_config to stable version which contains GetAll call * refactor dialSSH and break out dialHost as support function * move authentication parsing to per host part of loop this allows different hosts (jump hosts) to have different identity files specified * implement per Host identity file lookup * fix tilde (~) based home directory notation for convenience * updated go.sum * cleanup log outputs * remove unnecessary local variable * make use of net package URI building to support correct ipv6 as per commit from MaxMatti: dmacvicar@1152bdd * correctly use host:port format when dialing bastion host * put quotes around target in case it is empty * if the hostname override isn't present, simply use target name * add log output for port override * add default host key algorithm * move port configuration earlier so that hostkey callback works right the hostKeyCallback makes use of the SSH port and fails if a custom ssh port is being used by the host * cleanup log output, add error handling for dial host * add support for sshconfig based known hosts file behaviour * integrate HostKeyAlgorithms ssh_config option * move dial host impl so that bastion hosts have same features * add comments * use a more modern default host key this value was chosen as the lowest RSA available by default on a debian build running OpenSSH_9.2 and works out of the box for most hosts tested by authority. Any older systems can specifically set their key algorithms in .ssh/config * update auth method parse to allow for multiple private ssh keys * use a list of hostKeyAlgorithms instead of just one default * use camelCase to match go coding styles * use join instead of replace for a more predictable outcome replace could have resulted in weird behaviour such as "some~path" becoming incorrectly mangled * remove log.Fatal and let the upper layer deal with the logging * change magic number to constant * code formatting to match go coding style * add missing import for filepath module (0da4763) * lint fixes
|
Unfortunately it ignores the settings in ~/.ssh/config that concern the Port directive. For the bastion node and for the host a custom port is specified, but the connection still goes to port 22. Error: If I don't use bastion and specify port in I get an error: |
|
@mhtr please open a new issue, we can look at it over there. @dmacvicar I'm thinking of your previous comment about the dispersed nature of the ssh config object. The original motivation of my PR was to enable bastion hosts as they are critical in modern infra, however I aimed to change nothing in the structure of the code and as a result, most of the code simply got moved (not rewritten) for the purpose of disentangling dialSSH and dialHost in 19893fc. What I'm seeing is that ConnectionURI as an object is probably where this code should lie. Incidentally, this current bug is here in the call to Further, the unit test for connectionURI seems to me the ideal location to put a battery of unit tests as it relates to ssh_config. I can fix the u.Port() for now, but if I'm going to do a refactor of connection_uri, I will need some joint review to make sure it's a sensible path forward (but given the note on the project that refactors aren't particularly welcome, I'm not sure we want to take this on). In particular, I think the unit tests should be substantially beefed up, and my personal preference would be to come to a consensus on the unit test itself first: it should be comprehensive in that it should test garbage files, missing files, edge conditions with mixed sources of data... |
Hello @dmacvicar,
this PR is in relation to the issue #1058 I recently opened.
This is a preliminary PR and work in progress. I have listed the known issues with this PR at this point. I would also like to get feedback from you before putting more effort in.This feature is a must-have for my use case since I require to access my bare metal servers through a bastion host.
Known issues:
3. little to no effort on code style and documentation: this was a quick and dirty feature feasibility test to see if the desired workflow worked correctly