Skip to content
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

Add support for SSL context and SNI #92

Merged
merged 5 commits into from Nov 17, 2016
Merged

Add support for SSL context and SNI #92

merged 5 commits into from Nov 17, 2016

Conversation

jamesmyatt
Copy link
Contributor

@jamesmyatt jamesmyatt commented Jul 8, 2016

Use SSLContext objects, if available, since this provides more options and features including SNI (#11). Also, allows default system context from ssl.create_default_context() to be used, improves encapsulation and supports dependency injection.

Update: Removed support for SSL without SSLContext (#115)

@jamesmyatt
Copy link
Contributor Author

I've rebased this on the latest develop branch to get the updated tests, etc.

@jamesmyatt jamesmyatt changed the title Add support for SSL context Add support for SSL context and SNI Sep 8, 2016
@anthonyrisinger
Copy link

How can we move this forward? Is more work or review needed? SNI is the greatest!

@PierreF
Copy link
Contributor

PierreF commented Sep 26, 2016

Just spotted a small unexpected behavious: tls_insecure_set() does not work when using tls_set_context() with a ssl.create_default_context().

This is caused by SSLContext.check_hostname that already performs the match_hostname during handshake. This flag is due on a SSLContext produced by create_default_context().

Note: when using tls_set() and tls_insecure_set() it works since in this case the SSLContext is created using ssl.SSLContext() and not create_default_context().

I think either tls_insecure_set() should update the check_hostname flag on the ssl context or add a note in documentation that tls_insecure_set should not be used with tls_set_context.

Step to reproduce:

# Certificate created with: openssl req -x509 -nodes -newkey rsa:1024 -keyout server.key -out server.pem -days 365 -subj '/CN=localhost' -batch
# mosquitto config:
# listener 1885
# cafile server.pem
# certfile server.pem
# keyfile server.key

import paho.mqtt.client
import ssl

cl2 = paho.mqtt.client.Client()
ssl_ctx = ssl.create_default_context(cafile='server.pem')
ssl_ctx.check_hostname = False  # Without this line, I does not work
cl2.tls_set_context(ssl_ctx)
cl2.tls_insecure_set(True)

# certificate was generated for CN=localhost, not "CN=127.0.0.1"
cl2.connect('127.0.0.1', 1885)

@jamesmyatt
Copy link
Contributor Author

@PierreF , that makes sense. I think it's just a matter of re-arranging some of the logic. I'll look at it in more detail later.

@jamesmyatt
Copy link
Contributor Author

OK. So I think it's pretty complicated to get all of the logic completely consistent. I'm going open an issue to propose removing SSL support in Python versions that don't include SSLContext (i.e. require Python 2.7.9+ or Python 3.2+). It would remove quite a lot of branches and make the intended behaviour clearer.

@ralight
Copy link
Contributor

ralight commented Sep 27, 2016

That sounds fine.

Implement SNI support, if available (#11)

Signed-off-by: James Myatt <james@jamesmyatt.co.uk>
Signed-off-by: James Myatt <james@jamesmyatt.co.uk>
Signed-off-by: James Myatt <james@jamesmyatt.co.uk>
Signed-off-by: James Myatt <james@jamesmyatt.co.uk>
@jamesmyatt
Copy link
Contributor Author

jamesmyatt commented Sep 29, 2016

@PierreF , your issue should be fixed now. Note that you must call tls_insecure_set after tls_set now.

try:
# Try with server_hostname, even it's not supported in certain scenarios
sock = self._ssl_context.wrap_socket(sock, server_hostname=self._host)
except ValueError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssl.CertificateError get caught by the except ValueError. Which means that if certificate validation failed (and server_hostname is supported) we will try to re-wrap the socket (which fail).

We probably should add a except ssl.CertificateError:\n raise juste before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:( Thanks for checking this out!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now.

Looking at the ssl package docs, ssl.CertificateError should be the only error raised by the ssl code that isn't derived from OSError.

Signed-off-by: James Myatt <james@jamesmyatt.co.uk>
@Lokesh-K-Haralakatta
Copy link

When can we get the SNI support released with paho.mqtt.python pkg? We need this feature to support client side certificates in our connections to IoT Platform.

@jamesmyatt
Copy link
Contributor Author

@Lokesh-K-Haralakatta , might be better to ask at on the Paho-dev mailing list: https://dev.eclipse.org/mailman/listinfo/paho-dev

@PierreF PierreF modified the milestone: develop-next May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants