Skip to content
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

Issue client cert #35

Closed
webknjaz opened this issue Dec 28, 2018 · 20 comments
Closed

Issue client cert #35

webknjaz opened this issue Dec 28, 2018 · 20 comments

Comments

@webknjaz
Copy link
Member

Hi,

It'd be nice to have a public API for creating client certificates as well. Just like issue_server_cert() but issue_client_cert().

It's useful when testing client cert TLS auth in frameworks.

@njsmith
Copy link
Member

njsmith commented Dec 28, 2018

That would be useful! The only problem is that I've never used client certs myself though, so I'm not very clear on what exactly a good fake client cert would look like :-).

Do you have an example of the kind of client cert you use?

(See also #33)

@webknjaz
Copy link
Member Author

@dpeschman submitted a number of example certificates to Cheroot a while back: https://github.com/cherrypy/cheroot/tree/cbb2b2e/cheroot/test/ssl. But I don't know how he created them.

Info on the Internet looks no different from server cert creation either: https://kb.op5.com/pages/viewpage.action?pageId=19073746

Googling has also lead me to https://github.com/python-hyper/pep543/blob/a7c5089/test/conftest.py#L365-L375 which, at glance, suggests that client cert is built the same way as server cert I guess.

@njsmith
Copy link
Member

njsmith commented Dec 29, 2018

Yeah, I just read a bit more on this, and I can't find any evidence that client and server certs are actually different in any meaningful way. For example, those cheroot test certs appear to be totally normal server certs with associated hostnames and everything.

So, the good news is that trustme already supports issuing client certs :-). Just call ca.issue_server_cert and then use it for a client! No-one can stop you!

But this is kind of confusing. So I guess we should rename that to ca.issue_cert, and ideally also support non-hostname-based identities, like email addresses (#33).

@njsmith
Copy link
Member

njsmith commented Dec 29, 2018

@webknjaz any chance you'd like to review #36?

@pquentin
Copy link
Member

Oh, I did not see the request for review here. Anyway, the PR looked good, so I merged it. Thanks!

@webknjaz
Copy link
Member Author

webknjaz commented Dec 29, 2018

Oh.. I was sleeping :) Different time zone. But I tried using the server cert (before your PR) in our tests and they've failed. I'll try out your changes now.

@webknjaz
Copy link
Member Author

@njsmith
So I've tried out requests.get(cert=...), this param accepts either a tuple (client cert + client key) or a single file with those (paths only).
Ref: http://docs.python-requests.org/en/master/user/advanced/#client-side-certificates

It worked with cert.private_key_and_cert_chain_pem.tempfile() as path: after I remembered to configure trust.

This change probably additionally needs tests + docs + deprecation warning.

@njsmith
Copy link
Member

njsmith commented Dec 29, 2018

So just to confirm, this is working for you now without further changes?

I guess we could issue a deprecation warning when people use the old name. I was lazy about it because the maintenance burden for keeping the old alias is basically nil, and the benefits of switching for end users are nil, so why hassle people about it.

For docs and tests though, I think I already updated those – is there something you noticed as missing?

@webknjaz
Copy link
Member Author

It works but I'll need to test a few more things in our test suite.

I think docs might benefit from a tutorial-like example of client cert auth with requests or similar lib.

Even though I'm somewhat familiar with TLS I still had to read the source code in order to understand how to integrate trustme. More clear docs is always good :)

@webknjaz
Copy link
Member Author

Okay, it looks like you use both ExtendedKeyUsageOID.CLIENT_AUTH and ExtendedKeyUsageOID.SERVER_AUTH so that certificates fit in both client and server: https://github.com/python-trio/trustme/blob/master/trustme/__init__.py#L232-L233

Ref: https://cryptography.io/en/latest/x509/reference/#cryptography.x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH

@webknjaz
Copy link
Member Author

I've also tested creating a cert with non_localhost in identity field and it failed somewhere inside of idna. For client certs it shouldn't be validated tho. I worked around this by monkey-patching that function but this is something which should just work.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 30, 2018

Oh.. And my testing needs me to create an invalid certificate

As in https://github.com/cherrypy/cheroot/blob/cbb2b2e771b35a91d11c80e00d667faa267ef27d/cheroot/test/ssl/client_wrong_ca.cert which doesn't have locality in the issuer field as opposed to all other certs in that folder (Issuer: C = US, ST = XX, L = XXX, O = CherryPy):

$ openssl x509 -in cheroot/test/ssl/client_wrong_ca.cert -noout -text               
Certificate:
    Data:
        Version: 1 (0x0)
        Serial Number: 2 (0x2)
    Signature Algorithm: sha1WithRSAEncryption
        Issuer: C = US, ST = XX, O = CherryPy
        Validity
            Not Before: Oct  4 19:11:36 2010 GMT
            Not After : Oct  1 19:11:36 2020 GMT
        Subject: C = US, ST = XX, O = CherryPy, CN = localhost
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (4096 bit)
                Modulus:
[...snip...]

@njsmith is this something that trustme should solve or is it out of scope?

I mean, how about some public API like create_malformed_cert_with(L=None)?

@njsmith
Copy link
Member

njsmith commented Dec 30, 2018

I think docs might benefit from a tutorial-like example of client cert auth with requests or similar lib.

That would be great, yeah. I guess the requests part is pretty trivial (now that you figured out how to do it!), but for an example we'd also need to show the server side. Do you have any ideas how to do that in a minimal tutorial-style way? From a quick search it doesn't look like flask/werkzeug has any standard way to handle client auth. Maybe we could hack together something good enough for an example using http.server?

I've also tested creating a cert with non_localhost in identity field and it failed somewhere inside of idna

That's because non_localhost isn't a well-formed hostname – hostnames can't contain underscores – and trustme assumes that if you use a random string as an identity then you want it to be a domain name. If you really really want to make a cert with the deprecated common-name field set to "non_localhost" then see #34/#37, but for your purpose I think the simplest thing would be to just switch to a well-formed hostname, like non-localhost.

doesn't have locality

I think this is a red herring? AFAIK locality isn't a required field, and in fact trustme certs never contain a locality field in its names.

Based on the name and looking at the cert, I think the point of that cert is that it's signed by a different CA than the rest of your test certs, and that the CA it uses isn't trusted. So to reproduce it with trustme you can just create a second CA, and then use that CA to issue the cert:

ca_untrusted = trustme.CA()
client_wrong_ca = ca_untrusted.issue_cert(...)

@njsmith
Copy link
Member

njsmith commented Dec 30, 2018

Okay, it looks like you use both ExtendedKeyUsageOID.CLIENT_AUTH and ExtendedKeyUsageOID.SERVER_AUTH so that certificates fit in both client and server: https://github.com/python-trio/trustme/blob/master/trustme/__init__.py#L232-L233

Oh right, of course! I think I copied that from some example and then forgot about it. That does explain things.

I guess this is fine. Maybe at some point someone will need to configure that and we'll add more kwargs or something, but it seems like a reasonable default and we can wait for that to happen before worrying about it.

@webknjaz
Copy link
Member Author

From a quick search it doesn't look like flask/werkzeug has any standard way to handle client auth.

So the main reason I started messing around with client certs is that I'm a maintainer of CherryPy. And we have Cheroot now which is HTTP+WSGI server part factored out of CherryPy. A contributor implemented client auth a while back and submitted tests with static cert and key files.
I've been trying out to refactor that. Currently, I have a test which uses trustme to configure both server context and generate client certs.
It should be usable for such a tutorial. If not, it should be as easy as tls_context.verify_mode = ssl.CERT_REQUIRED on server-side context object.

a cert with the deprecated common-name

I don't think it's deprecated for client certs. It looks like you should be able to put any text as identity info: this example https://github.com/python-hyper/pep543/blob/a7c5089/test/conftest.py#L365 puts "PEP 543 Client Certificate" (any string) for a client cert in place where it puts "localhost" (FQDN) for a server cert.

Client cert with not_localhost identity is valid. So it's more like a bug to me.

just create a second CA

Yes, you're right. I came to the same conclusion after all and it worked :)

@webknjaz
Copy link
Member Author

@njsmith would #38 work for you?

@njsmith
Copy link
Member

njsmith commented Dec 30, 2018

I don't think it's deprecated for client certs. It looks like you should be able to put any text as identity info: this example https://github.com/python-hyper/pep543/blob/a7c5089/test/conftest.py#L365 puts "PEP 543 Client Certificate" (any string) for a client cert in place where it puts "localhost" (FQDN) for a server cert.

Well, it's a bit complicated :-).

It's not that trustme is explicitly trying to stop you from sticking "not_localhost" in a client cert. It's a side-effect of some other decisions.

The central issue is that x.509 certs have two totally different places where you can put the identity information: there's a free-form string called "common name" (CN), which traditionally used to be where people put hostnames, but really you can put anything there (including "PEP 543 Client Certificate"). And, there's a structured set of fields called "subject alternative name" (SAN), which is the new place to put hostnames and such. SAN is way better than CN, because it allows multiple names in the same cert, and it's just better defined – e.g. names are explicitly tagged to be hostname vs. ip vs. email address vs. whatever, and if a CA signs a cert with a given hostname in the SAN, that explicitly means "whoever owns this cert is the valid owner of this hostname", while technically all a CA is promising if it signs a cert with a CN field is that whatever string is in that CN field is somehow, someway related to the owner of the cert; maybe they own that hostname, maybe not, no-one knows. You can see how this bothers the security people. And at this point, major software like Chrome has started ignoring the CN field entirely, and you should expect other packages to follow over the next few years.

So anyway, because of all this, trustme uses the SAN field by default, rather than the CN field. This isn't some judgement on whether it's OK to have a client cert with "not_localhost" in the name or anything; it's just that the SAN field is generally what people want, especially if they're trying to test that their software can properly handle modern certificates.

But, since the SAN field is more strictly defined and explicitly typed than the CN field was, the side-effect of using SAN is that we end up being unable to support "not_localhost" as a plain identity passed into issue_cert().

Your old certs got away with this, because they were just using CN for everything. This isn't good practice (and will probably stop working entirely at some point), but it's very common if you look at tutorials for making test certs, because most people writing these tutorials aren't experts in x.509, and the openssl command-line tool makes it very annoying to use SAN properly.

So uh.... yeah, aren't you glad you asked? x.509 is super annoying.

Anyway the bottom line is:

  • If you want to write a test for what happens when you get a cert with an unexpected hostname, you should use a syntactically valid hostname, like not-localhost or something, and everything should work fine, and you can forget all this nonsense about CNs and SANs
  • If you specifically want to write a test for CN handling, then that's what Add support for specifying common names #37's for.

@webknjaz
Copy link
Member Author

@njsmith Thanks for such in-depth explanation! I think I'd want to test both cases after all. I'm trying to come out with more test cased because historically we've got SSL support half-broken and I'm trying to fix that. So I'll wait for #37 to do that.

@webknjaz
Copy link
Member Author

I'm trying to google some mentions of CN being discouraged for client certs but no doc explicitly points to it, only server certs.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 30, 2018

@njsmith I felt like client certs use-case is a bit different from server cert. Why can't I put some JSON string to CN there, for example? It never gets checked by the SSL itself. So I kept digging.

Now, I've found a documented use-case for CN which turned out to be the original case of why @dpeschman submitted client-side auth to us. It's LDAP.

Here's RFCs giving some examples:

And SO answer:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants