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

Specify how to enable ssl hostname verification for python client #6538

Closed
wants to merge 3 commits into from

Conversation

AurelienSylvan
Copy link

@AurelienSylvan AurelienSylvan commented Feb 29, 2024

Description

Python client documentation does not specify how to properly enable ssl hostname verification. The trick is that the False value when disabled needs to become a string when enabled, which is confusing.

Issues Resolved

None

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@hdhalter hdhalter added clients Documentation related to Clients backport 2.12 PR: Backport label for 2.12 3 - Tech review PR: Tech review in progress labels Mar 1, 2024
@hdhalter
Copy link
Contributor

hdhalter commented Mar 1, 2024

@saimedhi - Can you please review/sign off on this for technical accuracy? Thank you.

@saimedhi
Copy link

saimedhi commented Mar 7, 2024

Hello @AurelienSylvan,

May I know if you've tested using ssl_assert_hostname as a comma-separated list of hosts to enable hostname verification?

From what I've gathered in the urllib3 documentation:

  • We typically use ssl_assert_hostname when the server presents a certificate different from the hostname or the SNI hostname. here

  • It can be set to values like False, None, or an actual hostname. here

  • By default, assert_hostname is set to server_hostname. here

  • Documentation on using different hosts with distinct parameters can be found here.

  • As far as I understand, the term "comma-separated list of hosts to enable hostname verification" can be misleading when dealing with multiple hosts.

Thanks for your contribution! :)

@AurelienSylvan
Copy link
Author

@saimedhi
You're right. I've just tested with a list, and verification fails.
I don't remember why I thought it should be a list, maybe I got confused by the error message AttributeError('bool' object has no attribute 'strip').

Anyway, i'll correct that.
Thank you for your review.

Copy link

@saimedhi saimedhi left a comment

Choose a reason for hiding this comment

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

Hello @AurelienSylvan,

I know it's challenging to cover everything in just one line. I believe it would be beneficial to include detailed explanations, such as when to use it, its default value, and examples, in the opensearch-py guide here. We can then reference this documentation as needed. What are your thoughts on this approach?

@AurelienSylvan
Copy link
Author

Hello @saimedhi
Indeed, we can be more verbose. I've just added more details about ssl hostname verification, and removed the comments. WDYT ?

I don't get your point concerning the opensearch-py guide. Are you saying we should move this documentation into the opensearch-py repo ? Or add a reference somewhere ?

Aurélien Sylvan added 2 commits March 11, 2024 11:39
Signed-off-by: Aurélien Sylvan <[email protected]>
@saimedhi
Copy link

I don't get your point concerning the opensearch-py guide. Are you saying we should move this documentation into the opensearch-py repo ? Or add a reference somewhere ?

Thanks for the quick response, @AurelienSylvan! I've submitted a PR in opensearch-py for the ssl_assert_hostname guide. You're welcome to check it out and leave any comments or suggestions. Feel free to use it as a reference for your own PR.

hosts = [{'host': host, 'port': port}],
http_compress = True, # enables gzip compression for request bodies
use_ssl = True,
verify_certs = False,

Choose a reason for hiding this comment

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

To enable hostname verification, set verify_certs to True.

Please review this PR at your convenience. You may also wait until it's merged to avoid potential revisions and include some content here.

```
{% include copy.html %}

If you need to enable ssl hostname verification to the same host, specify None, or leave it to its default value.

Choose a reason for hiding this comment

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

I believe this case isn't necessary. We can just add it as a comment in the previous case, as seen in this PR. Feel free to share your thoughts on the PR if you have any other ideas. Thank you :)

@saimedhi
Copy link

saimedhi commented Apr 1, 2024

Hello @AurelienSylvan! I'm pleased to inform you that the pertinent guide has now been added to opensearch-py. Can we please close the PR? Thank you sincerely for your contributions!

@AurelienSylvan
Copy link
Author

Yep ! Let's close it. Thank you.

@AurelienSylvan
Copy link
Author

Closing this issue as documentation has been added to the opensearch-py library about handling ssl hostname verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Tech review PR: Tech review in progress backport 2.12 PR: Backport label for 2.12 clients Documentation related to Clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants