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

Enable hostname verification with verify option #379

Merged
merged 1 commit into from
May 24, 2023

Conversation

adamleko
Copy link
Contributor

Checklist:

@adamleko adamleko changed the title Enable hostname verification with verify option (fixes #378) Enable hostname verification with verify option May 24, 2023
@adamleko
Copy link
Contributor Author

adamleko commented May 24, 2023

I'd add unit tests for this but that depends on setting up a specific ClickHouse server to test against.

I think this change is straightforward enough as it is with a careful review, but if you have thoughts on how to set up your test environment and some pointers on how to access that I can take a crack at writing an integration test.

For manual testing you can verify that certificate validation failed by dumping out sock.getpeercert() after the socket.connect(sa) call in Connection._create_socket(…). This was always empty for me unless I set context.checkhostname to True - it seems as though that setting controls all certificate verification (both hostname verification and validating the certificate was signed by a trusted CA).

@coveralls
Copy link

Coverage Status

Coverage: 84.901% (-11.6%) from 96.455% when pulling 5b25ab3 on adamleko:master into 18f28c6 on mymarilyn:master.

@xzkostyan xzkostyan merged commit 1896173 into mymarilyn:master May 24, 2023
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.

TLS certification validation not enforced
3 participants