cloudtools / ssh-cert-authority

An implementation of an SSH certificate authority.
BSD 2-Clause "Simplified" License
728 stars 71 forks source link

Making sure Principal is same name as requester #43

Open Arvanaghi opened 5 years ago

Arvanaghi commented 5 years ago

I'd like to add a feature to make sure a requester can't request a cert for a Principal that has a different user's name (e.g. if Alice is an AuthorizedRequester, she can't request a cert for Bob@*).

It doesn't look like this is possible currently. And it also looks like the person who requested the cert in the first place is not stored anywhere persistently.

If that's correct, it seems like I would have to do some surgery. I.e., add a field to the certRequest struct to persistently store who requested that cert in the first place.

Is this accurate? Any thoughts here?

Arvanaghi commented 5 years ago

EDIT: I finished writing the code for this locally, if you want it.

Your example uses ec2 as a Principal so it seems like it's not what you had in mind. But the issue is AuthorizedUserA can request a cert with AuthorizedUserB's name as the Principal. If it's not caught in manual review, someone will sign that cert, and it could seem like it was PersonB accessing all those internal sites.

bobveznat commented 5 years ago

Yeah, that's a pretty straightforward confused deputy problem, huh. You're right, everywhere we used this we just logged in to hosts as ubuntu or ec2 or centos (depending on the flavor of the os). And then the KeyId field (search KeyId in security.rst) is used to point back to the actual server-controlled identity of the requester. If nothing else, and this is hardly a consolation, if you have sshd logging certificate information (and you should) you'll have log messages showing that user A got a certificate and used it as user B. Which is likely grounds for termination, at least at my place of employment.

If a config option existed to put the server into a "strict" mode such that the keyid and principal needed to match I'd take a look at it. I do not think this should be the default behavior, it would break every installation of ssh-cert-authority I've ever seen.

Thanks for pointing this out and taking the time to think about it.

Arvanaghi commented 5 years ago

It may break the behavior of existing installs, but don't you agree this should be default behavior?

I have no problem with you not accepting my code on this. But at least intellectually, I want to challenge that point.

bobveznat commented 5 years ago

Maybe I'm missing something. I'm used to an environment where the developers are many and the destination account they can login to is singular (e.g. ec2-user). We could call this an n:1 environment where n identities map to 1 user account on the destination hosts.

I don't have a concern with an environment that is configured this way given that auth.log (or secure) generates a complete audit record (as detailed in security.rst).

And I see the merit or perhaps the validity of the environment you're in. If the servers you're logging into have real user accounts that tie back to an actual user identity then what you've described here is what you want. We could call this an n:n environment where n identities map to n user accounts on the destination host.

Both environments are valid and ssh-cert-authority provides value to both kinds of environment. The crux of this issue makes sense as an enhancement for the n:n style environment. If you said this project is broken in an n:n environment I might even agree with you.

But I don't think we should break every other deployment of ssh-cert-authority to fix the n:n case. It needs to be either a configurable option or implied by the configuration file somehow. Perhaps the config for an environment in sign_certd can simply say that the principals in the request must match exactly the keyid or the request is rejected.

That's my first intuition on this. My gut says there's a more intuitive (and thus harder to screw up) way to match an administrator's intent with the behavior of the system but I'm not coming up with anything good right now. I'd be happy to hear your suggestions.

fbscarel commented 4 years ago

I have the exact same use-case as @Arvanaghi stated above, i.e. a datacenter with several servers and sysadmins, and integrated LDAP auth. A "strict" mode in which the keyid and principal need to match is essential to avoid impersonation. Even though we could, in principle, check fingerprint information on logs, that's somewhat cumbersome.

I don't think it even needs to be the default behaviour of the program or anything, but the option would be certainly very much welcome.

Edit: I've made a very simple (and hacky) change to the sign_certd code that checks if KeyId and Principals match -- considering the KeyId is limited to one element, I also disallow more than one Principal on any one cert. Link follows; I'll not send a pull request as the solution is not at all elegant and I'm sure you can come up with way better stuff: https://github.com/fbscarel/ssh-cert-authority/commit/7639dc479dd2b5ba2941a0895268a6cb5a67533c