'none' ProxyCommand values failing to mask non-'none' ones in less-specific blocks #670

Closed
pbrisbin opened this Issue Feb 2, 2016 · 8 comments

Projects

None yet

2 participants

@pbrisbin
pbrisbin commented Feb 2, 2016

I'm running into something similar to #261:

Given an SSH config like this:

Host gateway
    Port                   1234
    HostName               gateway.example.com
    ProxyCommand           none
    BatchMode              yes
    PasswordAuthentication no
    ForwardAgent           yes

Host *
    User                   admin
    ServerAliveInterval    60
    TCPKeepAlive           yes
    ProxyCommand           ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -q -p 1234 admin@gateway.example.com nc %h 22
    StrictHostKeyChecking  no
    UserKnownHostsFile     /dev/null

A command like fab -Hgateway ... won't connect (error reading SSH banner). The analogous ssh gateway ... works fine.

I've deduced that paramiko is using the wrong ProxyCommand -- it's using the one from Host * and not Host gateway. If I comment the ProxyCommand from the Host * block, the fabric command works.

Users with similar complaints are directed towards the documentation:

For each parameter, the first obtained value will be used. The configuration files contain sections separated by ``Host'' specifications, and that section is only applied for hosts that match one of the patterns given in the specification.The matched host name is the one given on the command line.

Since the first obtained value for each parameter is used, more host-specific declarations should be given near the beginning of the file, and general defaults at the end.

But I think we're doing things right. The Host gateway definition is earlier, and it sets ProxyCommand none first. I'd expect that would be respected, as it is with normal ssh commands.

Version info:

% pip list | grep paramiko
paramiko (1.16.0)
@bitprophet
Member
bitprophet commented Apr 24, 2016 edited

Users with similar complaints are directed towards the documentation

To be pedantic: the usual issue is users organizing their conf files with Host * followed by Host specific, and not being cognizant of the "first match wins" behavior. What you're experiencing looks like the opposite problem :)


I wasted a bunch of time proving that things are already "correct", with one annoying difference: I had a non-none value in my specific host block. So that worked just fine and I almost sent you on your way, until I saw...these lines: https://github.com/paramiko/paramiko/blob/04369de37e18167deccff70e2c77381d5d84ad23/paramiko/config.py#L78-L80

Welp.

Sure enough, #415 and the actual PR #433 simply excise/skip ProxyCommand none. Which is fine...until we hit your scenario where not having that none value allows later blocks to "win".


Offhand solutions that would avoid this problem & be more correct:

  • Set the value to Python None instead of the string "none".
    • Requires client code to be aware of this possibility instead of naively going "is there a ProxyCommand? RUN IT", which I'm sure at least some of it does now (Fabric possibly included - though we can of course change it there.)
  • Set the value to some sentinel (None or otherwise), then strip out the key/value pairs containing it at the end of parsing.
    • This is more in line with the spirit of #433 and prevents clients from having to worry about changed behavior.
    • But it feels slightly less clean to me, and continues to "misrepresent" the original source config file (given the author of the config file explicitly put a directive in which is no longer present in the parsed version).

Opinions accepted!

@bitprophet bitprophet added this to the 1.16.2 milestone Apr 24, 2016
@pbrisbin

Opinions accepted!

I think there are two orthogonal issues to think through:

  1. Public API:

    I'm not familiar with paramiko's current API (my only experience is through Fabric), but I would've assumed something like the following behavior, when given my example config file:

    result = parse_ssh_config(file, host="gateway")
    result.ProxyCommand
    # None
    
    result = parse_ssh_config(file, host="other")
    result.ProxyCommand
    # "ssh -o ..."
  2. How to track nones internally to preserve the correct "which host wins" behavior

    In Haskell, I'd go to Maybe for something like this, with Just None taking precedence over Nothing for the purpose of deciding which host wins. They would be equivalent in the public API though, both resulting in whatever you decided meant "no proxy command".

    Is there something like this in python?

@bitprophet bitprophet modified the milestone: 1.16.1, 1.16.2 Apr 25, 2016
@bitprophet
Member

Re 1: yes, that's why I was kinda leaning towards the "change the behavior to be more correct/explicit" option - it's definitely more in line with expectations. Only downside is it's backwards incompatible, so I would toss this into the 3.0 bucket (ETA: by PyCon-US or soon thereafter) if I go this route.

(Alternately, we could do it in two steps, go the "strip more correctly" route now as a straight bugfix, then update it to leave the Nones in place in 3.0. Depends on how badly you want this fixed before then :D)

Re 2: naw, Python doesn't have as "rich" a value structure as Haskell, None is basically it for "built-in" values meaning "not some actual value".

Sometimes folks use locally defined classes/instances when they need something that can't be confused with None (i.e. if None is itself a legit value for a field storing, say, results of function calls, and so you need a way to truly specify "no value was given, not even None).

But that's not ridiculously Pythonic and in this case there's no chance for confusion since the only extant values in the parsed config are strings or ints/floats. IMO the average Pythonista would expect ProxyCommand none to turn into {'proxycommand': None}.

@pbrisbin

Just to make sure I understand:

Host x

And

Host x
  ProxyCommand none

Both appear the same to clients. Specifically, both are represented as the proxycommand key just not being present in a map. Preserving this is required to be backwards-compatible.

The issue is that in addition to being returned as the same representation, they're also handled internally as the same representation (not being present in the map), and so no none setting can override a non-none setting even if the host rules say it should.

Assuming I've got that all right, I think I'd vote for your Alternately aside -- not because I'm itching for a fix, but just because I think it's an easy route to do this maintenance:

Ship a backwards-compatible bugfix which treats {} and {'proxycommand':None} as distinct values internally, such that the host rules are respected, but don't include keys with None values in the result returned to the client. Most of this work would not be wasted (IMO) as it's getting your internals in order, with just some compatibility code at the edges.

Later, ship a backwards-incompatible fix that removes the filtering and returns a truer representation of the config by include the keys with None values.

@bitprophet
Member

Yes, that's exactly it. I'll go see about doing step 1 now.

@bitprophet bitprophet changed the title from SSH host option not honored to 'none' ProxyCommand values failing to mask non-'none' ones in less-specific blocks Apr 26, 2016
@bitprophet bitprophet added a commit that referenced this issue Apr 26, 2016
@bitprophet bitprophet Changelog closes #670 7d66aa6
@bitprophet
Member

OK, that's in now, committed to the 1.15 branch & will be forward-ported soon.

Think it's cleaner to close this and make a new spinoff ticket for 3.0. So doing that. Thanks!

@bitprophet
Member

githuuuub y u no longer auto closing stuff via commit message >:(

@bitprophet bitprophet closed this Apr 26, 2016
@pbrisbin

Thanks!

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