-
Notifications
You must be signed in to change notification settings - Fork 85
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
HTTP SNI check #45
HTTP SNI check #45
Conversation
test_tls and test_dtls are failing.
Reason: Per default the verify mode is set to SSL_VERIFY_NONE. Now we set it to SSL_VERIFY_PEER. |
6d59a5c
to
e419fd4
Compare
77569cd
to
f809c5e
Compare
The tests test_tls and test_dtls are green now @sreimers Had to fix test_https_loop in retools. How can trigger travis for this PR without pushing an update? |
1f0a37f
to
1056d57
Compare
Ok, I had to push updates already. Anyway, I think that manual triggers are not possible until we have such "workflow_dispatch" mentioned here: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/ But maybe manual triggers are not needed very often. |
Also adds a verify handler that prints warnings if verification fails.
- Pass TLS hostname set by user to SSL_set_tlsext_host_name(). - Disable SNI check for IP addresses.
1056d57
to
5b40aa8
Compare
5b40aa8
to
2d85e44
Compare
} | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it is a good practice to remove public functions.
it is probably best to keep it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe we should keep it. But on the other side:
- tls_peer_set_verify_host()
- tls_set_servername()
- tls_set_verify_server()
is very confusing. Our TLS expert told me that SSL_set1_host() and SSL_set_tlsext_host_name() should be used together. Also setting TLS method to SSL_VERIFY_PEER. Here is a good article: https://quuxplusone.github.io/blog/2020/01/27/openssl-part-4/
If we have only one function which does this for the TLS client makes the re API simpler.
Should we keep the functions anyway? If yes, I would add more details to the doxygen of these functions.
@robdyck can you please test this patch? you should verify that SNI is not present in TLS Hello. |
should be ok to merge now |
TODO (but in another PR):