diff options
| -rw-r--r-- | paramiko/agent.py | 8 | ||||
| -rw-r--r-- | paramiko/pkey.py | 4 | ||||
| -rw-r--r-- | sites/www/changelog.rst | 10 | ||||
| -rw-r--r-- | tests/agent.py | 58 |
4 files changed, 65 insertions, 15 deletions
diff --git a/paramiko/agent.py b/paramiko/agent.py index 9d215b85..b29a0d14 100644 --- a/paramiko/agent.py +++ b/paramiko/agent.py @@ -53,6 +53,8 @@ ALGORITHM_FLAG_MAP = { "rsa-sha2-256": SSH_AGENT_RSA_SHA2_256, "rsa-sha2-512": SSH_AGENT_RSA_SHA2_512, } +for key, value in list(ALGORITHM_FLAG_MAP.items()): + ALGORITHM_FLAG_MAP[f"{key}-cert-v01@openssh.com"] = value # TODO 4.0: rename all these - including making some of their methods public? @@ -482,7 +484,11 @@ class AgentKey(PKey): def sign_ssh_data(self, data, algorithm=None): msg = Message() msg.add_byte(cSSH2_AGENTC_SIGN_REQUEST) - msg.add_string(self.blob) + # NOTE: this used to be just self.blob, which is not entirely right for + # RSA-CERT 'keys' - those end up always degrading to ssh-rsa type + # signatures, for reasons probably internal to OpenSSH's agent code, + # even if everything else wants SHA2 (including our flag map). + msg.add_string(self.asbytes()) msg.add_string(data) msg.add_int(ALGORITHM_FLAG_MAP.get(algorithm, 0)) ptype, result = self.agent._send_message(msg) diff --git a/paramiko/pkey.py b/paramiko/pkey.py index 4f9f6d57..60e5a8fc 100644 --- a/paramiko/pkey.py +++ b/paramiko/pkey.py @@ -797,7 +797,9 @@ class PKey: # message; they're *IO objects at heart and their .getvalue() # always returns the full value regardless of pointer position. self.load_certificate(Message(msg.asbytes())) - # Read out nonce as it comes before the public numbers. + # Read out nonce as it comes before the public numbers - our caller + # is likely going to use the (only borrowed by us, not owned) + # 'msg' object for loading those numbers right after this. # TODO: usefully interpret it & other non-public-number fields # (requires going back into per-type subclasses.) msg.get_string() diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 4b39715a..53f2b84c 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,16 @@ Changelog ========= +- :bug:`- major` Fixed a very sneaky bug found at the apparently + rarely-traveled intersection of ``RSA-SHA2`` keys, certificates, SSH agents, + and stricter-than-OpenSSH server targets. This manifested as yet another + "well, if we turn off SHA2 at one end or another, everything works again" + problem, for example with version 12 of the Teleport server endpoint. + + This has been fixed; Paramiko tweaked multiple aspects of how it requests + agent signatures, and the agent appears to do the right thing now. + + Thanks to Ryan Stoner for the bug report and testing. - :bug:`2012 major` (also :issue:`1961` and countless others) The ``server-sig-algs`` and ``RSA-SHA2`` features added around Paramiko 2.9 or so, had the annoying side effect of not working with servers that don't diff --git a/tests/agent.py b/tests/agent.py index fdc80eba..bcbfb216 100644 --- a/tests/agent.py +++ b/tests/agent.py @@ -2,7 +2,7 @@ from unittest.mock import Mock from pytest import mark, raises -from paramiko import AgentKey, Message +from paramiko import AgentKey, Message, RSAKey from paramiko.agent import ( SSH2_AGENT_SIGN_RESPONSE, SSH_AGENT_RSA_SHA2_256, @@ -10,6 +10,8 @@ from paramiko.agent import ( cSSH2_AGENTC_SIGN_REQUEST, ) +from ._util import _support + # AgentKey with no inner_key class _BareAgentKey(AgentKey): @@ -90,30 +92,60 @@ class AgentKey_: assert key.asbytes() == key.inner_key.asbytes() @mark.parametrize( - "kwargs,expectation", + "sign_kwargs,expected_flag", [ # No algorithm kwarg: no flags (bitfield -> 0 int) (dict(), 0), (dict(algorithm="rsa-sha2-256"), SSH_AGENT_RSA_SHA2_256), (dict(algorithm="rsa-sha2-512"), SSH_AGENT_RSA_SHA2_512), + # TODO: ideally we only send these when key is a cert, + # but it doesn't actually break when not; meh. Really just wants + # all the parameterization of this test rethought. + ( + dict(algorithm="rsa-sha2-256-cert-v01@openssh.com"), + SSH_AGENT_RSA_SHA2_256, + ), + ( + dict(algorithm="rsa-sha2-512-cert-v01@openssh.com"), + SSH_AGENT_RSA_SHA2_512, + ), ], ) - def signing_data(self, kwargs, expectation): + def signing_data(self, sign_kwargs, expected_flag): class FakeAgent: def _send_message(self, msg): + # The thing we actually care most about, we're not testing + # ssh-agent itself here self._sent_message = msg sig = Message() sig.add_string("lol") sig.rewind() return SSH2_AGENT_SIGN_RESPONSE, sig - agent = FakeAgent() - key = AgentKey(agent, b"secret!!!") - result = key.sign_ssh_data(b"token", **kwargs) - assert result == b"lol" - msg = agent._sent_message - msg.rewind() - assert msg.get_byte() == cSSH2_AGENTC_SIGN_REQUEST - assert msg.get_string() == b"secret!!!" - assert msg.get_string() == b"token" - assert msg.get_int() == expectation + for do_cert in (False, True): + agent = FakeAgent() + # Get key kinda like how a real agent would give it to us - if + # cert, it'd be the entire public blob, not just the pubkey. This + # ensures the code under test sends _just the pubkey part_ back to + # the agent during signature requests (bug was us sending _the + # entire cert blob_, which somehow "worked ok" but always got us + # SHA1) + # NOTE: using lower level loader to avoid auto-cert-load when + # testing regular key (agents expose them separately) + inner_key = RSAKey.from_private_key_file(_support("rsa.key")) + blobby = inner_key.asbytes() + # NOTE: expected key blob always wants to be the real key, even + # when the "key" is a certificate. + expected_request_key_blob = blobby + if do_cert: + inner_key.load_certificate(_support("rsa.key-cert.pub")) + blobby = inner_key.public_blob.key_blob + key = AgentKey(agent, blobby) + result = key.sign_ssh_data(b"data-to-sign", **sign_kwargs) + assert result == b"lol" + msg = agent._sent_message + msg.rewind() + assert msg.get_byte() == cSSH2_AGENTC_SIGN_REQUEST + assert msg.get_string() == expected_request_key_blob + assert msg.get_string() == b"data-to-sign" + assert msg.get_int() == expected_flag |