mikel / mail

A Really Ruby Mail Library
MIT License
3.6k stars 931 forks source link

Stop using insecure deprecated Net::IMAP.new args #1587

Open nevans opened 10 months ago

nevans commented 10 months ago

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 #1586.

nevans commented 10 months ago

How should this change be documented?

eval commented 6 months ago

I think accepting ssl_contextparams as value for `enable(ssl|starttls)` is somewhat opaque. It's also different from how smtp is configured.

As of branch 2-8-stable this is the situation:

| goal            | smtp                                                                  | imap                   |
|-----------------+-----------------------------------------------------------------------+------------------------|
| enable tls/ssl  | options tls (bool), ssl (bool)                                        | enable_ssl (bool)      |
| enable starttls | enable_starttls (bool)                                                | enable_starttls (bool) |
| verify certs    | openssl_verify_mode (string or integer) (serving tls and starttls)    |                        |
| certs           | (undocumented) options ca_path and ca_file (serving tls and starttls) |                        |

How about keeping the enable_* options as is and adding the options openssl_verify_mode, ca_path and ca_file just like smtp?

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. https://github.com/mikel/mail/issues/1586#issuecomment-1754237729

As for the 'reach' of this secure-by-default-setting: I think the 2-8-stable-branch is the one to target alongside the master-branch. The two branches have diverted in such a way (i.e. a lot of 2-8-stable is not on master) that I don't see a release being cut from master any time soon.