handle TLS SNI #1101
handle TLS SNI #1101
Conversation
(closes scrapy#981, scrapy#1101)
|
I noticed |
| @@ -17,4 +23,6 @@ def getContext(self, hostname=None, port=None): | |||
| # Enable all workarounds to SSL bugs as documented by | |||
| # http://www.openssl.org/docs/ssl/SSL_CTX_set_options.html | |||
| ctx.set_options(SSL.OP_ALL) | |||
| if hostname and ClientTLSOptions is not None: # workaround for TLS SNI | |||
| ClientTLSOptions(hostname, ctx) | |||
There was a problem hiding this comment.
I'm not sure but I think this should be applied only to TLS connections. It would be good to test it against different versions of SSL and TLS protocols to make sure it works as expected in all the cases.
There was a problem hiding this comment.
From the docstring and the import line, I just figured this was only applicable to SSL connections anyway.
We're also only accepting TLSv1 connections here (self.method = SSL.TLSv1_METHOD).
(Anything else is just stupid now anyway, after SSL3 has mostly left the building).
So there isn't much need to test other protocols for this class ( unless someone overrides the TLS line here, then that'd be an unsupported case ).
That's not to say that a test-case wouldn't be great.
But I noticed that all my SNI sites also work without that fix (strange), so I have no real host to point a testcase at, and it should preferrably be an offline test anyway? If such a test can even be fabricated.
Any help welcome.
There was a problem hiding this comment.
I'd merge this and put in the wild under unstable branch so we can get more feedback.
|
LGTM. |
implement the SNI fix as given in #981