asgiref icon indicating copy to clipboard operation
asgiref copied to clipboard

ASGI TLS extension

Open Kludex opened this issue 1 year ago • 18 comments

There's a PR on uvicorn to add this extension: https://github.com/encode/uvicorn/pull/1119

And I have a question... The author said we either need to maintain the list that is maintained by IANA, or use cryptography as dependency.

On the spec, this is written:

client_cert_name duplicates information that is also available in client_cert_chain. However, many ASGI applications will probably find that information is sufficient for their application - it provides a simple string that identifies the user. It is simpler to use than parsing the x509 certificate. For the server, this information is readily available.

There are theoretical interoperability problems with client_cert_name, since it depends on a list of object ID names that is maintained by IANA and theoretically can change. In practice, this is not a real problem, since the object IDs that are actually used in certificates have not changed in many years. So in practice it will be fine.

As a maintainer of uvicorn, I don't really want to generate the list as the PR is doing, neither add cryptography as dependency. Suggestions? Was this spec created by theory, or it was actually implemented when it was written?

Kludex avatar Aug 18 '24 07:08 Kludex

The spec was contributed by someone else so I'm afraid I can't speak to the intent, nor did I implement it.

What's the specific technical problem - client_cert_name can't be generated without a list of IDs from some external source?

andrewgodwin avatar Aug 18 '24 13:08 andrewgodwin

What's the specific technical problem - client_cert_name can't be generated without a list of IDs from some external source?

Yes. It looks like I can use cryptography package to help here, but is the spec forcing me to?

Just to confirm, is my understanding correct @synodriver? (they implemented this extension on nonecorn)

Kludex avatar Aug 18 '24 14:08 Kludex

A similar issue with this extension:

The extension specifies that the cipher_suite should be a 16-bit unsigned integer that encodes the pair of 8-bit integers specified in the relevant RFC, in network byte order. This is information that is available in the TLS stack (e.g., OpenSSL), but is not exposed. To resolve this one has to down the Transport Layer Security (TLS) Parameters from IANA and generate the list of name to integer, or use a library that can provide this mapping. Also, this integer means nothing to developers nor users, so to use one has to convert it back to a cipher name.

IMHO, it would be a lot simpler if the cipher_suite was just a string with the SSL object cipher name (e.g., TLS_AES_256_GCM_SHA384).

jschlyter avatar Aug 18 '24 15:08 jschlyter

For the client_cert_name I would opt for just dropping it for the reasons above.

jschlyter avatar Aug 18 '24 15:08 jschlyter

Author of https://github.com/encode/uvicorn/pull/1119 here.

When implementing the specification as outlined by this project I encountered several issues:

  1. client_cert_chain - Spec calls for all certs in the chain, however the Python SSL module only provides the last.
  2. client_cert_name - X509 DN formatted string is again not available from the Python SSL module. Parsing of the content contained in the SSLObject is required an the DN string must be carefully constructed according to RFC 4514. I did so using a relatively simplistic approach but now Uvicorn is subject to changes with this specification.
  3. client_cert_error - SSLObject provides no information on certificate errors.
  4. tls_version - This is provided by the Python SSL module however again it is not in the format specified by the ASGI extension thus manual effort to translate this to the required format is needed. Again, this now requires Uvicorn to ensure proper implementation of more external specifications.
  5. cipher_suite - Same story as tls_version not in the right format, requires translation.

Are any of the above technically impossible? No. Are they reasonable? In my opinion no.

It is my opinion that the ASGI TLS spec should function as a thin wrapper on the Python standard library SSL module. Although I do see the intention in making the formats for these fields as generic as possible, in reality the only way to accomplish this is to reinvent the wheel, requiring the implementation of several other TLS related specifications or to pull in a 3rd party dependency which has already done that.

If I were to have my way, I would trim the ASGI TLS spec down to only a few fields. Those fields would contain the DER/PEM encoded certificate/certificate-chain as available from the Python SSL module. ANY AND ALL further extraction of information such as DN, TLS version, Cipher Suite, etc. should be left as an exercise for the end user application to implement as needed. Essentially all we should be doing with this spec is making a limited set of SSLObject properties available via the request scope so applications can leverage it.

mdgilene avatar Aug 18 '24 15:08 mdgilene

I'm happy to consider PRs to modify the TLS spec - obviously we'd have to work out who has implemented it as-is, since making fields optional for servers to provide is technically backwards-incompatible.

andrewgodwin avatar Aug 19 '24 18:08 andrewgodwin

I think we only have nonecorn, and... I think @pquentin also forked hypercorn to add this extension? I'm not sure...

Kludex avatar Aug 20 '24 11:08 Kludex

@Kludex I have not implemented the full extension, only client_cert_name and alpn_protocol, because that's what I needed in urllib3 tests: https://github.com/urllib3/hypercorn/commit/3dd81376b98b337dfb64ee00a50791066149821e. I agree that the current spec isn't easy to implement fully and requires potentially CPU-intensive calls.

@mdgilene Regarding certificate chains, note that it's possible with an undocumented API with Python 3.10+ (https://sethmlarson.dev/experimental-python-3.10-apis-and-trust-stores) and there will be an official way to do it in Python 3.13: https://github.com/python/cpython/issues/109109

pquentin avatar Aug 20 '24 18:08 pquentin

I've implemented the ASGI TLS Extension partially for hypercorn (see https://github.com/pgjones/hypercorn/issues/62#issuecomment-2574715508) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

grydz avatar Jan 10 '25 14:01 grydz

As a developer using the extension (and with a lot of experience from the Python [cryptography](https://cryptography.io/) library) I fully agree with @grydz on all points.

jschlyter avatar Jan 12 '25 11:01 jschlyter

I've implemented the ASGI TLS Extension partially for hypercorn (see pgjones/hypercorn#62 (comment)) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

Here is how the specification could look like with the changes above: https://github.com/grydz/asgiref/compare/main...fix-asgi-tls-ext.

grydz avatar Jan 16 '25 07:01 grydz

I've implemented the ASGI TLS Extension partially for hypercorn (see pgjones/hypercorn#62 (comment)) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

Here is how the specification could look like with the changes above: https://github.com/grydz/asgiref/compare/main...fix-asgi-tls-ext.

That would be easier to implement. 👍

Kludex avatar Jan 16 '25 08:01 Kludex

I never really had strong opinions on the TLS extension - it was mostly drafted by others - so happy to accept a PR for a change if it has some justification and a bit of research about what servers, if any, implement each feature of the current one

andrewgodwin avatar Jan 23 '25 23:01 andrewgodwin

I've implemented the ASGI TLS Extension partially for hypercorn (see pgjones/hypercorn#62 (comment)) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

Would someone be interested in open a PR in Uvicorn with this proposal? We can evaluate how easy it is seeing the implementation.

Kludex avatar Jan 25 '25 10:01 Kludex

Any updates?

grydz avatar Feb 13 '25 07:02 grydz

I've implemented the ASGI TLS Extension partially for hypercorn (see pgjones/hypercorn#62 (comment)) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

Would someone be interested in open a PR in Uvicorn with this proposal? We can evaluate how easy it is seeing the implementation.

PR with some minor changes to @grydz suggestion here: encode/uvicorn#2586

I propose that the client_cert_chain is an iterable, as in the latest version the SSLSocket har gotten SSLSocket.get_verified_chain() that returns an iterable of the certificate chan from the peer. It then makes sense to return the client_cert_chain as an iterable as well.

Additionally I would propose to either add all tls related information from the server, or nothing. In the current version of the spec, only server_cert is forwarded, while it would make sense to also forward the full server ca-chain as well.

eirikhex avatar Feb 21 '25 14:02 eirikhex

I've implemented the ASGI TLS Extension partially for hypercorn (see pgjones/hypercorn#62 (comment)) and here are my thoughts on the specification:

  • client_cert_name should be removed because it's not the role of the ASGI web server to parse X.509 certificate. The content of the field is redundant with client_cert_chain and it's also easy to parse by the developer of the application with 3rd party library such as cryptography. It's reasonable to let the developer do the parsing to extract what he needs.
  • client_cert_error should also be removed because it cannot be filled with any relevant information.
  • client_cert_chain should be a unicode string and not an iterable for the same reason as client_cert_name, this is not the role of the ASGI web server to split the client certificate chain. Just let the developer do the PEM split if needed.
  • tls_version and cipher_suite shoud be filled with string returned by SSLSocket.cipher() and not converted to corresponding integers. Integer representation is used in the TLS protocol during Client Hello and Server Hello, the ssl module (or OpenSSL) always use the description with the upper case snake case style which is human-readable and used everywhere: IANA TLS Parameters, RFC5246, RFC8446.

Note that the ASGI TLS Extension is mandatory with two main use cases:

  • Client certificate authentication when you need to distinguish users (with the Common Name in the client certificate as an example).
  • mTLS where the client certificate and the server certificate must be issued by the same root CA.

Unfortunately at this time, very few ASGI web servers implement the ASGI TLS Extension.

Would someone be interested in open a PR in Uvicorn with this proposal? We can evaluate how easy it is seeing the implementation.

PR with some minor changes to @grydz suggestion here: encode/uvicorn#2586

I propose that the client_cert_chain is an iterable, as in the latest version the SSLSocket har gotten SSLSocket.get_verified_chain() that returns an iterable of the certificate chan from the peer. It then makes sense to return the client_cert_chain as an iterable as well.

Additionally I would propose to either add all tls related information from the server, or nothing. In the current version of the spec, only server_cert is forwarded, while it would make sense to also forward the full server ca-chain as well.

Great news, thanks for the PR on uvicorn.

Regarding client_cert_chain as an iterable, I agree that SSLSocket.get_verified_chain() would be the best option, but unfortunately, it's only available from Python 3.13. On older Python versions, we need to split the PEM certificate and it might complicate the code a bit on the ASGI server side with conditionals on the Python version.

grydz avatar Mar 10 '25 07:03 grydz

Regarding client_cert_chain as an iterable, I agree that SSLSocket.get_verified_chain() would be the best option, but unfortunately, it's only available from Python 3.13. On older Python versions, we need to split the PEM certificate and it might complicate the code a bit on the ASGI server side with conditionals on the Python version.

I agree with your point that it should not be a requirement to make the full chain available, however by keeping the client_cert_chain as an iterable, the server has the possibility to return the full chain if it has it (typically versions >=3.13). The suggested approach in the PR is to use SSLSocket.getpeercert() if SSLSocket.get_verified_chain() is not available.

Note that if the even the SSLSocket.getpeercert() opens up the possibility that there might not be a client cert available (typically non-mTLS connections)

eirikhex avatar Mar 16 '25 21:03 eirikhex

Bumping into this one as i'd like to do mTLS with websockets in FastAPI, please let me know if an extra set of hands would be helpful

rickwalder avatar Sep 30 '25 00:09 rickwalder

Bumping into this one as i'd like to do mTLS with websockets in FastAPI, please let me know if an extra set of hands would be helpful

Can you review https://github.com/Kludex/uvicorn/pull/2586 ?

Kludex avatar Oct 09 '25 07:10 Kludex