-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[#4887] add tls:
server endpoint that supports SNI, PEM parsing, and supporting code that changes OpenSSL connection/context building
#11861
base: trunk
Are you sure you want to change the base?
Conversation
as silly as 'IOpenSSLServerConnectionCreatorFactory' sounds as an interface name, there is a real problem with the previous structure: Context objects are configured by *every connection* with new ALPN/NPN parameters, which is not the way that contexts are supposed to work. The context configuration should happen once per-context per-*factory*, and the connection configuration should happen once per *connection*. However, 'per-context' is important, because an implementation of SNI requires that the context configuration happen on the creation of new contexts for other hostnames that may not have been loaded yet, not just the first context. so it is being called both too often and not often enough, and at the wrong time. client-side it's still worth doing this for symmetry, although the practical implications are somewhat less pronounced, since you just call the context methods a bit too often.
tls:
endpoint that supports SNI, PEM parsing, and supporting code that changes OpenSSL connection/context building
tls:
endpoint that supports SNI, PEM parsing, and supporting code that changes OpenSSL connection/context buildingtls:
server endpoint that supports SNI, PEM parsing, and supporting code that changes OpenSSL connection/context building
supporting wildcards and multi-SAN certs, what was I thinking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few commnents.
I am not very familiar with the serverFromString API.
I was expecting to see more documentation in this branch, covering how or why to use it.
I don't know when I will have time to fully review this.
I think that as long as it has good documentation and code coverage, we can merge it.
If anyone else is interested in this API, we can update it later.
Thanks again.
contextSetupHook: Callable[[SSL.Context], None], | ||
) -> IOpenSSLClientConnectionCreator: | ||
""" | ||
Create a client creator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is private ... so not a big name
but maybe we can have a better docstring.
To me, this docstring doesn't add any more info that I alreay got by reading the function signature def createClientCreator
Why not call it "factory" ? like getClientOptionsFactory
?
just asking :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code really suffers from a bunch of iterations of evolving an interface, where they all kind of do the same thing but progressively more correctly. OptionsFactory2
, OptionsFactory3
looks ugly and doesn't express the differences between them, so I tried to find synonyms ("creator" rather than "factory") to help draw the distinction
src/twisted/internet/_sslverify.py
Outdated
# Note: remember client options for testing. See | ||
# twisted.internet.test.test_endpoints.tlsHostnameFromEndpoint | ||
self._ctx._clientOptions = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this
"remember client for testing" seemls like a task for the developers
# Note: remember client options for testing. See | |
# twisted.internet.test.test_endpoints.tlsHostnameFromEndpoint | |
self._ctx._clientOptions = self | |
# Note: To help with testing, we keep a reference to the TLS options. | |
$ See | |
# twisted.internet.test.test_endpoints.tlsHostnameFromEndpoint | |
self._ctx._clientOptions = self |
I had to deal with Twisted context factory about 10 years ago. @glyph if you have time, maybe we can do a 15-30 minutes voice/screen sharing meeting and look into giving some feedack. I am a bit lost into all the SSL context wrapping API. |
That would be great. |
clean up implementation considerably, using verify callback like we are supposed to, not overwriting app_data, not overwrititng info_callback on the context
…ble to IOpenSSLTrustRoot
CodSpeed Performance ReportMerging #11861 will improve performances by 2.02%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
I am requesting a change for this PR.
I think that the biggest issue here is the lack of documentation for serverFromString
and the new TLS option.
How a developer should know that the tls:
server string is available and what are the available options ?
serverFromString
API is introduced here as:
fingerEndpoint = endpoints.serverFromString(reactor, "tcp:1079")
fingerEndpoint.listen(FingerFactory())
reactor.run()
Glyph mention that this is not the use case for which serverFromString
was designed for.
We also have the documentation page dedicated to endpoints but this PR is not updating that page.
I think that this PR should add an information that the SSL endpoint should not be used, and add a few words about the new TLS server endpoint.
Scope and purpose
Fixes #4887
This also makes it possible to do
twist web --path whatever --listen=tls:path/to/certbot/config/live
ifpath/to/certbot/config/live
is a directory containing.pem
files contining private keys, certificates, and chains in any layout (as, for example, the certbot live config dir does.