Implement asgiref tls extension

[mdgilene]

This is a partial implementation of the new TLS extension added to the ASGI spec.

This PR partially implements the new TLS extension which defines a new set of attributes to be added to the request scope under extensions.tls.

The spec defines 6 new attributes:

server_cert

PEM encoded server certificate.

client_cert_chain

List of PEM encoded certificates in the client certificate chain.

It appears at the moment only the last certificate in the chain is available from the SSLObject. So this implementation will only ever provide 1 certificate in the cert chain.

client_cert_name

DN for the first certificate present in the client_cert_chain.

Without pulling in an additional dependency to handle X509 parsing, obtain this value is a bit more difficult. I have added in a very basic lookup table to be able to obtain the DN in string format. Please if anyone else has a better way to handle this I am all ears.

client_cert_error

Error if verification of client certificate chain failed.

I do not believe the SSLObject provides this information. If it doesn't I couldn't find it anywhere. So without it we just leave this blank at all times.

tls_version

TLS version number.

The spec stats this should be the integer for the version number defined in the TLS specification. Again, I have achieved this through a simple lookup table. If there is a more standard way of obtaining this I would appreciate any help here.

cipher_suite

TLS cipher suite.

Again, the spec states This is a 16-bit unsigned integer that encodes the pair of 8-bit integers specified in the relevant RFC, in network byte order. Without direct access to openssl I don't know how to obtain this value. So for now I have simply set this to SSLObject.cipher which is off-spec but I don't know what else to do other than leave it blank.

Related Issues

#1118

Edit by @Kludex : Closes #1118

[mdgilene]

Note, I am new to contributing to this project so looking for input on this PR and if you see issues with code format/style/etc. please let me know and I will try to address it.

Also, I understand this addition to the spec is very new. If this is still blocked until a new release of asgiref that is understandable.

Thanks!

[Kludex]

If the connection is not over TLS, we shouldn't have it in the scope, right?

Reference: django/asgiref@6d19d45

[mdgilene]

I believe you're right. I missed this piece during my initial read through. I've updated it so that it's only added if the connection is made over TLS.

[Kludex]

@mdgilene would you mind rebasing it? It's already released by asgiref, so I think we can start reviewing it.

@tomchristie @euri10 I'd like this to be reviewed by you guys as well 😗

[tomchristie]

Interesting stuff, thanks.

Thoughts on stuff retrospectively, that might or might not be valuable to mention...

• It probably would have made sense for the ASGI spec addition there to also demo an implementation that meets the spec. It's a bit odd for us to try to wrangle out some information when there's not a clear indication that the info that's been added to the spec is actually available to us. That'd also help tease out if some of the design decisions in that bit of the spec made practical sense or not. (Eg.it looks a bit odd from this standpoint having to map TLS version strings into integers)
• It'd possibly be good to have a nice little simple user-focused example of "this is what we'd like to be able to do, and why. this new addition to the spec enables that". It's very easy to end up getting into cycles of adding tech for the sake of tech, without really making it clear what user-driven benefits we're attempting to provide for.

Anyways, those are just my high-level thoughts on this overall - perhaps not super useful at this point, but sometimes getting those sort of thought processes out in the open might help influence future changes?

That stuff aside, this seems to look decent. 👍

[tomchristie]

Can we drop this? (And exclusively populate it from self.ssl_certfile?)

[mdgilene]

We need to provide the server cert contents in the scope so I did this to avoid needing to read the certfile in on each connection. If that isn't a concern I can move it to the connection code. Otherwise not sure where else to put it.

Unless I am misunderstanding. ssl_certfile just contains the path to the cert on disk no? I couldn't find any existing location where it is being read in. I assumed it's just being passed down to the underlying socket which handles reading the file.

[mdgilene]

Interesting stuff, thanks.

Thoughts on stuff retrospectively, that might or might not be valuable to mention...

• It probably would have made sense for the ASGI spec addition there to also demo an implementation that meets the spec. It's a bit odd for us to try to wrangle out some information when there's not a clear indication that the info that's been added to the spec is actually available to us. That'd also help tease out if some of the design decisions in that bit of the spec made practical sense or not. (Eg.it looks a bit odd from this standpoint having to map TLS version strings into integers)
• It'd possibly be good to have a nice little simple user-focused example of "this is what we'd like to be able to do, and why. this new addition to the spec enables that". It's very easy to end up getting into cycles of adding tech for the sake of tech, without really making it clear what user-driven benefits we're attempting to provide for.

Anyways, those are just my high-level thoughts on this overall - perhaps not super useful at this point, but sometimes getting those sort of thought processes out in the open might help influence future changes?

That stuff aside, this seems to look decent. 👍

On your points,

1. Yeah I wasn't involved in the writing of the spec but a few of the choices for which fields to provide and in what format don't make sense due to limitations of python's tls module. Proper parsing of the certificate could be done but it would require pulling a dependency. Ultimately like the spec states parsing of the cert should be left up to the end user so parsing the cert for the DN only seems a bit useless. As for the TLS version number idk...honestly as a user I would probably rather just have the string version.

2. That would have been nice yes. I agree things should have a use case to support adding tech so for the sake of argument here, the primary user focused use case that comes to mind that wanting to implement user authentication based on client certificates. This could be done through implementing an OAuth2/OIDC provider service that offers a login flow with X.509 authentication. (If you are familiar with something like Keycloak, it does exactly this). In order to accomplish this though you need access to the client certificate on each connection to be able to parse it for whatever fields you are using to match against existing users.

[mdgilene]

@mdgilene would you mind rebasing it? It's already released by asgiref, so I think we can start reviewing it.

@tomchristie @euri10 I'd like this to be reviewed by you guys as well 😗

Yep, won't get to it today, but I'll try to get to it tomorrow if I can

Edit: Nevermind, turns out github has an in browser tool for rebasing. How neat :) Was thinking I'd need to pull everything down again. Rebase should be done now

[ajoino]

Interesting stuff, thanks.
Thoughts on stuff retrospectively, that might or might not be valuable to mention...

• It probably would have made sense for the ASGI spec addition there to also demo an implementation that meets the spec. It's a bit odd for us to try to wrangle out some information when there's not a clear indication that the info that's been added to the spec is actually available to us. That'd also help tease out if some of the design decisions in that bit of the spec made practical sense or not. (Eg.it looks a bit odd from this standpoint having to map TLS version strings into integers)
• It'd possibly be good to have a nice little simple user-focused example of "this is what we'd like to be able to do, and why. this new addition to the spec enables that". It's very easy to end up getting into cycles of adding tech for the sake of tech, without really making it clear what user-driven benefits we're attempting to provide for.

Anyways, those are just my high-level thoughts on this overall - perhaps not super useful at this point, but sometimes getting those sort of thought processes out in the open might help influence future changes?
That stuff aside, this seems to look decent. +1

On your points,

1. Yeah I wasn't involved in the writing of the spec but a few of the choices for which fields to provide and in what format don't make sense due to limitations of python's tls module. Proper parsing of the certificate could be done but it would require pulling a dependency. Ultimately like the spec states parsing of the cert should be left up to the end user so parsing the cert for the DN only seems a bit useless. As for the TLS version number idk...honestly as a user I would probably rather just have the string version.

2. That would have been nice yes. I agree things should have a use case to support adding tech so for the sake of argument here, the primary user focused use case that comes to mind that wanting to implement user authentication based on client certificates. This could be done through implementing an OAuth2/OIDC provider service that offers a login flow with X.509 authentication. (If you are familiar with something like Keycloak, it does exactly this). In order to accomplish this though you need access to the client certificate on each connection to be able to parse it for whatever fields you are using to match against existing users.

I have a use case involving JWTs for authorization and authentication. I don't remember all details from the top of my head, but I think it was a JSON encrypted by the "server's" (it's a library for a SOA architecture) public key put into another JSON encrypted by the client's private key. To get the information in the JWT I need the client cert, which currently is unavailable unless running behind nginx or similar.

If you want help I could try to write an example based on my use-case.

EDIT: I remember the use case a bit more now. A service consumer, call it A, got a JWT from a central authorization system, which contained authorization information for a producer, called B. The JWT was signed by A's private key and the authorization system's public key. When A tries to consume a service of B, B must decrypt the JWT, which requires it to get A's public key. The way I tried to implement it was to write a middleware but since I couldn't get the cert from the transport I gave up and worked on other features.

[amirkarimi]

Since this branch was behind and some tests were failing I rebased it and fixed the issues in my fork. Here is the compare. @mdgilene feel free to merge my fork into this PR. If you don't get the time let me know and I can open a new PR.

[Kludex]

Relevant reference for reviewer: pgjones/hypercorn#62

[mdgilene]

I'd be happy to look at this again as I've been out of the loop on this for a while. But from what it seems like is that we are potentially just waiting for additional updates/modifications to the spec as currently I do not believe the spec to be fully implementable.

[amirkarimi]

I'd be happy to look at this again as I've been out of the loop on this for a while. But from what it seems like is that we are potentially just waiting for additional updates/modifications to the spec as currently I do not believe the spec to be fully implementable.

I might have missed something but my impression from the last comment on pgjones/hypercorn#62 is that the spec can be implemented.

[jschlyter]

Any updates on this PR? I'm developing a FastAPI application using mTLS and really like to replace my workaround (monkey-patching uvicorn.protocols.http.httptools_impl.HttpToolsProtocol).

[pugo]

I think this PR is exactly what I would need to differentiate our clients that use mTLS to connect.

Is there anything I can do to help with this one?

[commonism]

You may want to look at nonecorn or https://github.com/urllib3/hypercorn/tree/tls-extension.
I use nonecorn.

[commonism]

I'm developing a FastAPI application using mTLS

I'm interested.
I use nonecorn, and (have to?) mangle the FastAPI description document to include mtls and extend the security for the Operation to allow or require mtls. It's part of my unit tests.
https://github.com/commonism/aiopenapi3/blob/master/tests/tls_test.py

Can you show me how you made FastAPI serve optional/required mtls?

[mdgilene]

I re-synced this PR with the current master branch. Again, this is still just waiting on a re-review from a maintainer.

@Kludex @tomchristie

[jschlyter]

I've tested this PR using my own application and it works as expected with some minor nits (see comments). After these are fixed, I support this PR.

[mdgilene]

I think this PR is at the point where I have no other additional changes and I believe it adequately implements the TLS extension as it is currently written. Down the line I would probably like parts of the spec re-visited as they don't make much sense within what is readily available from the native python SSL module however that is outside the scope of this PR.

At this point just looking for a maintainer to sign off.

[pugo]

This is great work!

I have been using this branch for some days now during development. I haven't stressed any corner cases, but it does what I expect. With this I can read the client certificate to extract any extra data I have added to it.

This allows us to adjust config and behavior depending on certificate info, without need to add user and session management.

[Kludex]

@synodriver We didn't talk much yet, but I think you are the only one who implemented this extension. Would you like to give a first review on this PR?

[synodriver]

@synodriver We didn't talk much yet, but I think you are the only one who implemented this extension. Would you like to give a first review on this PR?

Sorry for the late reply, I think this is definitely the first implementation and I just migrated it to hypercorn, hope it works as expected. (I am not very familiar with ssl). According to the feedback from users who are already use my fork, it should be fine... Or maybe we can release a beta version for this feature and wait for feedback from uvicron users?

[lschneidpro]

What is the ETA ? Looking forward to use the tsl extension

[Kludex]

People here, can you contribute to django/asgiref#466, please?