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

ASGI TLS extension #466

Open
Kludex opened this issue Aug 18, 2024 · 12 comments
Open

ASGI TLS extension #466

Kludex opened this issue Aug 18, 2024 · 12 comments

Comments

@Kludex
Copy link
Contributor

Kludex commented Aug 18, 2024

There's a PR on uvicorn to add this extension: encode/uvicorn#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?

@andrewgodwin
Copy link
Member

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?

@Kludex
Copy link
Contributor Author

Kludex commented Aug 18, 2024

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)

@jschlyter
Copy link

jschlyter commented Aug 18, 2024

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

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

@mdgilene
Copy link

Author of encode/uvicorn#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.

@andrewgodwin
Copy link
Member

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.

@Kludex
Copy link
Contributor Author

Kludex commented Aug 20, 2024

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

@pquentin
Copy link

@Kludex I have not implemented the full extension, only client_cert_name and alpn_protocol, because that's what I needed in urllib3 tests: urllib3/hypercorn@3dd8137. 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: python/cpython#109109

@grydz
Copy link

grydz commented Jan 10, 2025

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.

@jschlyter
Copy link

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.

grydz added a commit to grydz/asgiref that referenced this issue Jan 15, 2025
@grydz
Copy link

grydz commented Jan 16, 2025

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: grydz/asgiref@main...fix-asgi-tls-ext.

@Kludex
Copy link
Contributor Author

Kludex commented Jan 16, 2025

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: grydz/asgiref@main...fix-asgi-tls-ext.

That would be easier to implement. 👍

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

6 participants