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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-monkeypatch mutation of preferred kex algorithms etc #1463

Closed
bitprophet opened this issue Jun 21, 2019 · 8 comments
Closed

Allow non-monkeypatch mutation of preferred kex algorithms etc #1463

bitprophet opened this issue Jun 21, 2019 · 8 comments
Labels

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

There may well be an open feature ticket or PR for this but I can't find it...馃槖

Most recent example of why this is frequently useful: #1455, where the recently released new set of kex algorithms doesn't work in all cases and users need to disable it without just downgrading back to the previous release. But it comes up basically anytime we modify the set of implemented kexen, ciphers, etc.

It'd be nice if this could play well with OpenSSH config support as well, but last I checked we don't actually do a lot of automagic handling there so that might need to live in Fabric 2 land. I'd also rather get this out sooner instead of later.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

Brainstorm for how we could do this in a backwards compatible fashion:

Technically we could move around the _preferred_* members of Transport, but sadly this "API" well predates even my involvement and folks have been told to monkeypatch them for ages. So they are effectively public. Meaning anything that actually mutates them is probably a no-go.

Where are these members defined & used? Currently:

  • _preferred_ciphers holds the cipher list
    • it is only ever referenced for matching the other end, and is never modified save via SecurityOptions' setter (I don't even remember why this class exists, tbh).
  • _preferred_macs holds the MAC list
    • is otherwise same as ciphers.
  • _preferred_keys holds the list of key types
    • similar to ciphers and macs re: usage
    • one extra: when Transport.connect() is given a hostkey= value, that hostkey object's key name (literally its .get_name()) is used to overwrite _preferred_keys, ensuring that the handshake requests only that key type from the server
  • _preferred_kex is the list of kex algos
    • This gets mutated at class definition time based on if KexCurve25519.is_available(), and is otherwise left alone
  • _preferred_gsskex is a list of GSS specific kexen
    • Is added into _preferred_kex during __init__ if use_gss_kex==True
  • _preferred_compression starts with just ("none",)
    • and is updated to include some references to zlib if Transport.use_compression(True) is called

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

Dug into how SSHClient treats Transport() and was surprised to find that the former calls the latter only inside connect()! So while the monkeypatch suggested is usually from paramiko import Transport; Transport._preferred_kex = ["my", "new", "list"] actually making it work in a non-monkeypatched manner will take slightly more work.

What are the most common updates users will want? Typically it's stripping out "known bad" algorithms (as seen in #1455), though another common thing is limiting to a specific subset (e.g. what you'd do in your ssh_config if you were security conscious).

The latter, Paramiko tends to get PRs from folks asking for removal of old, known bad ciphers, so that's less critical IMHO (though would still be nice to offer sometime - probably in Fabric as Paramiko itself does not leverage its own SSHConfig anywhere).

For the former, seems we would want to:

  • Add new kwargs such as, idk, disable_kexen=["diffie-hellman-group16-sha512"] (or perhaps disable_algorithms={"kex": "diffie-hellman-group16-sha512"}) to Transport.__init__
  • Transport.__init__ stores those disable requests
  • Replace all read references to Transport._preferred_* with similarly-named properties that refer to the disable info and return filtered lists
  • Update Client to pass these kinds of args through (either just in connect, which I hate because that already has a million args, or bypass it with __init__ + attributes, which feels clunky)
  • Profit?

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

But then I realized that's similar to what I saw in SecurityOptions, and...this is all exactly what SecurityOptions says it's for! So why the hell are people always suggesting a monkeypatch? How old even is this class? It has no .. versionadded:: so it's probably very old?

Yea...it's been in since 2004, well before my time. Does it even work? Fabric never used it, and I never see anyone reporting bugs against it or etc, so my guess is everybody else slept on it like I did.

Will see whether this can end up being resolved with just a docs-level fix...I don't think I like SecurityOptions' design, but if it's functional, it's much better than new code doing arguably the same thing.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

Sadly it literally is the same as "overwrite the entire lists" so it's not useful for disabling only one at a time (you still have to copypasta all the other ciphers you might want) and it still suffers from the core problem above of the Client -> Transport lifecycle. So I think I do want to work my own thing in here.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

Also, I think I can stop worrying about folks monkeypatching, since most of them are presumably using the "modify the class attributes" approach, which is orthogonal to what I am imagining here (filtering those class attributes by the runtime-specified disable list).

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Jun 21, 2019

it is possible to filter-out with SecurityOptions with a bit of code, like SSHClient does for the host key type:
https://github.com/paramiko/paramiko/blob/2.5.0/paramiko/client.py#L393-L395

... but the "overwrite entire list" approach may make it simple to support the OpenSSH options "KexAlgorithms" and "Ciphers" etc in the ssh_config or from command-line options similar to what would be passed to openssh

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 21, 2019

Correct, and I'd like to do that sometime, but for now I'd just like to solve the case of "strip out this one algorithm" so folks stop having to cart around (inevitably outdated later on) lists to monkeypatch with.

I also find SecurityOptions to be a very bizarre method of solving this problem (on top of how it still requires full-scale overwriting with entire lists) so I'd prefer not to double down on it at this point.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 22, 2019

This is in master now.

@bitprophet bitprophet closed this Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants