Skip to content

TLS should not check the host name by default.#6

Merged
knu merged 1 commit into
ruby:masterfrom
tmtm:ssl_context_verify_mode
Jul 20, 2020
Merged

TLS should not check the host name by default.#6
knu merged 1 commit into
ruby:masterfrom
tmtm:ssl_context_verify_mode

Conversation

@tmtm

@tmtm tmtm commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

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.
@tmtm tmtm force-pushed the ssl_context_verify_mode branch from aeadb49 to bde75a1 Compare July 14, 2020 16:17
@marcandre

Copy link
Copy Markdown
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

znz commented Jul 18, 2020

Copy link
Copy Markdown
Member

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

rhenium commented Jul 18, 2020

Copy link
Copy Markdown
Member

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

Agreed. ruby/openssl#386

@rhenium

rhenium commented Jul 18, 2020

Copy link
Copy Markdown
Member

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

tmtm commented Jul 18, 2020

Copy link
Copy Markdown
Collaborator Author

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.

@knu knu left a comment

Copy link
Copy Markdown
Member

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants