hypercorn icon indicating copy to clipboard operation
hypercorn copied to clipboard

Implement the TLS extension

Open pgjones opened this issue 3 years ago • 11 comments

See spec, Uvicorn, and nonecorn.

Looks as if the specification is not implementable as currently defined, therefore I will wait to see if the specification is updated.

pgjones avatar Aug 26 '22 16:08 pgjones

Yes, I'm willing to make a pull request for that since nonecorn is forked from hypercorn and should be 100% compatible with upstream. But since nonecorn used to trace gitlab's branch of hypercorn, it seems that github is preventing me from making that pull request :)

synodriver avatar Aug 30 '22 14:08 synodriver

Does the nonecorn code implements the full specification? When I looked it was similar to the Uvicorn PR in that not everything could be implemented. If so I think we need to change the ASGI specification before we commit to implementing it.

pgjones avatar Aug 31 '22 21:08 pgjones

Indeed, nonecorn, just like the uvicorn PR, did not implement the full specification, it left some keys to None, and according to the spec, those keys can be set to None. So which part do you think can't be implemented? I'll dig deeper and have a try.

Besides, I only ported it to asyncio, trio is not implemented yet, therefore more efforts is needed.

synodriver avatar Sep 03 '22 07:09 synodriver

Required for OpenAPI mutualTLS authentication - e.g. FastAPI/starlette/hypercorn.

commonism avatar Aug 13 '23 10:08 commonism

I'm migrating urllib3 from Tornado to Hypercorn to get HTTP/2 support. urllib3 has a few tests about client certificates, and relies on its test server to make sure the correct certificate was used. I have a rough initial draft that supports only what I needed for urllib3 here: https://github.com/urllib3/hypercorn/compare/main...tls-extension. I'll try to extend it to create a proper pull request.

I don't think this should be enabled by default as it's a niche use case and this could easily affect performance. How should this be exposed in Config?

(Something else I need is the alpn_protocol, that I bundled in the TLS extension, but it's not in the ASGI specification. I'd like to submit this to the specification once the existing is implemented.)

pquentin avatar Nov 28 '23 10:11 pquentin

I'd start off with nonecorn instead - it does asgi tls ext already. https://github.com/nonebot/nonecorn/commit/836f3ce9a2fe01a0439b896bdfe67de303c03727

commonism avatar Nov 28 '23 11:11 commonism

Yes but it suffers from a few issues: it does not support Trio (which is what urllib3 uses), it's a bit hacky (various functions accept ssl and tls parameters), and to my knowledge does not have tests for the TLS extension. It also does not support alpn_protocols, so urllib3 needed a temporary fork anyway. More generally, I don't link the idea of having a long-term fork for things that Hypercorn itself wants. Working together would produce a better outcome.

pquentin avatar Nov 28 '23 11:11 pquentin

Would be great if you could get tlsext into hypercorn.

Inspiration for unit tests using pytest_asyncio/trustme - https://github.com/commonism/aiopenapi3/blob/e9294f24ad5d7fa219625d401cbd0ad323893f45/tests/tls_test.py

commonism avatar Nov 28 '23 12:11 commonism

any update ?

lschneidpro avatar Jul 12 '24 15:07 lschneidpro

yes - https://github.com/encode/uvicorn/pull/1119

commonism avatar Jul 16 '24 06:07 commonism