Skip to content
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

Add support for retrieving all IdentityFile directives #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

virtuald
Copy link
Contributor

Moving #22 to be from a different branch.

Additionally, I'm going to publish a re-targeted fork of this repo until you merge this, because it's annoying to add replace directives to my modules.

@kevinburke
Copy link
Owner

I finally have time to look at this today. I'm sorry for the delay. I would like to try to merge something by the end of the day.

  • Can we think of a better name than IsMultifileDirective ? Maybe SupportsMultiple?
  • The API for DefaultAll feels a little too convoluted. Could we either special case DefaultIdentityFiles(), or only return the results you'd get for Protocol 2, or some other change?

How are you currently using DefaultAll? I don't see any examples for it currently.

@kevinburke
Copy link
Owner

OK, I merged 1241662, which doesn't have DefaultAll and renames IsMultifileDirective to SupportsMultiple. Still open to ideas/merging future changes on both of those.

@virtuald
Copy link
Contributor Author

I've only been using DefaultAll only for IdentityFile, so making it a special case is fine for my usage. The reason that DefaultAll has the weird getter function is because the default value changes based on the config that you're currently parsing -- though I can't imagine anyone cares much about SSH protocol 1 anymore?

There's a wrapper struct of course...

func (s *Settings) GetAllStrict(alias, key string) ([]string, error) {
	val, err := s.cfg.GetAll(alias, key)
	if val != nil || err != nil {
		return val, err
	}
	return ssh_config.DefaultAll(key, alias, func(a, k string) string {
		v, _ := s.cfg.Get(a, k)
		return v
	}), nil
}

And here's how I call that:

	files, err := s.GetAllStrict(hostAlias, "IdentityFile")

@kevinburke
Copy link
Owner

If no one cares about ssh protocol 1 then let's just ignore it or return the default for ssh protocol 2

@virtuald
Copy link
Contributor Author

virtuald commented Mar 27, 2021

An alternative would be accepting a Config object as the third argument to DefaultAll instead of a function? Then it could remain correct even in the presence of SSH protocol 1 for some reason.

@virtuald
Copy link
Contributor Author

Rebased and made the change to use a config object. Unfortunately, UserSettings and Config don't have the same function signatures, so I had to add an internal method to make them both compatible -- which is likely why I chose a function originally.

@blampe
Copy link

blampe commented Jul 26, 2023

@kevinburke are you open to merging this or something similar that accomplishes the same?

I came across this while debugging some very surprising behavior where connections kept trying to use ~/.ssh/identity instead of the usual suspects (id_rsa, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants