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

Implement the TLS extension #62

Open
pgjones opened this issue Aug 26, 2022 · 10 comments
Open

Implement the TLS extension #62

pgjones opened this issue Aug 26, 2022 · 10 comments

Comments

@pgjones
Copy link
Owner

pgjones commented Aug 26, 2022

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.

@synodriver
Copy link
Contributor

synodriver commented Aug 30, 2022

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 :)

@pgjones
Copy link
Owner Author

pgjones commented Aug 31, 2022

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.

@synodriver
Copy link
Contributor

synodriver commented Sep 3, 2022

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.

@commonism
Copy link

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

commonism added a commit to commonism/aiopenapi3 that referenced this issue Aug 13, 2023
unit testing mutualTLS requires asgi tls extensions
switch from hypercorn to nonecorn
pgjones/hypercorn#62
commonism added a commit to commonism/aiopenapi3 that referenced this issue Aug 15, 2023
unit testing mutualTLS requires asgi tls extensions
switch from hypercorn to nonecorn
pgjones/hypercorn#62
@pquentin
Copy link

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: urllib3/hypercorn@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.)

@commonism
Copy link

I'd start off with nonecorn instead - it does asgi tls ext already.
nonebot/nonecorn@836f3ce

@pquentin
Copy link

pquentin commented Nov 28, 2023

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.

@commonism
Copy link

commonism commented Nov 28, 2023

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

@lschneidpro
Copy link

any update ?

@commonism
Copy link

yes - encode/uvicorn#1119

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

5 participants