Better parsing of URL parameter "ssl_options" #1132
Conversation
Due to the fact that `wrap_socket` is deprecated in Python 3.7, we can no longer use the old method of translating URL options to valid SSLContext options. Fixes #1107
Codecov Report
@@ Coverage Diff @@
## master #1132 +/- ##
=========================================
Coverage ? 85.26%
=========================================
Files ? 24
Lines ? 5096
Branches ? 697
=========================================
Hits ? 4345
Misses ? 551
Partials ? 200
Continue to review full report at Codecov.
|
|
@lukebakken - Shouldn't the following raise ValueError instead of just logging a warning? If I misconfigure something, I would hope to catch that right away via the exception , which is typical for Python code, instead of pretty much hiding it which is what just logging it amounts to: |
| sock.close() | ||
|
|
||
|
|
||
| # Note: this is the deprecated wrap_socket signature and info: |
There was a problem hiding this comment.
@lukebakken - this comment is confusing. We were using ssl.SSLSocket previously, not wrap_socket. Did you mean that ssl.SSLSocket was deprecated?
There was a problem hiding this comment.
The ssl.SSLSocket ctor we were using is deprecated and the old wrap_socket, too.
| elif os.path.isdir(opt_ca_certs): | ||
| cxt = ssl.create_default_context(capath=opt_ca_certs) | ||
| else: | ||
| LOGGER.warning('ca_certs is specified via ssl_options but ' |
There was a problem hiding this comment.
Should raise ValueError exception instead.
There was a problem hiding this comment.
I log a warning because the previous code did not raise an exception in this case. I will take these comments into account for version 1.0.0
There was a problem hiding this comment.
If I remember correctly, you don't see an error / exception until later with the old "create a socket to evaluate ssl options" method.
| password = opts.get('password') | ||
| cxt.load_cert_chain(opts['certfile'], keyfile, password) | ||
| else: | ||
| LOGGER.warning('certfile is specified via ssl_options but ' |
There was a problem hiding this comment.
Should raise ValueError exception instead.
| if opt_ciphers is not None: | ||
| cxt.set_ciphers(opt_ciphers) | ||
| else: | ||
| LOGGER.warning('ciphers specified in ssl_options but ' |
There was a problem hiding this comment.
Should raise ValueError exception instead.
| else: | ||
| LOGGER.warning('ciphers specified in ssl_options but ' | ||
| 'evaluates to None') | ||
|
|
There was a problem hiding this comment.
@lukebakken: Need to raise an exception if opts contains anything else that isn't supported. Otherwise, this change is very error-prone.
Due to the fact that
wrap_socketis deprecated in Python 3.7, we can no longer use the old method of translating URL options to valid SSLContext options.Fixes #1107