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

TLS: Validate the hostname of the server certificate #864

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkauf
Copy link

@mkauf mkauf commented Aug 12, 2020

Avoid MITM attacks by checking whether the server's certificate contains the server's hostname.

Currently this works only with OpenSSL 1.1.0 or newer.

Avoid MITM attacks by checking whether the server's certificate contains the
server's hostname.

Currently this works only with OpenSSL 1.1.0 or newer.
@michael-grunder
Copy link
Collaborator

Hi, thanks for the PR!

The code looks fine to me. The only question I have is whether users are going to want to make the check optional (as opposed to always happening with OpenSSL >= 1.1.0 when they specify server_name).

Is the problem that SSL_set_tlsext_host_name doesn't by itself actually do any hostname validation?

@yossigo Any thoughts?

@mkauf
Copy link
Author

mkauf commented Aug 17, 2020

Is the problem that SSL_set_tlsext_host_name doesn't by itself actually do any hostname validation?

Yes, it only sets the SNI hostname. This name is not used to validate the server certificate.

@yossigo
Copy link
Member

yossigo commented Aug 17, 2020

@mkauf Thanks for this PR for raising this issue in the first place!

Technically this is a breaking change because anyone who's using SNI but don't have compatible certs would experience connection errors. As hiredis 1.0.0 was just released I think we may be okay with that, but would like to raise two points:

  1. I think we need an option to disable name checks, even if SNI is used. I'd consider something like redisSSLContextSetOptions(ctx, REDIS_SSL_NO_HOSTNAME_CHECK);
  2. OpenSSL 1.0.x is still widely used and we'd want hiredis to behave the same way on both versions.

@michael-grunder Does this make sense to you?

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.

3 participants