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

Add rsa-sha2-256 and rsa-sha2-512 algorithms #1643

Closed
wants to merge 2 commits into from

Conversation

krisztian-kovacs
Copy link

These are specified in RFC 8332 (https://tools.ietf.org/html/rfc8332) and proposed by recent OpenSSH versions as a drop-in replacement for the deprecated ssh-rsa algorithm.
The advantage is that the same RSA keys can be used without relying on the SHA-1 digest now considered insecure.

This change adds support for the rsa-sha2-256 signature algorithm specified
in RFC 8332. A new RSA key type class is added, with the only difference
comared to RSAKey that signing and verifying blobs uses the SHA2-256 digest
algorithm instead of the deprecated SHA-1.

Recent versions of OpenSSH have deprecated algorithms using SHA-1.
This algorithm is specified in RFC 8332 and is basically ssh-rsa with
signing and verification using the SHA2-512 digest algorithm.
@ploxiln
Copy link
Contributor

ploxiln commented Mar 13, 2020

Cool. This looks like a more complete alternative to #1520

@bitprophet
Copy link
Member

Dropped in rapidly growing p0 milestone. I need to put some attention into the other two legs of my big project triad, but when I circle back here this will get looked at. Thanks!

@ChristopherRabotin
Copy link

Hey there. Is there anything I can do to help get this PR reviewed & merged and released in the next version of paramiko?

@bitprophet
Copy link
Member

Hey there. Is there anything I can do to help get this PR reviewed & merged and released in the next version of paramiko?

As long as it still merges cleanly, nope, it's just waiting on my time to roll through the milestone and merge stuff for release (as noted). Thanks for your patience!

@archoversight
Copy link

This is going to be key to continue functioning against RHEL 8 systems in FIPS mode where ssh-rsa is disabled completely.

@ChristopherRabotin
Copy link

ChristopherRabotin commented Nov 16, 2020

Have you tested this code for authentication against a server? I checked out your branch and tried connecting using a 3072 SHA256 key, and I still have "authentication failure." I'm sure the password provided to the key is correct because if it isn't, then the RSAKey object cannot be created (bit mismatch error or something).

Any idea what could be happening?

Edit: Let me add that the remote server I'm trying to connect only supports SFTP connections and does not allow for pure SSH connections. When trying to connect in that way, the server will close the connection after saying that only SFTP is supported. Could this confuse paramiko into thinking that the auth failed?

Thanks

Screenshot_2020-11-16 Kubernetes Dashboard

@bitprophet
Copy link
Member

Edit: Let me add that the remote server I'm trying to connect only supports SFTP connections and does not allow for pure SSH connections. When trying to connect in that way, the server will close the connection after saying that only SFTP is supported. Could this confuse paramiko into thinking that the auth failed?

Off the top of my head, this may be plausible, in which case you may find yourself pending on the auth reorganization listed under #387 (though I'd also consider a more targeted PR if it turns out that your analysis is correct, and if there's a reasonably simple way to fix it w/o upending regular ssh behavior. Big if, sadly.)

@krisztian-kovacs
Copy link
Author

@ChristopherRabotin Yes, this works for us with OpenSSH. (And do test our own third party implementation of rsa-sha2-256 and rsa-sha2-512 with this.) Do you have logs from the server that could maybe in diagnosing the issue?

@ChristopherRabotin
Copy link

ChristopherRabotin commented Nov 22, 2020 via email

@krisztian-kovacs
Copy link
Author

In this case maybe you could get the logs from paramiko itself. You can enable logging to a file like this:

paramiko.util.log_to_file("demo_server.log")

(From https://github.com/paramiko/paramiko/blob/master/demos/demo_simple.py#L39)

Feel free to remove any potential sensitive content, the SSH message sequence is what's interesting to help debug the issue.

@Dith3r
Copy link

Dith3r commented Apr 1, 2021

There is also list in client.Client._auth where it test filename and select only one type of RSAKey (ssh-rsa).

@kuwv
Copy link

kuwv commented Jun 10, 2021

Would this work with: #1103

@zdlhappy
Copy link

zdlhappy commented Jun 18, 2021

When to support rsa-sha2-512 and rsa-sha2-256? In progressing? @bitprophet


class RSASHA512Key(RSAKey):
"""
A special RSAKey that uses SHA-256 digest for sign/verify.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a typo here? Should it be SHA-512?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so.

@Alphadelta14
Copy link

Have you tested this code for authentication against a server? I checked out your branch and tried connecting using a 3072 SHA256 key, and I still have "authentication failure." I'm sure the password provided to the key is correct because if it isn't, then the RSAKey object cannot be created (bit mismatch error or something).

Any idea what could be happening?

Edit: Let me add that the remote server I'm trying to connect only supports SFTP connections and does not allow for pure SSH connections. When trying to connect in that way, the server will close the connection after saying that only SFTP is supported. Could this confuse paramiko into thinking that the auth failed?

Thanks

Screenshot_2020-11-16 Kubernetes Dashboard

I ran into the same sort of issue when tracking this issue down. The auth banner isn't particularly related. I did find in the server logs a userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedKeyTypes [preauth] rejection message that corresponded to it.
Wish the logs were better about that, but I guess we shouldn't expect servers to provide sensitive information about why they were rejected.
Switching pkey=RSAKey() to pkey=RSA512Key() from this PR fixed it for me though.

@Alphadelta14
Copy link

Alphadelta14 commented Jun 25, 2021

I ran into the same sort of issue when tracking this issue down. The auth banner isn't particularly related. I did find in the server logs a userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedKeyTypes [preauth] rejection message that corresponded to it.
Wish the logs were better about that, but I guess we shouldn't expect servers to provide sensitive information about why they were rejected.

Actually, it might be a good idea to check the key.get_name() in Transport.auth_publickey() to make sure that it inside of server_key_algo_list?

@Dreamsorcerer
Copy link

Any movement on this? ssh-rsa has now actually been deprecated and is getting removed from openssh. This PR is starting to become a notable security issue. Would be great to ensure this is merged and released in time for inclusion into the Ubuntu 22.04 archives.

@sktomer
Copy link

sktomer commented Aug 2, 2021

Hello... we really need this patch so that paramiko plays nice with the latest default crypto policies in fedora34 or later.
Is there a blocker for this patch or can it be qualified and merged asap ?

@bskinn
Copy link
Contributor

bskinn commented Aug 3, 2021

@Dreamsorcerer, what is the deadline for inclusion in Ubuntu 22.04?

@sktomer, is there any specific deadline associated with fedora crypto compat? Or just desirable ASAP because desirable (and secure)?

@Dreamsorcerer
Copy link

@Dreamsorcerer, what is the deadline for inclusion in Ubuntu 22.04?

I suspect the deadline is around February, but they probably just pull the package from Debian, so a Debian maintainer will need to package and release it before then. So, I'd say before the end of this year to be safe.

@bitprophet
Copy link
Member

Making the various accepted algorithms/ciphers/key types configurable is a longterm desire, but I'm hoping to get this out w/o adding too many dependencies. FWIW I am intending to follow OpenSSH here by having the sha2 based algorithms appear earlier in those lists, thus any connection to servers implementing sha2 would end up preferring that method.

I am still rummaging around in the code on my end and writing tests; intending to have this done by end of week unless something else blows up on me.

@bitprophet
Copy link
Member

bitprophet commented Dec 14, 2021

This gets hairier the more I dig (partly because a lot of the code in Transport that must change, is super old and wasn't written too forward-thinking or test-oriented) but I think I've mostly got it sorted in my head now, have wrapped the problem in TODOs and tests, and am starting the actual implementation finally (as of yesterday).

The main question mark is whether to bother with MSG_EXT_INFO and its server-sig-algs aspect, or to punt on it. I am leaning towards the latter, given a) Paramiko now has disabled_algorithms kwarg in Transport and SSHClient, so users who know they're connecting to legacy servers can disable SHA-2 easily; and b) OpenSSH itself defaults to the SHA-2 family before the legacy option, even in situations where a target doesn't support server-sig-algs, so there's precedent.

The counterargument is that some folks may have large target fleets whose SHA-2 support is heterogenous but which all supports server-sig-algs (which is where supporting that on the client end would be most valuable). I expect such users will make themselves known and maybe even supply a followup PR...


My current changelog entry for this request (stripping out some TODOs re: server-sig-algs) is as follows:

- :feature:`1643` Add support for SHA-2 variants of RSA key verification
  algorithms (as described in :rfc:`8332`). How SSH servers/clients decide when
  and how to use this functionality can be complicated; Paramiko's support is
  as follows:

  - Client verification of server host key during key exchange will now prefer
    ``rsa-sha2-512``, ``rsa-sha2-256``, and legacy ``ssh-rsa`` algorithms, in
    that order, instead of just ``ssh-rsa``.

      - Note that the preference order of other algorithm families such as
        ``ed25519`` and ``ecdsa`` has not changed; for example, those two
        groups are still preferred over RSA.

  - Server mode will now offer all 3 RSA algorithms for host key verification
    during key exchange, similar to client mode, if it has been configured with
    an RSA host key.
  - Client mode, when performing public key authentication with an RSA key or
    cert, will now attempt to auth using ``rsa-sha2-512``, ``rsa-sha2-256``,
    and ``ssh-rsa``, in that order, by default.

      - This mimics the default behavior of the OpenSSH client (as of the 8.x
        line and late 7.x) re: its ``PubkeyAcceptedAlgorithms`` (fka
        ``PubkeyAcceptedKeyTypes``) setting.
      - The existing ``disabled_algorithms`` argument to
        `~paramiko.transport.Transport` and `~paramiko.ssh_client.SSHClient`
        may be used to disable the SHA-2 RSA algorithms when connecting to
        legacy servers.

  - Server mode is now capable of pubkey auth involving SHA-2 signatures from
    clients (who are responsible for indicating the exact algorithm to use, so
    there is no guessing required on the server end).

EDIT: I still have the other related tickets open in tabs; agent support (#1925 / #1644) seems like it really wants to be in the same release, but it may get its own changelog entry or whatnot. #1103 I'll likely punt on given how long this has taken!

@bitprophet
Copy link
Member

Have the tests passing for hostkey exchange, which also necessitated doing part of the work for pubkey signatures as well. Hoping to wrap that and agent stuff up by early next week, if not sooner, and get this published.

@JackCurtin-eaton
Copy link

JackCurtin-eaton commented Dec 21, 2021

I'd love to be able to have this for my regression tests this week so I can update our product to open SSL 1.1.1l :)

@bitprophet
Copy link
Member

bitprophet commented Dec 21, 2021

(still poking away, @JackCurtin-eaton ...)

Looking at pubkey auth here and hitting a possible wrinkle, namely that Paramiko's design makes trying >1 pubkey auth step per actual key, a bit fraught - partly due to the underlying network aspect of "send message, wait for reply, act on reply, continue", and partly due to the use of threading events (though I suspect explicitly given events is not widely used and it already doesn't work if one is using eg 2FA).

Approaches open to me seem to be:

  1. Naively wrap all calls to Transport.auth_pubkey in a loop over the possible RSA algorithm types. Gross, copypasta. Also may just not work well in some spots without even more awful logic.
  2. New method on Transport that does the above for you. A bit better, still have to tweak the other calls to use this method but that's not awful. However anybody calling the old method directly will not know to do this w/o reading the changelog & may still be all "wah! why my RSA SHA2 not working?". And this adds more API complexity where we really shouldn't have to.
  3. Push the loop into the existing method. Might be fine, but anybody trying to pass in event will be in big trouble. Is anybody really doing that? No way to tell!
  4. Copy OpenSSH somewhat and just pray selecting the first possible algorithm (honoring disabled_algorithms, so users can control this somewhat) will be the right one. Yea, not great.
  5. Copy OpenSSH fully and implement server-sig-algs support after all, which as noted upthread allows almost-complete removal of guessing (outside of some weird legacy server that might support SHA2 but not server-sig-algs).

Those last two still require a spot of extra logic within auth_pubkey() but as it would be selection of a single algorithm and not a loop over multiple auth attempts, MUCH simpler to do w/o disruption.

Leaning towards the last option there - I think if I have to do weird funky logic and/or mess with the network events, I'd rather just...add MSG_EXT_INFO support anyways.

@bitprophet
Copy link
Member

Got MSG_EXT_INFO working, and most of what else is needed for pubkey auth (hitting some weird snags near the end). Hopefully not a lot more from here, we'll see how tomorrow goes.

@bitprophet
Copy link
Member

bitprophet commented Dec 22, 2021

Finally figured out why SHA2 based pubkey auth wasn't working with a real OpenSSH target but was working vs ourselves. Some very old code turned out to have multiple subtly distinct copies floating around and I'd only found/updated one of them.

Remember kids, friends don't let friends copy and paste.

Need to wrap this present in some more tests (& run a few more real world tests) but hopefully putting it on PyPI soon.

EDIT: oh right, and agent support.

@bitprophet
Copy link
Member

For anyone wanting to do some last minute testing, the branch as it stands is here: https://github.com/paramiko/paramiko/tree/rfc8832-sha2-key-algo - though it is still a feature branch getting force pushed to often so buyer beware. I expect to squash it into only a few commits before merging.

@bitprophet
Copy link
Member

bitprophet commented Dec 23, 2021

OK, I ended up adding serverside EXT_INFO support too as I needed it for testing the client side (derp).

As far as I can tell right now, this works reasonably well and is pretty well tested too (and I made sure that it passes on Python 2 locally, thank you containers!) - Circle is currently happy too: https://app.circleci.com/pipelines/github/paramiko/paramiko/140/workflows/de9d64c8-eb42-48c4-86e4-9a07b86db929

Only thing left is agent. I intend to tackle that tomorrow morning and release to PyPI.

@bitprophet
Copy link
Member

Going to merge my work to main and close this ticket since agent is arguably separate tickets. Thanks all for the patches/feedback/discussion.

@bitprophet bitprophet closed this Dec 23, 2021
@TTimo
Copy link

TTimo commented Dec 23, 2021

How can third parties (me) track when this fix will be available in a new official paramiko release?

@bitprophet
Copy link
Member

bitprophet commented Dec 23, 2021

@TTimo I'll report back here when it's released, but otherwise you can always bookmark the changelog (https://paramiko.org/changelog.html) and look there to see if the entry for this ticket is under a tagged release or still under "next release".

I will be releasing to PyPI today come hell or high water, the only question is whether it will include the agent support or if that will follow later. Gonna poke at that shortly.

@bitprophet
Copy link
Member

Cool, the changes required for agent support are very minor, though once again I had to write my own bits as the existing PRs weren't quite lining up with the latest state of the code. Putting finishing touches on now and a few tests (agent support is not tested at all, argh, wtf folks - I'm only gonna add a few very quick ones but better than nothing!).

@bitprophet
Copy link
Member

2.9 now up on PyPI!

@bskinn
Copy link
Contributor

bskinn commented Dec 23, 2021

👏👏👏

Marathon effort. Thank you!

@bitprophet
Copy link
Member

bitprophet commented Dec 24, 2021

#1955 spawned from this thread. Already have a proving test and fixed enough that it passes, just need to fix regressions in other tests now 🙃

EDIT: 2.9.1 now out.

@Kami
Copy link

Kami commented May 7, 2022

Thanks for contributing this change.

Here is just some info / context on how we handle "backward incompatible" nature of this change as consumers in Libcloud - apache/libcloud#1685.

In short, if we detect paramiko >= 2.9.0 and we get auth error back from the server (afaik, there is no easy way to catch and only retry on a more granular exception, but please correct me if I'm wrong), we try to fall back to the previous approach (disable sha-2 variants). We confirmed this workaround works correctly in various setups.

Sadly this does have a potential to decrease security (think "downgrade" attack) in case user doesn't require backward compatibility layer and that's why we noticed how to disable it in docs / upgrade notes / deployment page.

Feedback and comments are of course welcome.

@perlun
Copy link

perlun commented Feb 21, 2023

@bitprophet I know you are incredibly busy so sorry for disturbing you. I have trouble getting this to work with my (RSA-based) key though. 🤔 Here's what it logs on the (Ubuntu 22.04) server side (OpenSSH_8.9p1 Ubuntu-3ubuntu0.1, OpenSSL 3.0.2 15 Mar 2022):

Feb 21 10:58:02 ubuntu-2204-centre-test-c4d5124d sshd[19345]: userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedAlgorithms [preauth]
Feb 21 10:58:02 ubuntu-2204-centre-test-c4d5124d sshd[19345]: userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedAlgorithms [preauth]
Feb 21 10:58:02 ubuntu-2204-centre-test-c4d5124d sshd[19347]: userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedAlgorithms [preauth]
Feb 21 10:58:02 ubuntu-2204-centre-test-c4d5124d sshd[19347]: userauth_pubkey: key type ssh-rsa not in PubkeyAcceptedAlgorithms [preauth]

Connecting via OpenSSH (OpenSSH_9.1p1 Debian-2, OpenSSL 3.0.7 1 Nov 2022) to the same host with the same key works fine. Paramiko 2.12.0 running from pytest-testinfra 7.0.1.

ssh host -v gives me this on connecting. Any ideas? 🤔

debug1: Offering public key: /home/plundberg/.ssh/id_rsa RSA SHA256:nEecUUWt8//qdNKTcQ5nPAGpTLXmWQm5jBt/Qzx4JvQ agent
debug1: Server accepts key: /home/plundberg/.ssh/id_rsa RSA SHA256:nEecUUWt8//qdNKTcQ5nPAGpTLXmWQm5jBt/Qzx4JvQ agent

@bskinn
Copy link
Contributor

bskinn commented Feb 23, 2023

@perlun It'll be best if you post this as a new issue, linking back to this PR. That way it will be more visible and also I can tag it for Support.

@perlun
Copy link

perlun commented Feb 24, 2023

@perlun It'll be best if you post this as a new issue, linking back to this PR. That way it will be more visible and also I can tag it for Support.

Thanks, good point @bskinn. 👍 Here we go: #2191

@Kache
Copy link

Kache commented Mar 24, 2023

Ran into this "slightly backwards incompatible" issue, as summarized in Paramiko fails ... when upgrading to 2.9.0+

Aren't the paramiko changelog docs for 2.9.0 wrong? I wouldn't have been able to solve the incompatibility without finding this post by @Kami with the correct workaround.

@bskinn
Copy link
Contributor

bskinn commented Mar 24, 2023

Thanks for reporting this, @Kache -- could you repost this as a new issue so that it can be independently addressed?

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

Successfully merging this pull request may close these issues.