Openssh compatibility #93

Merged
merged 44 commits into from Mar 1, 2013

Projects

None yet

6 participants

@lndbrg
Contributor
lndbrg commented Oct 16, 2012

Provide better support for the rules specified by man ssh_config

@bitprophet
Member

I'm +1 on the changes generally, but not sure I can merge this before a 2.x version, as it -- however correctly -- breaks backwards compatibility.

For example, the IdentityFile change turns config objects' identityfile key values into lists when they were strings before, so code expecting a string in that slot (e.g. parts of Fabric, though that is only one client-usage example) will break.


Another outright problem (i.e. it would need fixing even if we could merge this now) seems to be that settings no longer override correctly, e.g. in this test ssh_config file, when I would expect config['myhost']['port'] to be 664, it instead turns out to be 666.

Note how in that linked sample config the Host * block is at the top, not bottom, of the file. I actually tested this with regular ssh and it seems to have the same bug re: putting the wildcard up top, so perhaps technically your setup is "correct", but I'd still classify this as a bug and it's certainly confused my users in the past.

(If you can provide an argument why this specific behavior has to occur for other, correct/useful behavior to work, I may reconsider, in which case this would again fall under the "backwards incompat" umbrella.)


I do like the regex treatment for ProxyCommand and will be pulling in that chunk of it re #97 and crediting you for it as usual.


Thanks in general for this -- apologies for the kickback!

@bitprophet
Member

Another note re: the ordering thing, this came up in 'ssh' #33 (see last comment) and that's one reason why the implicit/explicit Host * block is always bumped to the end of the list when it's generated -- your updates most likely removed that behavior or at least didn't preserve it.

@Matir
Matir commented Nov 6, 2012

According to man ssh_config, "For each parameter, the first obtained value will be used." Since Host * matches all hosts, any values set in Host * will override values set later in the file. For this reason, I usually place Host * at the bottom of my .ssh/config

@lndbrg
Contributor
lndbrg commented Nov 6, 2012

The behaviour described in 'ssh' #33 is actually wrong. I have done extensive testing against latest version of openssh and read the man page several times. These set of patches actually conforms to the described behaviour of man ssh_config, where as earlier it was a bit of a hit and miss.

@bitprophet
Member

See also Fabric #816 which I believe backs up the changes indicated here.

I also skimmed my earlier comments here and am now unsure whether it's wise to merge this before or after a backwards compat barrier; we did change the behavior in a major way in Paramiko 1.8 (thus causing the above user's issue) so for better or worse there is precedent. So, when I revisit this I may change that decision and go for this for Paramiko 1.10/1.11/whatever.

@lndbrg
Contributor
lndbrg commented Jan 28, 2013

@bitprophet it might be a wise idea to also consider merging a mix of #110 and #128 when merging these. They add better logic for fqdn handling.

@bitprophet
Member

@lndbrg Yea, I see them. Thanks! Will poke at all this now and hopefully I'll feel confident enough to merge all of it before I shove out 1.10 (tonight, if we're lucky, otherwise this weekend.)

@bitprophet
Member

@lndbrg If you have time to rebase this against latest master & verify the result is still kosher, I'll do my best to merge it this weekend and release. Thanks for your patience!

@lndbrg
Contributor
lndbrg commented Feb 28, 2013

I'll try to do it today or tomorrow.

@lndbrg
Contributor
lndbrg commented Feb 28, 2013

Hmms, it looks like i got some of yout commits in it too. But this works, no failing tests

@bitprophet bitprophet added a commit that referenced this pull request Mar 1, 2013
@bitprophet bitprophet Changelog re #93 2775263
@bitprophet bitprophet merged commit 1903ee1 into paramiko:master Mar 1, 2013

1 check passed

default The Travis build passed
Details
@bitprophet
Member

Skimmed the new tests, reread man ssh_config a bit, I think this looks good now. Paramiko's tests pass so I've merged into master. Have to update some of Fabric's test suite to account for the new behavior and then that is another level of testing.

Will release Paramiko 1.10 and Fabric 1.6 today so hopefully folks will soon either be happy or discover anything we've both missed.

Thanks yet again for the help on this, @lndbrg!

@lndbrg lndbrg added a commit that referenced this pull request Oct 2, 2014
@lndbrg @bitprophet lndbrg + bitprophet Update documentation of lookup method.
The documentation reflected pre #93 behaviour.
7895271
@hhgabelgaard hhgabelgaard referenced this pull request in clouder-community/clouder May 4, 2015
Closed

identityfile is a list if paramiko >= 1.10.0 #11

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