Fix a couple minor-but-critical Agent issues wrt SHA2 + certs

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