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

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

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

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Sep 20, 2023

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.

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.
@nevans
Copy link
Contributor Author

nevans commented Sep 20, 2023

How should this change be documented?

Copy link
Collaborator

@sebbASF sebbASF left a comment

Choose a reason for hiding this comment

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

Looks good to me

@eval
Copy link
Collaborator

eval commented Jan 2, 2024

I think accepting ssl_context_params 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.
#1586 (comment)

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.


imap.starttls if settings[:enable_starttls]
imap = Net::IMAP.new(settings[:address], port: settings[:port], ssl: ssl)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a good opportunity to nilify the default port setting as net/imap has good defaults since forever.

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.

Net::IMAP connection uses insecure deprecated options
3 participants