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

Paramiko does not respect CanonicalizeHostname and related options #897

Closed
mleinart opened this issue Feb 16, 2017 · 12 comments
Closed

Paramiko does not respect CanonicalizeHostname and related options #897

mleinart opened this issue Feb 16, 2017 · 12 comments

Comments

@mleinart
Copy link

Openssh 6.5 introduced the ability for clients to canonicalize ambiguous hostnames for matching in known_hosts and Host config blocks https://www.openssh.com/txt/release-6.5:

ssh(1): Add support for client-side hostname canonicalisation
using a set of DNS suffixes and rules in ssh_config(5). This
allows unqualified names to be canonicalised to fully-qualified
domain names to eliminate ambiguity when looking up keys in
known_hosts or checking host certificate names.

And related from https://www.openssh.com/txt/release-6.6:

ssh(1): if hostname canonicalisation is enabled and results in the
destination hostname being changed, then re-parse ssh_config(5) files
using the new destination hostname. This gives 'Host' and 'Match'
directives that use the expanded hostname a chance to be applied.

Paramiko currently ignores this setting and uses the hostname as-is in all cases

@mleinart
Copy link
Author

This is somewhat related to #896 as the 'canonical' and 'originalhost' Matches are very useful in combination with CanonicalizeHostnames

@bitprophet
Copy link
Member

Noting that #896 duped #717 which is the master ticket for implementing Match.

Agree that I'd like to see us support the canonicalization features.

@bitprophet
Copy link
Member

bitprophet commented Aug 27, 2019

Digging into this now, aiming to implement everything but CanonicalizePermittedCNAMEs as it's both a bunch of extra work & doesn't seem nearly as integral to the base use case as the others.


I've never used this feature myself so it's amusing/frustrating trying to wrap my head around it, especially in tandem with the "options only get written once" behavior. For example, the below seems like it should be a standard-enough use of the feature where I want to refer to short names but have settings in the file aimed at their matching FQDNs:

CanonicalDomains paramiko.org
CanonicalizeHostname yes

Host www.paramiko.org
    User shuttleworth

Host *
    User ewing

My assumption was that ssh random gets user ewing, but ssh www would:

  • parse the file once
  • notice the canonicalization settings & resolve www to www.paramiko.org, setting HostName to that & triggering a second parse
  • load the www.paramiko.org Host stanza
  • set user to shuttleworth

However, only the HostName update occurs; User is only ever set to ewing (going both by ssh -F configfile -G <target> and regular sshing). This threw me for a loop until I read the source and realized it's simply the "I will never overwrite an options value I've already looked up" behavior at work. The canonicalization re-parsing does not invalidate this, as I had sort of assumed.

But this seems counterintuitive - is one simply disincentivized to ever use Host * or similarly broad globbing, when also using canonicalization? I'm not the sharpest tool in the shed but I've also worked with SSH configs for ages and if I'm getting confused, I have to believe the average user is too.


In the end of course, it's moot, I need to implement the OpenSSH behavior regardless - but I want to make sure I understand the actual use of the feature, or I'm likely to miss corner cases in my tests.

@bitprophet
Copy link
Member

The above definitely isn't biting just me, see eg this RedHat ticket. But again, it's moot, so, at least I confirmed I am not simply dumb/crazy.

@bitprophet
Copy link
Member

Some related refactorings (prior support for %h involves DNS related shizzle that makes a lot of sense to reuse, such as honoring the use of AddressFamily), a bunch of stub tests, and some actual implementation & filled-out tests later, and basic support is in place. What's left are the side features (like MaxDots) and a big pile of edge/corner cases that I came up with while reading the manpage. And some TODOs.

I may not get that all done today but if I don't, it'll get done late next week when I return from a (non-tech) conference.

bitprophet added a commit that referenced this issue Aug 27, 2019
- Refactor DNS lookup related junk previously only relevant to %h
- Refactor guts of lookup() so it can be done >1 time
- Changelog/tests/implementation for canonicalization itself

Closes #897
bitprophet added a commit that referenced this issue Aug 27, 2019
- Refactor DNS lookup related junk previously only relevant to %h
- Refactor guts of lookup() so it can be done >1 time
- Changelog/tests/implementation for canonicalization itself

Closes #897
@bitprophet
Copy link
Member

bitprophet commented Aug 27, 2019

Pushed a WIP commit for this. (later edit: separated branches for this and #717 because it was annoying me.)

bitprophet added a commit that referenced this issue Sep 13, 2019
- Refactor DNS lookup related junk previously only relevant to %h
- Refactor guts of lookup() so it can be done >1 time
- Changelog/tests/implementation for canonicalization itself

Closes #897
bitprophet added a commit that referenced this issue Sep 14, 2019
- Refactor DNS lookup related junk previously only relevant to %h
- Refactor guts of lookup() so it can be done >1 time
- Changelog/tests/implementation for canonicalization itself

Closes #897
bitprophet added a commit that referenced this issue Sep 26, 2019
- Refactor DNS lookup related junk previously only relevant to %h
- Refactor guts of lookup() so it can be done >1 time
- Changelog/tests/implementation for canonicalization itself

Closes #897
@bitprophet
Copy link
Member

This is done enough! What's obviously missing:

  • CanonicalizePermittedCNAMEs - lots of fiddly coding/testing for a presumably less critical edge case, deferred
  • Proxy(Command|Jump) honoring of whether CanonicalizeHostname is yes or always - that's not Paramiko's own concern at this time but would be implemented in the downstream library, such as Fabric. (Fabric is not getting support for that at this time, which basically means that yes acts like always, for now, I think...)

@bitprophet
Copy link
Member

Also, when testing how this impacts IdentityFile I found an unreported bug around duplication in that value; eg say you had two stanzas that both applied and both said IdentityFile foo - you'd end up with ["foo", "foo"]. Not super problematic - just means clients would potentially try the same key >1 time - but still dumb and can cost you authentication attempts or whatever.

This became even more obvious under canonicalization's triggering of a 2nd parse pass. It's now tested for and in the changelog as fixed.

@totaam
Copy link

totaam commented Dec 2, 2023

Hi, could you at least merge something like 1e50709 to avoid KeyError: 'canonicaldomains' for those that set CanonicalizeHostname always?

@bskinn
Copy link
Contributor

bskinn commented Dec 18, 2023

Hi, could you at least merge something like 1e50709 to avoid KeyError: 'canonicaldomains' for those that set CanonicalizeHostname always?

This seems like a separate concern, @totaam, especially given this ticket was closed as completed more than four years ago.

Is this commit you reference part of another PR? If so, please comment on that PR to mark your interest in its being merged.

If it's not part of another PR, please create a new ticket for the fix. Either an issue with this request, if you're not comfortable making the requested changes yourself; or, a PR with the implementation.

@totaam
Copy link

totaam commented Dec 19, 2023

This seems like a separate concern

This is a direct result of the code added in this issue.

please create a new ticket for the fix

#2338

@bskinn
Copy link
Contributor

bskinn commented Dec 19, 2023

This is a direct result of the code added in this issue.

Of course, my apologies, I didn't express myself well there.

It would have been better had I said, "This seems like something best addressed in a separate issue/PR ..."

Thanks for the new PR!

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

No branches or pull requests

4 participants