-
Notifications
You must be signed in to change notification settings - Fork 931
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
Net::IMAP connection uses insecure deprecated options #1586
Comments
I haven't tested this yet, but it's tentatively as simple as the following: ssl = settings[:enable_ssl]
starttls = settings[:enable_starttls]
ssl &&= Hash.try_convert(ssl) || {}
starttls &&= Hash.try_convert(starttls) || {}
imap = Net::IMAP.new(settings[:address], port: settings[:port], ssl: ssl)
imap.starttls(starttls) if starttls |
This fixes the insecure `verify = false` argument that was previously unconfigurable. Now _any_ SSLContext params can be set on the IMAP connection. The original parameters have been undocumented since ~2007, will print deprecation warnings in the next release and will be removed in a year. Fixes mikel#1586.
Some mail servers use self-signed certificates. There is not much wrong with that because they mainly care about encryption (which self-signed provides), and care less about MITM (because that's pretty rare on server to server communication). So I would keep the default |
This library is used in plenty of situations where the identity of the remote server cannot be safely assumed without verification. Although I understand this will be considered a breaking change by some people, as far as I'm concerned this is shocking behavior. It's broken for everyone now, to put it mildly. Fixing security bugs this severe should not cause a major version bump for the simple reason that a major version bump will prevent users from receiving the security fix. Every bugfix changes behavior, by definition, and that always runs the risk of being backward incompatible for someone. The API is compatible and it should only affect servers or clients that have been misconfigured. We might not have been surprised by this as the default in 2012 (unfortunately). But in 2023, there is frankly no excuse for a TLS client to not verify the server's identity by default. If users can't figure out how to set up Let's Encrypt or add a ca_file or ca_path to their SSLContext, then they should be forced to explicitly ask for From RFC-3501 (2006):
For the purposes of a library like this gem, requiring the user to add an explicit |
Also, for what it's worth, |
I asked Google Bart about the he number of mail servers with a self-signed certificates, and after some prodding it came up with 7%, stating that most of those would be on internal corporate networks (where you can't easily get a certificate). No idea about the accuracy. Anyway, I'm fine with the PR as it stands, I'm sure the people who need it can find a way to disable verification again, but the mail gem maintainers are the ones to convince. |
I'm currently working on SASL-related PRs for both So, if it's important, I think it's fine to add |
FYI the deprecation warning in Net::IMAP has appeared. I appreciate the discussion above regarding 'breaking changes', but if this issue isn't addressed one way or another reasonably shortly all use of IMAP by users of this library will break. |
The parameters that are used to create the Net::IMAP connection in Mail::IMAP are undocumented:
mail/lib/mail/network/retriever_methods/imap.rb
Line 167 in b6b6cb7
I believe the documentation was removed in 2007 (ruby/net-imap@e631911e). Pending the following PR: ruby/net-imap#175, the next release of net-imap will print a deprecation warning when those parameters are used. About a year later, they'll be removed entirely.
A bigger issue is that the fifth and final argument is false: that is the
verify
parameter and by setting it to false, the SSLContext will haveverify_mode: OpenSSL::SSL::VERIFY_NONE
.Proposal
When
Hash.try_convert(enable_ssl)
returns a Hash, that will be used withWhen
Hash.try_convert(enable_starttls)
returns a Hash, that will be used withIn this way, any SSLContext attribute can be set.
When
enable_ssl
orenable_starttls
are truthy but not hash-like, use{}
. This way, the default is secure.I can write a PR for this, if you agree with my proposal.
The text was updated successfully, but these errors were encountered: