Remove hardcoded tlsv1 version in ssl.wrap_socket #223
Conversation
|
Hmm - yeah, I'm fine with the default. Alternatively, we may want to just add |
This sounds reasonable, care to make this change @duczen? Thanks for the PR! |
nsq/conn.py
Outdated
| self.io_loop.remove_handler(self.socket.fileno()) | ||
|
|
||
| opts = { | ||
| 'cert_reqs': ssl.CERT_REQUIRED, | ||
| 'ca_certs': default_ca_certs() | ||
| } | ||
| opts.update(options or {}) | ||
| self.socket = ssl.wrap_socket(self.socket, ssl_version=ssl.PROTOCOL_TLSv1, | ||
| if not opts.get('ssl_version'): | ||
| opts['ssl_version'] = ssl.PROTOCOL_TLSv1_2 |
There was a problem hiding this comment.
there's a slightly more elegant way to do this - if you include ssl_version in the opts dict initially (just above, next to ca_certs), then opts.update(options) can override it
(sorry for not seeing this earlier)
There was a problem hiding this comment.
Ah, yes. I like that much better. I'll have that up in a sec.
|
Nice, thanks. One last thing - can you squash your commits into one? (We prefer the commits to be manually squashed, and then we use the "Create a merge commit" option to record who merged.) |
duczen
force-pushed
the
remove-tls-hardcode
branch
from
206ad0c to
8cf3de1
Compare
Removed the hard coded TLSv1 version in
ssl.wrap_socket.TLS options for a reader can be set with
tls_options, however the ssl_version is already explicitly set which yields the following error when trying to set a different version:TypeError: wrap_socket() got multiple values for keyword argument 'ssl_version'This prevents me from using pynsq with
-tls-min-version=tls1.2set on my nsqds. Removing the hard coded version allows setting this value as/if needed. Running without the hardcoded value,nsq.Readerwithtls_v1=Trueand omittingtls_optionscorrectly uses the min tls version set by nsqd.