Add support for retrieving all IdentityFile directives #22
Conversation
|
Travis failed, but the reason it failed was because the analysis tools no longer support the version of go that you're testing on. The recent versions of go passed. |
|
I just merged a change to master to fix the Travis branch |
|
So I didn't anticipate or read the spec closely enough that multiple values could be okay. I don't think that's something that's specific to the IdentityFile directive though, it looks like CertificateFile also allows multiple values, and I stopped looking after that. I think we need a second method
You would then do |
|
You're right, I didn't look carefully enough. It seems like the following directives support multiple:
|
|
Please ping when this is ready for re-review!
…--
Kevin Burke
925.271.7005 | kev.inburke.com
On Wed, Jul 11, 2018 at 10:41 AM, Dustin Spicuzza ***@***.***> wrote:
You're right, I didn't look carefully enough. It seems like the following
directives support multiple:
- CertificateFile
- DynamicForward
- RemoteForward
- SendEnv
GetAll seems reasonable.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOSI8WnUIbKVGfN-5qYWwOcuXKk4IuSks5uFjjOgaJpZM4VKer9>
.
|
|
Updated, tests are good. |
|
One sec, needs a Strict variant. But it'll be mostly the same. |
virtuald
force-pushed
the
master
branch
3 times, most recently
from
7c6828f to
6a12ff4
Compare
|
Ok, should be good for review. |
|
Still ready for review. 👍 |
|
Lost track of this, addressed most concerns. |
|
Ping. |
| // Some multi-valued settings have different defaults based on other settings, so | ||
| // you must provide the host alias and a function to retrieve a setting | ||
| func DefaultAll(keyword string, alias string, get func(alias, key string) string) []string { | ||
| if strings.ToLower(keyword) == "identityfile" && get(alias, "Protocol") == "2" { |
There was a problem hiding this comment.
I'm confused by this function, by this comment:
// Some multi-valued settings have different defaults based on other settings, so
// you must provide the host alias and a function to retrieve a setting
and by the fact that this logic appears but not in Default. I think maybe if users want this logic they can implement it themselves, alternatively let's pull this function out into a different PR and keep this one focused on GetAll / GetAllStrict.
There was a problem hiding this comment.
Originally I had put this logic in GetAll (see ee013b8). However, I needed to wrap the config, so I need a Default and a DefaultAll. It seems convenient to make it available to people who wanted to do such a thing.
The comment sucks. But if you look at the manpage for ssh_config (OSX 10.12), you can see that the default value for identityfile differs based on which protocol the client has specified. Though, on Linux it seems that this isn't the case. If you want to remove support for legacy stuff, then I can change this to just default to protocol 2.
| "SetEnv": true, | ||
| } | ||
|
|
||
| // IsMultifileDirective indicates a directive that can be found |
There was a problem hiding this comment.
Is this the right name for it? SetEnv doesn't really have to do with files I don't think but returns true.
If we leave this un-exported for now we can mess with the naming later.
There was a problem hiding this comment.
From OSX 10.11 for SendEnv:
Variables are specified by name, which may contain wildcard characters. Multiple environment variables may be separated by whitespace or spread across multiple SendEnv directives. The default is not to send any environment variables.
I don't know where SetEnv came from, I can remove it.
There was a problem hiding this comment.
Actually, here's where it came from. You can see from the config parsing source code that it will keep adding additional directives as it encounters them.
There was a problem hiding this comment.
Multifile in this context means "can the directive be accumulated across multiple ssh configuration files". I'm happy to rename it if you have a better name.
|
I was also trying to retrieve Also happy to help out myself if that's desirable, though I'd need to ramp up on the project's code base first. @virtuald @kevinburke curious to hear where we stand on this one, thanks! |
|
Hey, apologies, I didn't realize this was blocked on me. I will add it to my todo list. |
|
Rebased against master again. |
|
I gave the PR a quick try to retrieve top-level IdentityFiles from my ssh config: it worked like a charm for me. ❤️ Looking forward to seeing this one getting merged. |
|
@kevinburke I've been using this PR for years, works fine. Just merge it, don't let perfect be the enemy of good? :) |
|
I'm getting ready to use this library too. This sounds like something I'd appreciate merged. 👍 |
|
I will try to take a look this week, not going to promise anything (I don't get paid to maintain this library) |
|
Closing in favor of #28. Also, my repo will be a retargeted fork of this repo with this change so that users don't have to use a replace directive in their module. |
4 participants
Additionally, this returns the correct defaults for SSH protocol 2
Unfortunately, since this is the only directive that can have multiple values, there's quite a bit of code duplication. However, it seems to work!