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!

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

Reference: django/asgiref@6d19d45

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.

@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 😗

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. 👍

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 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

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.

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.

Relevant reference for reviewer: pgjones/hypercorn#62

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'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.

Finally got back around to this. I've addressed the merge conflicts and fixed up the tests. There is one linting error I still see but it's for a line I haven't touched so not sure what to do about that since it would see that issue should exist in master already. Any advice about what to do here would be appreciated.

As for the larger discussion as to whether or not this is a sufficient implementation of the specification, I would say it is. The fields that I am leaving set to None are allowed to be None by the spec so I would say it technically meets the requirements. Without pulling in a large library like cryptography as a dependency we simply cannot obtain that the required information. It would seem like the best route would be to leave that up to the application developers. If they desire detailed cert information the whole cert is provide in its raw form and they can parse it themselves.