-
Notifications
You must be signed in to change notification settings - Fork 460
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
expanded ssh_config parameters for qemu+ssh uri option #1059
Merged
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
ba090fc
bump ssh_config to stable version which contains GetAll call
19893fc
refactor dialSSH and break out dialHost as support function
94ae671
move authentication parsing to per host part of loop
76ed8d0
implement per Host identity file lookup
7dacf0b
fix tilde (~) based home directory notation for convenience
7273b7d
updated go.sum
4abb01a
cleanup log outputs
f190d80
remove unnecessary local variable
329a202
make use of net package URI building to support correct ipv6
c1e4fbf
correctly use host:port format when dialing bastion host
100f82b
put quotes around target in case it is empty
e203600
if the hostname override isn't present, simply use target name
d47102d
add log output for port override
1516454
add default host key algorithm
7caaf18
move port configuration earlier so that hostkey callback works right
ad51d68
cleanup log output, add error handling for dial host
6754108
add support for sshconfig based known hosts file behaviour
60cf3f1
integrate HostKeyAlgorithms ssh_config option
060987d
move dial host impl so that bastion hosts have same features
31af282
add comments
3680059
use a more modern default host key
615b189
update auth method parse to allow for multiple private ssh keys
dec8f23
use a list of hostKeyAlgorithms instead of just one default
86ac64d
Merge branch 'dmacvicar:main' into ssh-config-parameters
memetb 4c8eb53
use camelCase to match go coding styles
0da4763
use join instead of replace for a more predictable outcome
186e5ac
remove log.Fatal and let the upper layer deal with the logging
6f1618f
change magic number to constant
fe60b2c
code formatting to match go coding style
2c1c934
add missing import for filepath module (0da4763a)
d1aa5a8
Merge branch 'main' into ssh-config-parameters
dmacvicar 2e39b4e
lint fixes
dmacvicar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
anderr
are nil?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.
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 forproxy != ""
?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.
ProxyJump
can be absent, in which caseGet()
(as opposed toGetStrict()
)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 iserr == nil
andproxy == ""
. 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 aboutproxy
. 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.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. ifssh_config
gets touched during this setup).Thoughts?
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.
@dmacvicar see above for my thoughts on this codepath.