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

Vhost support using multiple TLS certificates #2270

Merged
merged 40 commits into from Oct 28, 2021
Merged

Vhost support using multiple TLS certificates #2270

merged 40 commits into from Oct 28, 2021

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Oct 15, 2021

Usage:

app.run(ssl=['/etc/letsencrypt/live/example.com/', '/etc/letsencrypt/live/mysite.example/'])

A connecting client provides TLS SNI which we then use to match against all loaded certificates, using the first one that matches. If no certificates match, the TLS connection gets disconnected prior to any request being made.

This PR also changes default SSLContext creation with more secure settings, so that A and A+ grades are possible on SSL Test.

sanic/tls.py Outdated Show resolved Hide resolved
@ahopkins
Copy link
Member

This pull request has been mentioned on Sanic Community Discussion. There might be relevant details there:

https://community.sanicframework.org/t/how-can-i-support-multiple-domain-names-and-multiple-ssl-certificates-on-the-same-website/900/5

@Tronic
Copy link
Member Author

Tronic commented Oct 15, 2021

TODO:
[X] Testing for compatibility with certs other than Let's Encrypt (filenames, RSA? - I only tested LE EC certs so far)
[X] Test certs for IP addresses and wildcard names (using Python ssl function to do the matching, but need to know it actually works)
[X] Whether to use chain.pem rather than fullchain.pem (NO!)
[X] Store the SNI received in conn_info
[?] Verify that any Host header received is for the same name (reject with Bad Request if mismatch?)

The last point may be problematic, especially if proxies are used, so it is not implemented at this point.

@Tronic Tronic changed the title Initial support for using multiple SSL certificates. Vhost support using multiple SSL certificates Oct 15, 2021
@Tronic
Copy link
Member Author

Tronic commented Oct 15, 2021

I am amazed by the CodeQL static analysis being able to notice that information from a certificate is logged after reading the cert with an undocumented function, with the data being passed a long way until we get to log it. What it does not understand is that the public certificate is not sensitive data. Can we somehow suppress this warning?

@Tronic
Copy link
Member Author

Tronic commented Oct 16, 2021

TODO 2:
[X] Tests
[X] Documentation

Anything else still overlooked?

sanic/tls.py Outdated Show resolved Hide resolved
@ahopkins
Copy link
Member

Can we somehow suppress this warning?

Yes.

sanic/tls.py Outdated Show resolved Hide resolved
@Tronic Tronic marked this pull request as ready for review October 17, 2021 18:10
@Tronic Tronic requested review from a team as code owners October 17, 2021 18:10
@Tronic Tronic changed the title Vhost support using multiple SSL certificates Vhost support using multiple TSL certificates Oct 19, 2021
@Tronic Tronic changed the title Vhost support using multiple TSL certificates Vhost support using multiple TLS certificates Oct 19, 2021
@Tronic
Copy link
Member Author

Tronic commented Oct 24, 2021

This needs a review. Ping @sanic-org/framework and also check the related docs PR sanic-org/sanic-guide#71

@sjsadowski
Copy link
Contributor

@sanic-org/framework If someone will give this a once-over, I'm happy to merge it in as-is.

@Tronic thanks for the fast turnaround on this!

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. LMK your thoughts on the CLI change.

sanic/__main__.py Outdated Show resolved Hide resolved
sanic/models/server_types.py Show resolved Hide resolved
sanic/tls.py Show resolved Hide resolved
sanic/tls.py Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Oct 27, 2021

Code Climate has analyzed commit bb793fa and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 99.2% (86% is the threshold).

This pull request will bring the total coverage in the repository to 86.7% (0.2% change).

View more on Code Climate.

@ahopkins ahopkins merged commit 6c7df68 into main Oct 28, 2021
@ahopkins ahopkins deleted the tls-vhosts branch October 28, 2021 13:50
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
* Initial support for using multiple SSL certificates.

* Also list IP address subjectAltNames on log.

* Use Python 3.7+ way of specifying TLSv1.2 as the minimum version. Linter fixes.

* isort

* Cleanup, store server name for later use. Add RSA ciphers. Log rejected SNIs.

* Cleanup, linter.

* Alter the order of initial log messages and handling. In particular, enable debug mode early so that debug messages during init can be shown.

* Store server name (SNI) to conn_info.

* Update test with new error message.

* Refactor for readability.

* Cleanup

* Replace old expired test cert with new ones and a script for regenerating them as needed.

* Refactor TLS tests to a separate file.

* Add cryptography to dev deps for rebuilding TLS certs.

* Minor adjustment to messages.

* Tests added for new TLS code.

* Find the correct log row before testing for message. The order was different on CI.

* More log message order fixup. The tests do not account for the logo being printed first.

* Another attempt at log message indexing fixup.

* Major TLS refactoring.

CertSelector now allows dicts and SSLContext within its list.
Server names are stored even when no list is used.
SSLContext.sanic now contains a dict with any setting passed and information extracted from cert.
That information is available on request.conn_info.cert.
Type annotations added.
More tests incl. a handler for faking hostname in tests.

* Remove a problematic logger test that apparently was not adding any coverage or value to anything.

* Revert accidental commit of uvloop disable.

* Typing fixes / refactoring.

* Additional test for cert selection. Certs recreated without DNS:localhost on sanic.example cert.

* Add tests for single certificate path shorthand and SNI information.

* Move TLS dict processing to CertSimple, make the names field optional and use names from the cert if absent.

* Sanic CLI options --tls and --tls-strict-host to use the new features.

* SSL argument typing updated

* Use ValueError for internal message passing to avoid CertificateError's odd message formatting.

* Linter

* Test CLI TLS options.

* Maybe the right codeclimate option now...

* Improved TLS argument help, removed support for combining --cert/--key with --tls.

* Removed support for strict checking without any certs, black forced fscked up formatting.

* Update CLI tests for stricter TLS options.

Co-authored-by: L. Karkkainen <tronic@users.noreply.github.com>
Co-authored-by: Adam Hopkins <admhpkns@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants