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

Generic certificate support #1042

Merged
merged 4 commits into from Aug 29, 2017

Conversation

Projects
None yet
3 participants
@radssh
Contributor

radssh commented Aug 22, 2017

Roll agnostic certificate support into PKey, and tweak publickey
authentication to use it only if set. Requires explicit call to
PKey.load_certificate() in order to alter the authentication behavior.

I know you just sunk a good bit of time/effort into a different PR, but this might be worth considering.

radssh added some commits Aug 22, 2017

Generic certificate support
Roll agnostic certificate support into PKey, and tweak publickey
authentication to use it only if set. Requires explicit call to
PKey.load_certificate() in order to alter the authentication behavior.
amendment
Forgot about AgentKey, and put ECDSA line in wrong __init__. That’s
what I get for only screening with test_pkey…
'Trying discovered key %s in %s' % (
hexlify(key.get_fingerprint()), filename))
if filename.endswith('-cert.pub'):
key = pkey_class.from_private_key_file(filename.rstrip('-cert.pub'), password)

This comment has been minimized.

@ploxiln

ploxiln Aug 22, 2017

Contributor

rstrip() is not what you want to use here - the argument is not a suffix, it is a set of characters.

>>> "abc-cert.pub".rstrip('-cert.pub')
'a'

instead you could do something like filename[:-len("-cert.pub")]

This comment has been minimized.

@radssh

radssh Aug 22, 2017

Contributor

Good eye - it happens to work with the basic key type names that end either with 'a' or '9', but that doesn't mean that it's right... I'll update shortly...

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 23, 2017

Very interesting approach! I really like this.

If I understand correctly (#531 is proof that I am not truly capable of intelligent thought) you're relying on how certificate files are stored as wire format on-disk, and thus can be slapped into a being-constructed auth Message as-is without any actual requirement to peek 'inside' the blob (and therein lies the agnosticity. Agnosticism? Agnosticism.)

I think I'd be happy to merge this as-is, though noting for the future that these would be nice add-ons later:

  • support for truly arbitrary certificate filenames; seems even if one explicitly adds a cert file to key_filenames, it's only treated as a cert if it conforms to the -cert.pub convention.
  • reinstating a non-agnostic certificate class as in #531, for advanced users (and/or server implementations) who actually do want the ability to introspect a certificate blob for its individual fields
@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 23, 2017

(Trying to get out of the edit-button habit...) actually, re: explicit cert names, one could still manually rely on my_private_key_obj.load_certificate(args), so that part is fine. Hooray.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Final pass on this now. One note from rereading/documenting (also going to manually test): the bits in Client that load up -cert.pub files currently appends to the list after the 'primary' private key whose name they're based on; shouldn't they instead replace that entry?

Otherwise we'll end up trying the certificate-less variant of the key before the certificate-bearing one, which would cause an auth failure in most situations where one is using a cert. (Yes, presently the intent is that one would 'fall through' that failure into a possible success with the subsequent cert, but given how messy #387 is already, I dunno if it's wise to complicate things if there's no reason.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Also wondering how best to handle OpenSSH compatibility here:

  • As-is, this code will load the cert files when it finds default key files in ~/.ssh, which is the same as OpenSSH. That's fine.
  • However, the equivalent of ssh -i (SSHClient.connect(key_filename=['some_key'])) seems a bit backwards right now - the values in key_filename are examined for the cert suffix and honored, instead of testing whether the suffixed version of each value in key_filename exists (which is the OpenSSH behavior - you say ssh -i key_file and it will look for key_file plus key_file-cert.pub.)
  • So at minimum I'd like to add that behavior so it matches the default-keys bits and OpenSSH.
  • I also wonder whether we should preserve that "inverse" behavior (if key_filename= is given a cert path, it is able to load the matching private key) or replace it.
    • It's always nice to add more functionality instead of blindly/exactly copying OpenSSH, but at the same time, more code is more bugs, and I could see users who're used to OpenSSH specifically getting a bit confused.
    • Probably gonna keep it for now (especially since I'm refactoring these key-loading chunks a bit) but those are my thoughts.

Also, if it was not clear, I have a branch open and am actively working on this right now.

@radssh

This comment has been minimized.

Contributor

radssh commented Aug 28, 2017

Looking at the debug output from OpenSSH client (7.4p1) when relying on standard named keys from ~/.ssh (which I'm presuming look_for_keys is trying to mimic), it tries the non-cert keys first, then attempts certificates:

debug1: SSH2_MSG_SERVICE_ACCEPT received
debug1: Authentications that can continue: publickey,password,keyboard-interactive
debug1: Next authentication method: publickey
debug1: Offering RSA public key: /Users/paul/.ssh/id_rsa
debug1: Authentications that can continue: publickey,password,keyboard-interactive
debug1: Offering ED25519 public key: /Users/paul/.ssh/id_ed25519
debug1: Authentications that can continue: publickey,password,keyboard-interactive
debug1: Offering RSA-CERT public key: /Users/paul/.ssh/id_rsa
debug1: Server accepts key: pkalg ssh-rsa-cert-v01@openssh.com blen 1107

Explicitly specifying CertificateFile or even IdentityFile=~/.ssh/id_rsa-cert.pub will force the certificate (only) to be used, but that's some fine grained control that look_for_keys doesn't provide. Also, when loading keys into ssh-agent, the current behavior is to load the "plain" public key entity first, followed by the certificate, so it seems the existing prioritization closely matches OpenSSH.

When explicitly loading keys, once load_certificate is called, the key explicitly will only make use of the certificate for authentication, not the plain public key, avoiding the ambiguity that look_for_keys opens up.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Took entirely too long to figure out why I'm getting No existing session errors with the (yet-untouched, I've new tests but they are commented out rn) test suite after updating the implementation to perform either-or cert loading as described above.

Realized, duh, the test server implementation used in the suite is probably barfing on the cert part of the auth messages. Prior to my change we only tried loading the cert blob if it was explicitly given.

Huh...yup. The root problem seems to be the lack of server-side support - an in-process server is attempting to field e.g. the existing tests that field various cert types, and now that SSHClient loads cert info, we're triggering the parts of the patch that update Transport so it sends a cert-capable message.

So it ends up asking the server to deal with a key of type ssh-rsa-cert-v01@openssh.com and as that's not in Transport._key_info, we get a KeyError which ends up making the Client think that the key was bad. (this took longer than it should have to figure out, in part because of a lovely bare except buried in there since ancient times...UGH.)


tl;dr we do need at least light server support if we're going to test this functionality end-to-end (the PR's tests only test the basic PKey method, not any of the Client or Transport code), so I may need to pull down more of #531 than originally intended. Grump. This always happens...it's never, ever, ever, ever as simple as the initial works-for-me implementation... 😠 (not 😠 at @radssh just at how this always ends up being the case 😭 )

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Seems I can at least get the existing tests happy enough by tweaking RSAKey so it's capable of reading a cert-bearing Message:

  • update the checking of the 1st field, 'type', so it a) accepts cert type and b) branches
  • conditionally read the subsequent 'nonce' field iff it's a cert
  • the next two fields are the public numbers in either case
  • after that is all the cert-specific stuff that we can, for now, ignore in both cases (the rest of the Message is simply not read, period.)
    • That part can be filled in later when someone wants "real" server-side support; for now it ought to behave like the user submitted a non-cert-bearing pubkey auth message.
@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Hah, not quite, amusingly that earlier test never actually instructs the server to accept RSA (it's a series of "submit some keys, accept some other possibly-overlapping list of keys" tests). So we were triggering early code (creation of key object from network message) but not later code (verification of key material), since code in the middle (key type acceptance) was (correctly, for that earlier test) bailing out.

Now a later, simpler test that just goes "does this basic RSA key work?" dies at signature verification. I have to refresh my memory of exactly why this is; IIRC it's because the client-side key object instance knows to include its certificate data when generating the signature, but the server-side object has never had load_certificate called on itself and will thus only expect a signature matching the base key data. (this may, again, be indicative of needing the key classes to fully understand certificate structure.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 28, 2017

Yea, that's the issue, so for this to really work we do need PKey classes to instantiate w/ public blob set if created from a Message object that is of cert type. Shouldn't be super hard still? Famous last words.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

Refactored the shit out of PublicBlob and PKey.load_certificate while I had the patient anesthetized, including granting load_certificate some Message-originating powers; then used that in RSAKey (since receiving the remote end's key data always involves RSAKey(message)) to correctly update public blob data when certs are in play - allowing for correct signature verification, even though the cert blob is not fully unpacked or otherwise interpreted.

End result seems to be a passing test suite! But now I need to fill in those new tests I added. I think most of the actual functionality is already there so they may just-pass... 🤞

EDIT: also need to update the other key classes. Going by the spec, they all look the same for the first few fields, so I may be able to just factor it out back into PKey.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

Hrm. When I get to testing ECDSA certs, there's a perplexing mismatch going on between what I expect the type field to be (going by the spec here) and what it apparently actually is.

Re: the key and cert in question:

  • Created a temporary public file from the public base64 of tests/test_ecdsa_256.key, whose leading key-type field is ecdsa-sha2-nistp256 (i.e. the same key type string that the rest of our and presumably OpenSSH's uses)
  • Created a certificate from it using (macOS 10.12 standard install OpenSSH 7.4p1) ssh-keygen -s ca_key -I user_test -n test tests/test_ecdsa_256.key.pub (where ca_key is a freshly generated throwaway RSA key to serve as the "CA" key in this case)
  • Confirmed the type field in the resulting cert is ecdsa-sha2-nistp256-v01@openssh.com

So far so good. Updated our code so that ecdsa-sha2-nistp256-v01@openssh.com maps to ECDSAKey. Also so far so good, it's running.

Unfortunately...load_certificate gets pissy here because the internal (inside the base64 message blob within the cert file) key type is, not ecdsa-sha2-nistp256-v01@openssh.com, but instead, ecdsa-sha2-nistp256-cert-v01@openssh.com (notice the extra -cert- in the middle!)

Test output (running temporarily under pytest for its test selection & inlined exception superpowers):

# Verify that the blob message first (string) field matches the
# key_type
m = Message(key_blob)
blob_type = m.get_string()
if blob_type != key_type:
    msg = "Invalid PublicBlob contents: key type={0!r}, but blob type={1!r}" # noqa
>   raise ValueError(msg.format(key_type, blob_type))
E   ValueError: Invalid PublicBlob contents: key type='ecdsa-sha2-nistp256-v01@openssh.com', but blob type='ecdsa-sha2-nistp256-cert-v01@openssh.com'

Now, at first I thought I'd screwed up because all the other cert types in play do have -cert- in the middle...but no...the spec clearly states that ECDSA got to be special for some reason and lacks it.

And the "blob" in play here is the one generated by ssh-keygen, not by me. I have never typed out ecdsa-sha2-nistp256-cert-v01@openssh.com so far today, nor does it appear in the repo (specifically, even grepping the entire thing for -cert fails to find it, implying I am not crazy.)

So to me this seems to be a mismatch between the published spec and the actual behavior of ssh-keygen. If it is, it may still be extant, since 7.4 is pretty recent for SSH and I see no entries about this issue in their release notes.

EDIT: and on further digging I realize that even the spec itself disagrees internally! The -cert- version of the identifiers appears in the opening/header, while the actual spec bits lack -cert-.

Anyway, the obvious empirical solution is to look at the actual source and sure enough, it's got to be a typo in the spec, the constant definition & their own test data clearly has -cert-, consistent with the other types.

Thanks a lot for wasting an hour or two of my day, whoever wrote that document 😠

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

Then wasted a smaller but still nonzero amount of time wondering why Ed25519 certs didn't work...until I noticed the patch added an explicit .public_blob = None in the end of the constructor. So even though I was calling my blob-setting subroutine earlier...'twas getting overwrought. Boooo.

But! Now all the damned tests pass under Python 2.

Sadly...Python 3 has issues. Ones I think predate all my meddling. I'll tackle that tomorrow, unless @radssh wants to give it a shot. I'm pushing my branch now.

bitprophet added a commit that referenced this pull request Aug 29, 2017

bitprophet added a commit that referenced this pull request Aug 29, 2017

Stub tests and partly-working implementation of 'load certs found alo…
…ngside key_filenames' behavior re #1042

This actually breaks existing tests due to test server not supporting
certs...bah

bitprophet added a commit that referenced this pull request Aug 29, 2017

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

bitprophet added a commit that referenced this pull request Aug 29, 2017

@bitprophet

This comment has been minimized.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

Seems like the main issue is just stuffing strings-not-bytes into base64 so it's probably a trivial fix applying b.

Actually...I may as well just try that real quick.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

Yea that was pretty easy.

bitprophet added a commit that referenced this pull request Aug 29, 2017

@bitprophet bitprophet merged commit 8864bdc into paramiko:master Aug 29, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018

Stub tests and partly-working implementation of 'load certs found alo…
…ngside key_filenames' behavior re paramiko#1042

This actually breaks existing tests due to test server not supporting
certs...bah

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018

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 paramiko#1042

dkhapun pushed a commit to cyberx-labs/paramiko that referenced this pull request Jun 7, 2018

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