django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.48k stars 210 forks source link

ASGI TLS extension #466

Open Kludex opened 2 months ago

Kludex commented 2 months ago

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?

andrewgodwin commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

mdgilene commented 2 months ago

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.

andrewgodwin commented 2 months ago

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 commented 2 months ago

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

pquentin commented 2 months ago

@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