Skip to content

Conversation

@pquentin
Copy link
Member

Closes elastic/elasticsearch-py#1958

While it looks like it might be possible to use an IP address in a certificate, I've never seen it used and it's explicitly forbidden for SNI (RFC 3546, Section 3.1). Additionally, server_hostname is not set in urllib3 when the host is an IP, which raises an exception in the ssl standard module.

The main majority of the new code comes from urllib3. I could have used urllib3.util.ssl_.is_ip_address directly but thought that was rude!

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks excellent, thanks! One thing I'd like is if we could test this new behavior with all the HTTP node classes since the issue didn't show up until we did that. Would be good to make sure warnings aren't being emitted too. Maybe using pytest-httpserver+trustme serving on 127.0.0.1?

@pquentin
Copy link
Member Author

Thanks, I've did just that. This clarified that IP addresses certificates are perfectly fine, however we need to make sure that check_hostname is not set.

This is quite a low-level concern and it's unfortunate that we need to worry about it here, but I don't think there's any way around this as long as we create our own SSLContext. If we had another API to set the SSL version and disable verification, we would not need this PR.

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This is excellent, thank you for this! :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error "ValueError: check_hostname requires server_hostname"

2 participants