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

client support for RSA ssh certificates #531

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@jasonrig

jasonrig commented May 26, 2015

I have extended the RSAKey class to support the ssh-rsa-cert-v01@openssh.com type of certificate authentication in a new RSACert class. A test case was added to test_pkey.py to test loading an RSA certificate (a signed public key that's the same one given in the tests already).
I think this class should be a reasonable example from which DSS and ECDSA certificate classes can also be created.

Let me know what you think!

Jason

@coveralls

This comment has been minimized.

coveralls commented May 26, 2015

Coverage Status

Coverage increased (+0.14%) to 72.68% when pulling ce44a23 on jasonrig:rsacert into 5441520 on paramiko:master.

@dimrozakis

This comment has been minimized.

dimrozakis commented Aug 11, 2016

Any updates on this one? Are there any plans to get this merged?

@bitprophet bitprophet added this to the 2.1 milestone Aug 12, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 12, 2016

Will try and take a look next feature release pass. If you can update it so it merges/works cleanly with master that would help its chances :) thanks!

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 12, 2016

Also, real-world reproduction steps for testing the functionality would be awesome, I haven't used cert-style authentication much yet.

@jasonrig

This comment has been minimized.

jasonrig commented Aug 13, 2016

Hey @bitprophet and @dimrozakis, I'll rebase on master and clean up where required. I have reasonable experience with SSH certs, so I'll provide some extra docs to help introduce the concepts and real-world steps for use. Sometime next week, I hope :)

By the way, my requirements for certs+paramiko have been exclusively RSA where paramiko is the client and not server, so this functionality is not included here, but would be awesome if someone with time and expertise could contribute it... just for completeness.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.1%) to 74.358% when pulling a5015ea on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.09%) to 74.334% when pulling 259b329 on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.1%) to 74.361% when pulling 5fe6ec2 on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.06%) to 74.306% when pulling 1eb6e78 on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.07%) to 74.32% when pulling d7cccba on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.1%) to 74.375% when pulling 0a06565 on jasonrig:rsacert into 833d9f7 on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Aug 16, 2016

Coverage Status

Coverage increased (+0.1%) to 74.388% when pulling 164c671 on jasonrig:rsacert into 833d9f7 on paramiko:master.

@jasonrig

This comment has been minimized.

jasonrig commented Aug 16, 2016

@bitprophet, I've rejigged the PR to merge cleanly with master, added a few tests and some docs (mostly references to people who write about it a lot better than I).
As for real-world reproduction, DigitalOcean have a very good article about how to set up certificate-based auth with OpenSSH. Conceptually it's quite simple:

  • set up a CA using ssh-keygen
  • modify SSH server config to trust the CA (can be done globally or per-user)
  • sign a certificate with the CA keys using ssh-keygen
  • put the cert and private key somewhere where the ssh command can find it... or use paramiko!
  • log in to remote system
@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 16, 2016

Thanks @jasonrig! Greatly appreciated. As noted, it's in the next feature release bucket so I'll walk through here when I get to that.

@dimrozakis

This comment has been minimized.

dimrozakis commented Sep 1, 2016

Hey @jasonrig , just returned from holidays, I'm thrilled to see you jumped in and updated the PR, much appreciated!

@bitprophet is there any estimation on when the next feature release will come out?

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 8, 2016

@dimrozakis I'm really bad at estimates so I need to stop giving them! I will note that I tend to work in sprints lately, so the next time I personally need to push out something feature-y I will also go down the milestone list and try to knock out as much as I can.

@typpo

This comment has been minimized.

typpo commented Feb 16, 2017

Adding my support to see this PR merged after two years. We use this kind of signed cert to manage server access across our organization without having to set up individual ssh keys on each server.

Facebook engineering recommends this sort of setup to control authorization on large scale production systems: https://code.facebook.com/posts/365787980419535/scalable-and-secure-access-with-ssh/ I expect this use case will become more common!

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.2%) to 74.35% when pulling 281215a on jasonrig:rsacert into d35b67a on paramiko:master.

4 similar comments
@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.2%) to 74.35% when pulling 281215a on jasonrig:rsacert into d35b67a on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.2%) to 74.35% when pulling 281215a on jasonrig:rsacert into d35b67a on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.2%) to 74.35% when pulling 281215a on jasonrig:rsacert into d35b67a on paramiko:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.2%) to 74.35% when pulling 281215a on jasonrig:rsacert into d35b67a on paramiko:master.

@jasonrig

This comment has been minimized.

jasonrig commented Feb 17, 2017

Rebased on master, tests still pass ^.^
I can squash the commits if the code review is favourable.

@Mletter1

This comment has been minimized.

Mletter1 commented Feb 27, 2017

I look forward to testing this out. Will this be integrated with the clients connect function or is it already integrated through transport? what would the example client connection look like using certs.

@jasonrig

This comment has been minimized.

jasonrig commented Feb 28, 2017

Hi @Mletter1
This should work in almost exactly the same way as the current key-based authentication, since it simply overrides the RSAKey class with some extra certificate-specific details. In other words, if you currently use an RSA key for authentication, the RSA certificate method should be a drop-in replacement.

My use-case has been specifically for RSA certificates, and only for Paramiko acting as a client. As mentioned in the PR, implementing other types of certificates (DSA, ECDSA, etc.) should be trivial, but I just have not had the time. As for server logic, I don't feel I have experience enough to implement this without a closer reading of the spec and Paramiko code.

@bitprophet bitprophet modified the milestones: 2.4.0, 2.2 Aug 16, 2017

@jasonrig

This comment has been minimized.

jasonrig commented Aug 17, 2017

@incognick I'm happy to take advice on whether, for example, from_private_key_file should be implemented with a sensible guess for the certificate file name as the public key. Though I'm a bit hesitant to weave RSACert into other paramiko components if only to keep the scope of the PR small enough to not delay any review further than it has already been.

I would say subsequent improvements beyond this core enhancement should be submitted as additional PRs down the track. However I do not take issue with anyone forking my repo and basing a new branch on this PR with their desired improvements, and submitting their own changes as a new PR. So if you see any room for improvement, please go ahead and implement it!

@bitprophet bitprophet modified the milestones: 2.3.0, 2.4.0 Aug 17, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 17, 2017

I'm +1 on merging as-is and figuring out integration with other cert-unaware codebases like Fabric afterwards. Moved this to the 2.3 milestone since it's thematic with some other auth-related features there like smartcard support (#827). (Actually makes me wonder whether that should be made aware of this...thoughts welcome, tho always possible to add later too.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 19, 2017

Successfully tested out OpenSSH-level certificates on my end so I understand that level of things. Also skimmed the RFC. So next up is actual review, manual testing & merging \o/

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 21, 2017

As-I-go commentary (I will address most/all while merging):

  • Not 100% sure why this inherits from RSAKey itself, seems like very little actual overlap?
    • Plus the constructor renames/reorders a bunch of the init args (unless this is an artifact of PR age but I don't think so) so it's not even really usable as a semi-drop-in replacement
    • I'm thinking I'll try to do a 2nd-draft version of this, then, with a distinct new class that simply encapsulates an RSAKey. We'll see. Need to manually test & understand the test suite changes first.
    • EDIT: After a bit more reading/thinking, it makes some sense, insofar as the intent is to supply the exact same sort of "public+private key material and operations on/via same" object to transport, etc as the existing PKey subclasses.
      • It's a bit new, insofar as the established pattern is "just load private key material, from which the public key can be mathematically derived"; now we're adding "also need the certificate data which encapsulates what is presumably the public key half of the private key", meaning two data sources instead of one.
        • also (maintainer hat on!) more fun opportunities for users and/or client libraries shooting themselves in the foot by supplying mismatched cert and private key material, or one and not the other.
        • constructor logic for these classes is already annoyingly torturous due to the "you can give the privkey material in about 3 different ways" and I'd like to change that sometime, but probably not now...
  • There seem to be unused encryption param attributes, eg self.d/p/q. (p/q could be part of DSS but that's not complete; none of the keys in the extension RFC seem to mention a d at all; it's not referenced anywhere else in the code either.)
  • Not a fault of the OP, but, starting to try to get away from thoughtless vanilla SSHException use, so we should make some subclasses for where those are used here, if possible.
    • Though spots that 100% mimic RSAKey behavior can probably stay.
    • Similarly, the not-implemented exceptions for the later methods should be NotImplementedError, not Exception (nobody should ever raise vanilla Exception, honestly! How would you catch it?!)
@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 21, 2017

@ploxiln I edited that a bunch after you emoji'd it...FYI :D

@ploxiln

This comment has been minimized.

Contributor

ploxiln commented Aug 21, 2017

I see :)

It's good that you're investigating and finding the relationship with RSAKey is a bit awkward, and there are some currently unused attributes, sooner rather than later. I clearly didn't do that :)

It might be a good idea to open a pull request with your in-progress cleaned-up merges, or just point to your branch, so you might get some eyes and help on it.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 21, 2017

Was planning to at least link the branch once it's pushed, yea. Also trying very hard not to overstep bounds here re: overall key class organization stuff...>_>


Another random side note: wondering if we want to add 'full' client side support for this in the same way OpenSSH client does; e.g. its -i and CertificateFile. Especially re: file paths, I could see folks being confused, because! Right now we take a path explicitly assuming it's "the cert" - but -i does a tiny bit of magic:

If no certificates have been explicitly specified by the CertificateFile directive, ssh will also try to load certificate information from the filename obtained by appending -cert.pub to identity filenames.

That said - not super worried because we can add that later if folks feel it's missing, and at the moment, one is either explicitly instantiating RSACert (and thus must be aware of where both key and cert file paths are) or relying on Transport._key_types (which will be dealing with already-not-file-related Message objects).

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 21, 2017

Actually, just realized that would solve the possible confusion with RSACert constructor - users forgetting to give cert_filepath - since we could have it implicitly set cert_filepath to filepath + -cert.pub if it (and cert_file_obj) are None. Worth thinking about.

Still very unhappy with this fragile constructor logic anyways, so, maybe I'll meet myself halfway and rip it up into multiple constructors. Would serve as an experiment re: the same operation on the *Key classes, without actually touching anything but a new class, and thus backwards compat.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 21, 2017

Nah. Much of the annoyance I'm having stems from the original classes too (insofar as they mutate their responsibilities depending on whether they were given private key data or not). This stuff's all due a bigger shakeup later so I will defer to then. Still need to add a bunch more tests around corner cases though.

@radssh

This comment has been minimized.

Contributor

radssh commented Aug 21, 2017

You might get a decent compromise in the bang-for-the-buck category by being intentionally ignorant about the contents of the certificate blob. On the client side, while the components that make up the certificate are mildly interesting, for use in client authentication, that is left up to the server side to make sense of; the client offers the certificate as a blob, then if the certificate could be used for authentication, it proves possession of the corresponding private key. The client does not even need to unravel the blob itself.

This is why the current Paramiko code works just dandy with certificates loaded in a ssh-agent, as long as it can get the blob contents, and sign the composited message, it's all the same on the server side. Getting the details of the certificate content, or even being able to generate new certificates would be nice to have features, but probably pales in comparison to just being able to blindly loading a blob and being able to offer it as an alternative publickey authentication attempt, if available.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

In expanding the test suite (and by extension, some functionality/exceptions) I ran into one case that has no immediately obvious answer: what to do if someone instantiates the class with a public-only key alongside the cert? (Provided that it is the "correct" public key; mismatching public key data is covered elsewhere.)

It's a little tempting to except here too, because it implies user error:

  • for one thing, certificate data includes the public numbers anyways, so it's at best a bit silly;
  • the primary use case for no-private-key-material certificate objects is that of authenticating the remote end, which only requires the certificate blob and does not require any additional key material (again in part because of the previous bullet point).
    • In most cases where Paramiko is using a cert object for this purpose, it's getting it straight out of a Message object, and so once again, this is not a situation where the constructor should really be expecting additional key kwargs.
  • This is one of the MANY reasons I want to eventually split up this class hierarchy's constructors into multi-constructors and/or multiple classes...there are just too many of these "probably never gonna happen, but when it does, it's going to waste some poor fool's entire afternoon" possibilities.

All that said, it would not technically hurt to be given the pubkey material a 'second time', so it's remotely possible someone is dealing with grandfathered-in kwarg dicts or whatever, and would be unhappy about an exception. Which is why I'm torn - it does not violate correctness.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

@radssh That's a good point, and also applies to the comment I posted after yours (didn't see yours til just now.) The public key part, which is embedded in the cert, is all that's necessary for auth as a client; my understanding above is a bit off (just re-read Transport to remind myself; probably adding some comments therein so future-me doesn't make this mistake again), private key is not required when one has a cert.

EDIT: hm, no, that can't be right either. Clearly not a good brain day today. It's fine, I'm not supposed to be an expert or anything. My original assertion was right - to successfully auth one does need private key data so one can respond to server's challenge, "prove you have the private half of this certified public key".

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

Again, I suspect eventually breaking our classes up would also make it harder for high-functioning morons like myself to get tied in knots. Handing "the authentication process" discrete certificate and private key objects, for example, would make it more obvious that the cert largely just replaces the process of distributing a known_hosts file to the remote end (yes, alongside a bunch of other mostly-authorization related stuff that we currently don't care about unpacking) and does not replace the core need of the authenticating user to prove ownership of the private key.

EDIT: On the other hand, from the perspective of the network messages involved (i.e. auth_handler.py) it's "nice" that it's all one object, because of the call the auth handler makes against the key - specifically str(), which is expected to yield a binary Message object (and is why the auth handler did not need any updates for this all to work.) But this is another case where I have to add some comments because as a rusty (or new) reader it's rather opaque that this is what's going on.

Still not suggesting we go this route today but food for thought.

@radssh

This comment has been minimized.

Contributor

radssh commented Aug 22, 2017

private key is not required when one has a cert

not quite - to be of much use at all, the client needs the private portion to do the signing (prove possession of (unlocked) private portion).

Having a cert without the private portion of the key is almost like having the public key (plus a bunch of other publicly viewable/extractable items). Not much you can do with it alone, other than regurgitate it. The cert does contain a form of embedded public key contents, but it requires some minor unraveling to get to (RSA you can get (e,n), DSS (p,q,g,y), but as discrete values, not as a ready-made Message that would fit with Paramiko), so to transform cert -> pubkey can be done, but would need a case by case handling. Generally, since you don't need to open the envelope to pass it along, just need to sign the sealed envelope; why bother with those details.

Point is, unless you really "need" the pubkey, you can skip the unraveling completely, and just handle the cert blob, as long as you can submit the whole enchilada (Message with blob, and other bits) + (Private key signed Message) to the remote side to authenticate. It's the remote side that (should) verify not only the signature of the auth request done by "your" private key, but also the unraveled blob, which itself is a signed Message by some CA, presumably identifiable by the server as trusted, according to the CA public key on file on the server side.

The worst that can happen on the client side, is that the server declines authentication using the cert, for whatever reason (untrusted CA, malformed signature, validity interval expired, principal name does not match, etc.), and the client has to try another auth method (plain public key) or just cope with the failed authentication. Yes, it would be nice to be able to elegantly detect inconsistencies in advance, but that could be construed as scope creep 😄

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

Once again, I edited my comments once I had that same realization (re: private key.) 🙃

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

More waffling - realizing that my initial read on the msg and data params is also wrong; in normal RSAKey usage, they may only be public key material, and in RSACert they are expected to be cert data. So they don't quite map along the same lines as filename and file_obj, which are expected to be private key related. (Have I mentioned yet how frustrating this API is to understand or extend? I have? Okay.)

Upshot is that I'm probably just gonna put the constructor args back the way I found them and file it under "let's just reorganize the entire friggin thing in a .0 release".

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

(At least the docstring is getting a massive workout from all this...and the test suite...)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

Noticed another oops, the key param doesn't appear to be used at all. In RSAKey it's a way of handing in an already-created key object (i.e. some Cryptography primitive) so presumably we should honor it here too.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 22, 2017

Not done but pushing what I have so far today as the 531-int branch. Think this is the correct diff view (had to construct it manually because Github wasn't wanting to show me the original branch despite it still existing on Jason's end...):

jasonrig/paramiko@rsacert...paramiko:531-intfiles_bucket

Can't promise exactly when I will pick this back up again but it shouldn't be more than a few days to a week tops. I intend to finish fleshing the tests out, the hopefully small bits of extra implementation needed to satisfy them (mostly around raising more footgun errors; and I'll be happy to skimp on some of 'em for now) and then merging.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 23, 2017

While I was supporting my dayjob, @radssh implemented what I think I'll consider a second draft of this (in #1042), with a different approach that has benefits over the one here:

  • It's type-agnostic because it just treats the cert blob as what it is, an already-formatted SSH protocol message, and so there's no need to break it down only to reconstruct it again later. Thus it should support all common cert types out of the box.
    • This has the drawback of not being useful for server-side support, for which I suspect this PR's approach may eventually be required.
  • It implements the "you give me key X? I look for cert X-cert.pub" behavior that hadn't yet been implemented here.
  • It doesn't have any of the drawbacks of the direct-subclass approach (all the constructor, NotImplementedError method, etc, annoyances) because it just tacks an extra method onto the existing classes.
  • It's less code, with fewer edge/corner cases.

Obviously I'm not super tickled at the idea of dropping or deferring something both the OP and myself put a bunch of effort into, but given that using certs to authenticate as a client feels like the much more common use case, #1042 serves that better and with less code and so it's more tempting to merge first.

@bitprophet bitprophet removed this from the 2.3.0 milestone Aug 23, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 23, 2017

Dropping this from the release milestone and I encourage all followers of this ticket to try out the #1042 functionality when it's released. Keeping this alive for now & hoping anyone who finds the #1042 code lacking can report details on their use cases here. Right now I'm expecting that will chiefly be a) server-side support and b) host-certificate validation support on the client side.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 29, 2017

On second thought (and partly because #1042 got expanded during testing & does more now!), going to close this. Any remaining missing functionality can be reported as its own ticket/s.

EDIT: to be clear: #1042 has just been merged to master and will be released in the near future. It should serve almost all client-side cert needs as well as limited server-side and sig verification!

@bitprophet bitprophet closed this Aug 29, 2017

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