Add support for new-ish 'Match' config directive

[illtown]

[Maintainer edit: this isn't a bug but simply a lack of a new upstream feature. See http://man.openbsd.org/ssh_config for details on Match.]

---

this option is applied to * (all hosts) and should be evaluated to give true or false, and according to this set further parameters until next 'Host' or 'Match' keyword.
Paramiko now treats 'Match' keyword as a parameter of *

[bitprophet]

It looks like we've never actually supported Match.

But this may be because it seems to have only been added to OpenSSH in 2013! :) http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/readconf.c?rev=1.206&content-type=text/x-cvsweb-markup

Will tweak ticket subject/desc, and would happily accept a PR if one was submitted.

[illtown]

Yeah, I've looked through the source code and found no mention of Match.
And I found a lot of 'if/elif/else' while parsing ssh_config file.
I wonder, would it be better to rely on ssh parsing its config himself?
For example, paramiko could just parse the output of ssh -G mysecretserver.com instead of reimplementing the parsing techniques.
Just a thought.

[bitprophet]

@illtown Problem is that then ties us to requiring OpenSSH being present, which almost defeats the purpose of this being a standalone SSH implementation ;)

(I could see us adding that as a tool in the test suite, however - a way of ensuring we're "up to spec". Hrm.)

Re: if/else/etc, he parsing code, at its heart, is quite old (like the rest of the codebase) so I'm sure it could be cleaned up a good amount. Presumably we could leverage a grammar library or similar. Anyway, not a super high priority right now, there are other areas with more serious bugs/corner cases that need similar attention :(

[bitprophet]

#896 brings up the point that was only implicit here, namely that because we appear to just ignore Match lines entirely, things inside such blocks are parsed as if they were normal top level rules...which is actively unsafe/wrong.

I'd accept either a PR that implements Match outright or one that at least attempts to skip past anything inside such blocks, which I assume would be a bit simpler/faster. Still don't have time myself to dig into this.

[mleinart]

FWIW I started picking at this after filing #896 here: mleinart:match_parsing. I'm trying to be delicate and not change structure too much while also keeping an eye to what I'll need for #897

This is what I've got so far (probably will poke at it more):
mleinart/paramiko@97085f6
mleinart/paramiko@2bfb50e

[bitprophet]

Seems reasonable at a quick glance; I need to read up on exactly how Match is supposed to work before I can give a qualified thumbs up though. EDIT: also not philosophically opposed to improving how SSHConfig is organized/API'd (like with most of this codebase 😆 ) but agree a minimum-viable-changeset is the first thing we should be merging. Thanks for taking a stab!

[mleinart]

Cool! I'm going to keep poking at it. Sitting down to get some better tests written might be the thing that prompts me to do a bigger refactor :p

[computator]

Just wanted to say that maybe this should be higher priority because it breaks entirely if there are any match blocks in your config. I have some match blocks unrelated to the host I am attempting to connect to, but the config in the match blocks is overriding the hostname (among other things) thus causing it to try and connect to the wrong host.

[bitprophet]

See potentially useful test case in #1297 (comment) from a dupe ticket.

I have the need for this internally now so tackling it this week. Will see how easily I can fold in the work from/for #897 and maybe eg #872 while we're at it. Usual "iteration > biting off more than we can chew" rule applies - better to release some decent fix than spend weeks polishing a bigger overhaul.

[bitprophet]

Some day I'll learn never to give ETAs 😩 #897 is done as of today, I think this will be next priority, unless #872 looks real easy.

[bitprophet]

I'm something like 3/5 of the way through this so far, it's a slog unfortunately - I can't in good conscience only do it partway due to the nature of the feature, and history shows that doing so anyways leads to a lot more badfeels for users. ("Does not support X" is much easier to understand and work around than "Supports X buuuuuuuut...)

Wasn't able to make direct use of @mleinart's patch as it seems to only really work for single-keyword Match directives (i.e. Match host foo but not Match host foo originalhost biz,baz localuser bar) due to short-circuiting on success as well as failure. However it's been valuable as a sanity check otherwise, so cheers for that.

---

Main reason I am dropping a note is to record (it's in the docs too tho) the unfortunate fact that Match user is hamstrung by not having access to connection-time data. It seems clear that most "real" uses of that sub-keyword are intended to catch stuff like ssh user@host, so without access to that runtime user, all we can really do is look at any configured User bits.

In future we may want to update SSHConfig.lookup to accept an optional username argument so at least Fabric et al can supply this info. That's out of scope for now though.

[bitprophet]

Everything but Match exec is done. Some thoughts on that:

• Ideally I'd love to replace subprocess with invoke, since it could potentially clean up all the raw select/subprocess stuff in older ProxyCommand code here, and would make implementing Match exec trivial.
  • If I don't do this, I'll have to crap out some boilerplate subprocess-using code for exec, because the code for ProxyCommand isn't terrifically reusable due to its interweaving w/ timeout & socket-like behavior concerns, neither of which apply to config parsing.
    • I double checked and yea, openssh does not appear to have any timeout specified for Match exec subprocesses.
• Problem the first: the ProxyCommand code has some minor differences from the typical behavior of invoke.Runner, such as returning partially-read results (and swallowing the exception) on timeout, manipulating Popen's bufsize argument (Invoke doesn't touch this at the moment), and not specifying shell=True (Invoke currently always calls Popen(..., shell=True), at least when subprocess is involved at all [the "no pty" use case]).
  • Suspect these are all fixable pretty easily & would have minor benefits for Invoke's userbase as well - but it's still work that needs doing & releasing before we can specify an Invoke release as a Paramiko dependency.
  • On top of that, it might be fun to shove ALL of ProxyCommand into Runner, insofar as ProxyCommand is just tying together subprocess and "acts like a socket". Making Runner (or some wrapper thereof) also implement that interface might be wise.
    • This would leave Paramiko's side of things almost purely "run this Invoke code & transmute read errors into ProxyCommandFailure".
  • An aside - the new 2nd use of subprocess-or-invoke, for Match exec, does work well with existing Invoke code because Match exec explicitly states it uses the user's shell (& its implementation mirrors Invoke's non-pty mode of execution, which is also its default).
• Problem the second: adding Invoke as a Paramiko dependency complicates the "trinity" that already exists for me between Paramiko, Invoke, and Fabric, insofar as it means Fabric 2 would depend on Invoke twice (directly and also transitively).
  • Obviously, as I control all of them, it should be possible to keep everything lined up correctly - it just means a bit more work/bookkeeping/opportunity for tripping over myself
  • And that some Fabric 2 users will need to upgrade their Invoke more often than they might otherwise. Should not be an issue since I am overly obsessed with backwards compat...
• Problem the third: Paramiko's current use of subprocess is conditional/optional - it's wrapped in a try/except import block because some folks use Paramiko inside cloud-exec platforms like GCE which may run trimmed down Python interpreters that do not offer subprocess. And Invoke does not do this (partly because shell commands are one of its primary use cases, vs Paramiko where it is a very minor feature).
  • So this is another Invoke level change that would need to happen, one that's slightly more potentially disruptive than adding minor new levers/options (per problem the first).
  • Not too worrisome, and we already have some conditional imports there for eg Windows support. I just don't love it.
  • alternately, I could simply make the imports of Invoke conditional within Paramiko, and stop worrying about this particular wrinkle. Duh?

---

I am currently leaning towards a "have cake & eat too" option so I can wrap up this long-running feature add:

• Add Invoke as an optional install dependency (similar to how we treat gssapi requirements)
  • Not 100% sold on this but it limits pre-3.0 instability and helps with any potential problems even installing Invoke on those limited platforms. (though I don't think that's actually a big deal, the big deal should only be at import time)
  • OTOH the stumbling block of "oh, I needed to install this other thing too?" is a pain and Invoke takes care to be widely installable...
  • Right now Paramiko is very mixed on its requirements, eg we hard require two libraries purely for Ed25519 support, which obviously not all users need.
    • Feels like something to be fully fixed in 3.0, with a no-deps-besides-Crypto core, additional extra-depends like gssapi, and some kitchen-sink extra-depends with everything.
    • OTOH, there's no real reason to wait on the kitchen-sink extra as it would be purely additive.
  • OS level packagers need to deal with even optional deps, typically, so that's one argument for "you're overthinking this, just make it a hard dep for now & sort this all out later"...
• Use it for Match exec only, for now, because it should be trivial/easy/not require any hacking on Invoke's end
• In some future release, after Invoke gains what we need for ProxyCommand's use case, we consume the ProxyCommand subprocess use w/ Invoke too (and bump the dependency to whatever version that is)

[bitprophet]

God bless my habit of leaving large blocks of text for future-me. Would've been hard to remember all the above after weeks of dayjob and family obligations...

Re: the questions above:

• Makes most sense to NOT hard-require Invoke:
  • we must treat its import as if it were optional, since otherwise we'd end up triggering its subprocess import and make Paramiko unusable on limited platforms again.
  • And if we're doing that, having it a hard install requirement is just silly.
  • Plus if the 3.0 intent is to have that "limited requirements core + various extras_require", it's also silly to go the other direction before then, if there's no gain.
• I may experiment briefly with adding more extras_require, since that is usually low-cost
• Do the conditional import and use for Match exec purposes
• See what else is left re: unfulfilled tests

[bitprophet]

This is done and in master now! 🎉 Look for it in the 2.7 release, out soon™