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 should not check the host name by default. #6

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

tmtm
Copy link
Collaborator

@tmtm tmtm commented Jul 14, 2020

In tlsconnect(), the host name is checked when @ssl_context.verify_mode is not OpenSSL::SSL::VERIFY_NONE, but the verify_mode of @ssl_context generated by default is nil.

In tlsconnect(), the host name is checked when
@ssl_context.verify_mode is not OpenSSL::SSL::VERIFY_NONE, but the
verify_mode of @ssl_context generated by default is nil.
@marcandre
Copy link
Member

Good catch.
It's clearly listed that "The default mode is VERIFY_NONE, which does not perform any verification at all."

I would have expected OpenSSL::SSL::SSLContext.new.verify_mode not to be nil too.

@znz
Copy link
Member

znz commented Jul 18, 2020

I prefer secure default and easy to disable.

For example, open-uri takes ssl_verify_mode:.

require 'open-uri'
URI.open('https://expired.badssl.com/', ssl_verify_mode: OpenSSL::SSL::VERIFY_NONE, &:read)

@rhenium
Copy link
Member

rhenium commented Jul 18, 2020

I would have expected OpenSSL::SSL::SSLContext.new.verify_mode not to be nil too.

Agreed. ruby/openssl#386

@rhenium
Copy link
Member

rhenium commented Jul 18, 2020

The change in this pull request is correct as it currently trying to verify CN/SAN without checking the signature, which is completely useless.

Aside from that, net/smtp should have a better default. In fact, other net/* libraries do verify server certificate by default. (And that's why @ssl_context.verify_mode != OpenSSL::SSL::VERIFY_NONE wasn't an issue for them.)


net/* libraries all have different interfaces for SSL/TLS parameters, which I think should be unified at some time, but that's yet another topic.

  • net/http: Net::HTTP has attributes (listed in Net::HTTP::SSL_ATTRIBUTES), which will be passed to SSLContext#set_params[*].
  • net/ftp: Net::FTP.{open,new} takes a Hash as :ssl keyword, and it will be passed to SSLContext#set_params.
  • net/imap: Net::IMAP.new takes as a Hash as :ssl keyword for IMAP over SSL (and Net::IMAP#starttls's first argument, for STARTTLS), and it will passed to SSLContext#set_params.
  • net/pop: Net::POP3#enable_ssl (or Net::POP3#enable_ssl) takes a Hash, and it will be passed to SSLContext#set_params.
  • net/smtp: Net::SMTP#enable_tls and Net::SMTP#enable_starttls* take a pre-made SSLContext.

[*] SSLContext#set_params is a utility method to set sane default parameters for public internet services (which can be overridden by the Hash passed to the method).

@tmtm
Copy link
Collaborator Author

tmtm commented Jul 18, 2020

That's right. I also think net/smtp should be able to connect securely by default.
But that's a different story than this PR.
In this PR, I fixed that net/smtp.rb has a different implementation than the author intended.

Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

I agree that calling post_connection_check() does not make sense when verify_mode is not set.

@knu knu merged commit 219ba20 into ruby:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants