Fix _wrap_socket_sni #347
Fix _wrap_socket_sni #347
Conversation
There was a problem hiding this comment.
Thank you @moisesguimaraes. I looks good. I have just one question: Is the change breaking the current API?
|
@matusvalo I think the API is fine, but the travis tests are breaking on my fork. I was surprised that there was no error here due to that. My hypothesis is that the tests should load the certificate authority in order to validade the certificates coming from the docker container, the fail seems to be the evidence that client connections were not properly validating the server's identity. |
|
Thank you @moisesguimaraes . Can you provide also the fix of the integration tests? |
|
Sure, I think I can spend some time on it tomorrow. |
| if (server_hostname is not None) and ( | ||
| hasattr(ssl, 'HAS_SNI') and ssl.HAS_SNI) and ( | ||
| hasattr(ssl, 'SSLContext')): | ||
| if cert_reqs is not None: |
There was a problem hiding this comment.
For callers that never explicitly specified a cert_req or ssl_version on a client facing connection and expect no certification, this patch will result in a verify_mode of CERT_REQUIRED if the defaults are used. If that happens and ca_certs are not provided (likely in this case) the wrap call will fail (in theory - totally untested in practice).
There was a problem hiding this comment.
this assumption is correct, the CI was failing basically because we were not providing the ca_certs to the client side.
| context = ssl.SSLContext(ssl_version) | ||
|
|
||
| if certfile is not None: | ||
| context.load_cert_chain(certfile, keyfile) | ||
| if ca_certs is not None: |
There was a problem hiding this comment.
If ca_certs is None but verify_mode is != CERT_NONE it is worth calling SSLContext.set_default_verify_paths() to attempt to load the system-wide CA files.
There was a problem hiding this comment.
I will address this in a different patch.
* Change the default value of ssl_version to None. When not set, the proper value between ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER will be selected based on the param server_side in order to create a TLS Context object with better defaults that fit the desired connection side. * Change the default value of cert_reqs to None. The default value of ctx.verify_mode is ssl.CERT_NONE, but when ssl.PROTOCOL_TLS_CLIENT is used, ctx.verify_mode defaults to ssl.CERT_REQUIRED. * Fix context.check_hostname logic. Checking the hostname depends on having support of the SNI TLS extension and being provided with a server_hostname value. Another important thing to mention is that enabling hostname checking automatically sets verify_mode from ssl.CERT_NONE to ssl.CERT_REQUIRED in the stdlib ssl and it cannot be set back to ssl.CERT_NONE as long as hostname checking is enabled. * Refactor the SNI tests to test one thing at a time and removing some tests that were being repeated over and over. Signed-off-by: Moisés Guimarães de Medeiros <guimaraes@pm.me>
moisesguimaraes
force-pushed
the
fix-wrap-socket-sni
branch
from
41c782e
to
501e2a2
Compare
Ah sorry I missed that Ken's comment isn't yet addressed |
|
I need to address that in a different patch, as the right thing to do would be create a context via ssl.create_default_context() dropping the ssl_version param as we won't be using ssl.SSLContext() anymore. |
|
Leaving this to you. |
Ack I see |
|
Thank you @moisesguimaraes for your PR! |
Change the default value of ssl_version to None. When not set, the
proper value between ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER
will be selected based on the param server_side in order to create
a TLS Context object with better defaults that fit the desired
connection side.
Change the default value of cert_reqs to None. The default value
of ctx.verify_mode is ssl.CERT_NONE, but when ssl.PROTOCOL_TLS_CLIENT
is used, ctx.verify_mode defaults to ssl.CERT_REQUIRED.
Fix context.check_hostname logic. Checking the hostname depends on
having support of the SNI TLS extension and being provided with a
server_hostname value. Another important thing to mention is that
enabling hostname checking automatically sets verify_mode from
ssl.CERT_NONE to ssl.CERT_REQUIRED in the stdlib ssl and it cannot
be set back to ssl.CERT_NONE as long as hostname checking is enabled.
Refactor the SNI tests to test one thing at a time and removing some
tests that were being repeated over and over.
Signed-off-by: Moisés Guimarães de Medeiros guimaraes@pm.me