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
Implement asgiref tls extension #1119
base: master
Are you sure you want to change the base?
Conversation
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 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...
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. 👍 |
uvicorn/config.py
Outdated
@@ -213,6 +213,7 @@ def __init__( | |||
self.callback_notify = callback_notify | |||
self.ssl_keyfile = ssl_keyfile | |||
self.ssl_certfile = ssl_certfile | |||
self.ssl_cert_pem: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we drop this? (And exclusively populate it from self.ssl_certfile
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
On your points,
|
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 |
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. |
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 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 |
Any updates on this PR? I'm developing a FastAPI application using mTLS and really like to replace my workaround (monkey-patching |
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? |
You may want to look at nonecorn or https://github.com/urllib3/hypercorn/tree/tls-extension. |
I'm interested. Can you show me how you made FastAPI serve optional/required mtls? |
I re-synced this PR with the current master branch. Again, this is still just waiting on a re-review from a maintainer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
for rdn in peercert["subject"]: | ||
rdn_strings.append( | ||
"+".join( | ||
[f"{RDNS_MAPPING[entry[0]]} = {entry[1]}" for entry in reversed(rdn) if entry[0] in RDNS_MAPPING] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there should be no spaces around the =
nor after the comma delimiter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After sitting down to look at this again, I'm wondering if we should just not set the client_cert_name
property. It is marked as optional in the specification and I think properly constructing it is beyond the scope of this library.
Correcting the issue you mentioned is easy, but it did get me thinking about other edge cases involving reserved characters in the RDN values. This led me to this page which describes how to handle escaping various characters that appear in the values. https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ldap/distinguished-names
IMO this is beginning to get too complicated for a quick lookup table approach and without pulling in something like the cryptography
library as a dependency which would have robust handling for this situation, I don't see a viable path forward.
I'm leaning towards just providing the bare minimum, i.e. the server cert and client cert in PEM encoded format and leave any parsing of the certificates as an exercise for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, leave it out.
ssl_info["tls_version"] = ( | ||
TLS_VERSION_MAP[ssl_object.version()] if ssl_object.version() in TLS_VERSION_MAP else None | ||
) | ||
ssl_info["cipher_suite"] = list(ssl_object.cipher()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cipher_suite
should be an integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here again lies with the specification. I would need a lookup table for every single cipher suite for TLSv1 through TLSv1.3. The RFC referenced in the specification only has the ciphers for TLSv1.3.
This is another area where the specification doesn't easily align with what is readily available via the Python TLS module.
So rather than trying to collect and maintain a list of cipher mappings I opted for just passing through the result from the underlying SSLObject. This is obviously "off-spec" however. If we want to maintain strict compatibility with the spec then in a similiar regard to the client_cert_name
we can just not set this value.
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 toSSLObject.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