From 03df3cf9cd0f12cc04abe88a8674e6968363340c Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 28 Aug 2017 17:45:54 -0700 Subject: Overhaul PublicBlob and use it better within RSAKey. This allows server-side Paramiko code to correctly create cert-bearing RSAKey objects and thus verify client signatures, and now the test suite passes again, barring the stub tests. Re #1042 --- paramiko/client.py | 2 +- paramiko/pkey.py | 105 +++++++++++++++++++++++++++++++++++--------------- paramiko/rsakey.py | 20 +++++++++- paramiko/transport.py | 1 + tests/test_pkey.py | 9 +++-- 5 files changed, 102 insertions(+), 35 deletions(-) diff --git a/paramiko/client.py b/paramiko/client.py index 3b3895b6..bff223ea 100644 --- a/paramiko/client.py +++ b/paramiko/client.py @@ -541,7 +541,7 @@ class SSHClient (ClosingContextManager): self._log(DEBUG, msg) # Attempt to load cert if it exists. if os.path.isfile(cert_path): - key.load_certificate(pubkey_filename=cert_path) + key.load_certificate(cert_path) self._log(DEBUG, "Adding public certificate {0}".format(cert_path)) return key diff --git a/paramiko/pkey.py b/paramiko/pkey.py index 3a872491..948152fa 100644 --- a/paramiko/pkey.py +++ b/paramiko/pkey.py @@ -365,10 +365,11 @@ class PKey(object): encryption ).decode()) - def load_certificate(self, **kwargs): + def load_certificate(self, value): """ - Supplement the private key contents with data loaded from - an OpenSSH public key (.pub) or certificate (-cert.pub) file. + Supplement the private key contents with data loaded from an OpenSSH + public key (``.pub``) or certificate (``-cert.pub``) file, a string + containing such a file, or a `.Message` object. The .pub contents adds no real value, since the private key file includes sufficient information to derive the public @@ -382,7 +383,13 @@ class PKey(object): that is for the server to decide if it is good enough to authenticate successfully. """ - blob = PublicBlob(**kwargs) + if isinstance(value, Message): + constructor = 'from_message' + elif os.path.isfile(value): + constructor = 'from_file' + else: + constructor = 'from_string' + blob = getattr(PublicBlob, constructor)(value) if not blob.key_type.startswith(self.get_name()): raise ValueError('PublicBlob type %s incompatible with key type %s' % (blob.key_type, self.get_name())) @@ -396,36 +403,74 @@ class PKey(object): # {ssh-rsa, ssh-dss, ssh-ecdsa, ssh-ed25519}, but should # provide rudimentary support for {*-cert.v01} class PublicBlob(object): - ''' - OpenSSH plain public key or OpenSSH signed public key (certificate) - A mostly dumb container - ''' - def __init__(self, pubkey_filename=None, pubkey_string=None): - ''' - Can read from a file or string. - ''' - if pubkey_filename: - with open(pubkey_filename) as f: - fields = f.read().split(None, 2) - elif pubkey_string: - fields = pubkey_string.split(None, 2) - else: - raise ValueError('PublicBlob() requires either a pubkey_filename or pubkey_string') + """ + OpenSSH plain public key or OpenSSH signed public key (certificate). + + Tries to be as dumb as possible and barely cares about specific + per-key-type data. + + ..note:: + Most of the time you'll want to call `from_file`, `from_string` or + `from_message` for useful instantiation, the main constructor is + basically "I should be using ``attrs`` for this." + """ + def __init__(self, type_, blob, comment=None): + """ + Create a new public blob of given type and contents. + + :param str type_: Type indicator, eg ``ssh-rsa``. + :param blob: The blob bytes themselves. + :param str comment: A comment, if one was given (e.g. file-based.) + """ + self.key_type = type_ + self.key_blob = blob + self.comment = comment + + @classmethod + def from_file(cls, filename): + """ + Create a public blob from a ``-cert.pub``-style file on disk. + """ + with open(filename) as f: + string = f.read() + return cls.from_string(string) + + @classmethod + def from_string(cls, string): + """ + Create a public blob from a ``-cert.pub``-style string. + """ + fields = string.split(None, 2) if len(fields) < 2: - raise ValueError('PublicBlob() not enough fields %s', fields) - self.key_type = fields[0] - self.key_blob = decodebytes(fields[1]) + msg = "Not enough fields for public blob: {0}" + raise ValueError(msg.format(fields)) + key_type = fields[0] + key_blob = decodebytes(fields[1]) try: - self.comment = fields[2].strip() + comment = fields[2].strip() except IndexError: - self.comment = None - # Verify that the blob message first (string) field matches the key_type - m = Message(self.key_blob) + comment = None + # Verify that the blob message first (string) field matches the + # key_type + m = Message(key_blob) blob_type = m.get_string() - if blob_type != self.key_type: - raise ValueError( - 'Invalid PublicBlob contents. Key type [%s], expected [%s]' % - (blob_type, self.key_type)) + if blob_type != key_type: + msg = "Invalid PublicBlob contents: key type {0!r}, expected {1!r}" + raise ValueError(msg.format(blob_type, key_type)) + # All good? All good. + return cls(type_=key_type, blob=key_blob, comment=comment) + + @classmethod + def from_message(cls, message): + """ + Create a public blob from a network `.Message`. + + Specifically, a cert-bearing pubkey auth packet, because by definition + OpenSSH-style certificates 'are' their own network representation." + """ + type_ = message.get_text() + message.rewind() + return cls(type_=type_, blob=message.asbytes()) def __str__(self): if self.comment: diff --git a/paramiko/rsakey.py b/paramiko/rsakey.py index 7abcfa28..3f28689a 100644 --- a/paramiko/rsakey.py +++ b/paramiko/rsakey.py @@ -54,8 +54,26 @@ class RSAKey(PKey): else: if msg is None: raise SSHException('Key object may not be empty') - if msg.get_text() != 'ssh-rsa': + type_ = msg.get_text() + nonce = None + # Regular public key - nothing special to do besides the implicit + # type check. + if type_ == 'ssh-rsa': + pass + # OpenSSH-compatible certificate - store full copy as .public_blob + # (so signing works correctly) and then fast-forward past the + # nonce. + elif type_ == 'ssh-rsa-cert-v01@openssh.com': + # This seems the cleanest way to 'clone' an already-being-read + # message? + self.load_certificate(Message(msg.asbytes())) + # Read out nonce as it comes before the public numbers. + # TODO: usefully interpret it & other non-public-number fields + nonce = msg.get_string() + else: raise SSHException('Invalid key') + # Now that we've read type and (possibly) nonce, public numbers are + # next in either case. self.key = rsa.RSAPublicNumbers( e=msg.get_mpint(), n=msg.get_mpint() ).public_key(default_backend()) diff --git a/paramiko/transport.py b/paramiko/transport.py index 388f60cb..0dc2e28a 100644 --- a/paramiko/transport.py +++ b/paramiko/transport.py @@ -204,6 +204,7 @@ class Transport(threading.Thread, ClosingContextManager): _key_info = { 'ssh-rsa': RSAKey, + 'ssh-rsa-cert-v01@openssh.com': RSAKey, 'ssh-dss': DSSKey, 'ecdsa-sha2-nistp256': ECDSAKey, 'ecdsa-sha2-nistp384': ECDSAKey, diff --git a/tests/test_pkey.py b/tests/test_pkey.py index 034331a2..dac1d02b 100644 --- a/tests/test_pkey.py +++ b/tests/test_pkey.py @@ -485,7 +485,7 @@ class KeyTest(unittest.TestCase): # PKey.load_certificate key = RSAKey.from_private_key_file(test_path('test_rsa.key')) self.assertTrue(key.public_blob is None) - key.load_certificate(pubkey_filename=test_path('test_rsa.key-cert.pub')) + key.load_certificate(test_path('test_rsa.key-cert.pub')) self.assertTrue(key.public_blob is not None) self.assertEqual(key.public_blob.key_type, 'ssh-rsa-cert-v01@openssh.com') self.assertEqual(key.public_blob.comment, 'test_rsa.key.pub') @@ -502,5 +502,8 @@ class KeyTest(unittest.TestCase): # Prevented from loading certificate that doesn't match key1 = Ed25519Key.from_private_key_file(test_path('test_ed25519.key')) - self.assertRaises(ValueError, key1.load_certificate, - pubkey_filename=test_path('test_rsa.key-cert.pub')) + self.assertRaises( + ValueError, + key1.load_certificate, + test_path('test_rsa.key-cert.pub'), + ) -- cgit v1.2.1