From 25199dde74ea1dc230789b4f4083ade01d33929f Mon Sep 17 00:00:00 2001 From: nick evans Date: Mon, 14 Oct 2024 15:49:52 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=97=91=EF=B8=8F=20Add=20deprecation?= =?UTF-8?q?=20warnings=20to=20.new=20and=20#starttls=20[=F0=9F=9A=A7=20WIP?= =?UTF-8?q?]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * `ssl` was renamed to `tls` in most places, with backwards compatible aliases. Using `ssl` does not print any deprecation warnings. Using both `tls` and `ssl` keywords raises an ArgumentError. * Preparing for a (backwards-incompatible) secure-by-default configuration, `Net::IMAP.default_tls` will determine the value for `tls` when no explicit port or tls setting is provided. Using port 143 will be insecure by default. Using port 993 will be secure by default. Providing no explicit port will use `Net::IMAP.default_tls` with the appropriate port. And providing any other unknown port will use `default_tls` with a warning. 🚧 TODO: should we use a different config var for default tls params when port is 993 and `tls` is unspecified? 🚧 TODO: should we use a different config var for choosing `tls` when `port` is non-standard vs choosing `port` and `tls` when neither are specified? 🚧 TODO: should we use a different var for `default_tls` be used to config params when port is 993 but tls is implicit? Another var? --- lib/net/imap.rb | 44 ++++++++++++++++++++++++++++++++++++++---- lib/net/imap/config.rb | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 5e95f41c..bf4d62f8 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -802,8 +802,21 @@ def idle_response_timeout; config.idle_response_timeout end # Returns +false+ for a plaintext connection. attr_reader :ssl_ctx_params - # Creates a new Net::IMAP object and connects it to the specified - # +host+. + # Creates a new Net::IMAP object and connects it to the specified +host+. + # + # ==== Default port and SSL + # + # When both both +port+ and +ssl+ are unspecified or +nil+, + # +ssl+ is determined by {config.default_ssl}[rdoc-ref:Config#default_ssl] + # and +port+ is based on that implicit value for +ssl+. + # + # When only one of the two is specified: + # * When +ssl+ is truthy, +port+ defaults to +993+. + # * When +ssl+ is +false+, +port+ defaults to +143+. + # * When +port+ is +993+, +ssl+ defaults to +true+. + # * When +port+ is +143+, +ssl+ defaults to +false+. + # * When +port+ is nonstandard, the default for +ssl+ is determined + # by {config.default_ssl}[rdoc-ref:Config#default_ssl]. # # ==== Options # @@ -831,7 +844,9 @@ def idle_response_timeout; config.idle_response_timeout end # SSL session verification mode. Valid modes include # +OpenSSL::SSL::VERIFY_PEER+ and +OpenSSL::SSL::VERIFY_NONE+. # - # See {OpenSSL::SSL::SSLContext}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html] for other valid SSL context params. + # See + # {OpenSSL::SSL::SSLContext}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html] + # for other valid SSL context params. # # See DeprecatedClientOptions.new for deprecated SSL arguments. # @@ -912,7 +927,7 @@ def initialize(host, port: nil, ssl: nil, # Config options @host = host @config = Config.new(config, **config_options) - @port = port || (ssl ? SSL_PORT : PORT) + ssl, @port = default_ssl_and_port(ssl, port) @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(ssl) # Basic Client State @@ -2887,6 +2902,27 @@ def remove_response_handler(handler) PORT = 143 # :nodoc: SSL_PORT = 993 # :nodoc: + def default_ssl_and_port(tls, port) + if tls.nil? && port + tls = true if port == SSL_PORT || /\Aimaps\z/i === port + tls = false if port == PORT + elsif port.nil? && !tls.nil? + port = tls ? SSL_PORT : PORT + end + if tls.nil? && port.nil? + tls = config.default_tls.dup.freeze + port = tls ? SSL_PORT : PORT + if tls.nil? + warn "A future version of Net::IMAP::Config#default_tls " \ + "will default to 'true', for secure connections by default. " \ + "Use 'Net::IMAP.new(host, ssl: false)' or " \ + "Net::IMAP.config.default_tls = false' to silence this warning." + end + end + tls &&= tls.respond_to?(:to_hash) ? tls.to_hash : {} + [tls, port] + end + def start_imap_connection @greeting = get_server_greeting @capabilities = capabilities_from_resp_code @greeting diff --git a/lib/net/imap/config.rb b/lib/net/imap/config.rb index 7532737c..295355aa 100644 --- a/lib/net/imap/config.rb +++ b/lib/net/imap/config.rb @@ -207,6 +207,35 @@ def self.[](config) # The default value is +5+ seconds. attr_accessor :idle_response_timeout, type: Integer + # The default value for the +ssl+ option of Net::IMAP.new, when +port+ is + # unspecified or non-standard and +ssl+ is unspecified. default_ssl is + # ignored when Net::IMAP.new is called with any value for +ssl+ besides + # +nil+, + # + # *Note*: A future release of Net::IMAP will set the default to +true+, as + # per RFC7525[https://tools.ietf.org/html/rfc7525], + # RFC7817[https://tools.ietf.org/html/rfc7817], and + # RFC8314[https://tools.ietf.org/html/rfc8314]. + # + # (The default_ssl config attribute was added in +v0.5.?+.) + # + # ==== Valid options + # + # [+false+ (original behavior)] + # Plaintext by default, with no warnings. + # [+nil+ (planned default for +v0.6+)] + # Plaintext by default, and prints a warning. + # + # This option will be removed in +v0.7+, when +:warn+ becomes the + # default. + # [+:warn+ (planned default for +v0.7+)] + # A warning will be printed, and behave as if the default were +true+. + # [+true+ (planned future default)] + # Use TLS by default, with the default SSL context params set by calling + # {OpenSSL::SSL::SSLContext#set_params}[https://docs.ruby-lang.org/en/master/OpenSSL/SSL/SSLContext.html#method-i-set_params] + # with no params. + attr_accessor :default_ssl, type: [false, nil, :warn, true] + # Whether to use the +SASL-IR+ extension when the server and \SASL # mechanism both support it. Can be overridden by the +sasl_ir+ keyword # parameter to Net::IMAP#authenticate. @@ -362,6 +391,7 @@ def defaults_hash debug: false, open_timeout: 30, idle_response_timeout: 5, + default_ssl: false, sasl_ir: true, enforce_logindisabled: true, responses_without_block: :warn, @@ -390,9 +420,17 @@ def defaults_hash version_defaults[0.6] = Config[0.5].dup.update( responses_without_block: :frozen_dup, + default_ssl: nil, ).freeze version_defaults[:next] = Config[0.6] - version_defaults[:future] = Config[:next] + + version_defaults[0.7] = Config[0.6].dup.update( + default_ssl: :warn, + ).freeze + + version_defaults[:future] = Config[0.7].dup.update( + default_ssl: true, + ).freeze version_defaults.freeze end From 272a3a576da95cc15b5e286f66afed69476ad8a6 Mon Sep 17 00:00:00 2001 From: nick evans Date: Mon, 14 Oct 2024 15:50:03 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=9A=A7=20update=20default=5Fssl=5Fand?= =?UTF-8?q?=5Fport=20...=20WIP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/net/imap.rb | 79 ++++++++++++++++++++++++++++++++++-------- lib/net/imap/config.rb | 18 ++++++++++ 2 files changed, 82 insertions(+), 15 deletions(-) diff --git a/lib/net/imap.rb b/lib/net/imap.rb index bf4d62f8..4ef2464b 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -2903,26 +2903,75 @@ def remove_response_handler(handler) SSL_PORT = 993 # :nodoc: def default_ssl_and_port(tls, port) - if tls.nil? && port - tls = true if port == SSL_PORT || /\Aimaps\z/i === port - tls = false if port == PORT - elsif port.nil? && !tls.nil? - port = tls ? SSL_PORT : PORT - end - if tls.nil? && port.nil? - tls = config.default_tls.dup.freeze - port = tls ? SSL_PORT : PORT - if tls.nil? - warn "A future version of Net::IMAP::Config#default_tls " \ - "will default to 'true', for secure connections by default. " \ - "Use 'Net::IMAP.new(host, ssl: false)' or " \ - "Net::IMAP.config.default_tls = false' to silence this warning." - end + case [tls && true, classify_port(port)] + in true, nil then return tls, SSL_PORT + in false, nil then return tls, PORT + in nil, :tls then return true, port + in nil, :plain then return false, port + in nil, nil then return use_default_ssl + in true, :tls | :other then return tls, port + in false, :plain | :other then return tls, port + in true, :plain then return warn_mismatched_port tls, port + in false, :tls then return warn_mismatched_port tls, port + in nil, :other then return warn_nonstandard_port port end + # TODO: move this wherever is appropriate tls &&= tls.respond_to?(:to_hash) ? tls.to_hash : {} + end + + # classify_port(port) -> :tls | :plain | :other | nil + def classify_port(port) + case port + in (SSL_PORT | /\Aimaps\z/i) then :tls + in (PORT | /\Aimap\z/i) then :plain + in (Integer | String) then :other + in nil then nil + end + end + + def warn_mismatched_port(tls, port) + if tls + warn "Using TLS on plaintext IMAP port" + else + warn "Using plaintext on TLS IMAP port" + end + [tls, port] + end + + def warn_nonstandard_port(port) + tls = !!config.default_ssl + if config.warn_nonstandard_port_without_ssl + warn "Using #{tls ? "TLS" : "plaintext"} on port #{port}. " \ + "Set ssl explicitly for non-standard IMAP ports." + end + # TODO: print default_ssl warning [tls, port] end + TLS_DEFAULT_WARNING = + "Net::IMAP.config.default_ssl will default to true in the future. " \ + "To silence this warning, " \ + "set Net::IMAP.config.default_ssl = (true | false)' or " \ + "use 'Net::IMAP.new(host, ssl: (true | false))'." + private_constant :TLS_DEFAULT_WARNING + + def use_default_ssl + case config.default_ssl + when true then [true, SSL_PORT] + when false then [false, PORT] + when :warn + warn TLS_DEFAULT_WARNING unless port + port ||= SSL_PORT + warn "Using TLS on port #{port}." + [true, port] + when nil + warn TLS_DEFAULT_WARNING unless port + port ||= PORT + warn "Using plain-text on port #{port}." + [false, port] + end + end + def start_imap_connection @greeting = get_server_greeting @capabilities = capabilities_from_resp_code @greeting diff --git a/lib/net/imap/config.rb b/lib/net/imap/config.rb index 295355aa..9a1a0949 100644 --- a/lib/net/imap/config.rb +++ b/lib/net/imap/config.rb @@ -236,6 +236,22 @@ def self.[](config) # with no params. attr_accessor :default_ssl, type: [false, nil, :warn, true] + # Whether to warn for using default_ssl when the port is non-standard. + # + # Although default_ssl is used for non-standard ports, this warning is + # different replaces the warning when default_ssl is +nil+ or +:warn+. + # When this option is false but default_ssl is +nil+ or +:warn+, that + # warning will be printed instead. + # + # ==== Valid options + # + # [+false+ (original behavior)] + # Don't print a special warning for nonstandard ports without explicit + # +ssl+. + # [+true+ (eventual future default)] + # Print a special warning for nonstandard ports without explicit +ssl+. + attr_accessor :warn_nonstandard_port_without_ssl, type: :boolean + # Whether to use the +SASL-IR+ extension when the server and \SASL # mechanism both support it. Can be overridden by the +sasl_ir+ keyword # parameter to Net::IMAP#authenticate. @@ -392,6 +408,7 @@ def defaults_hash open_timeout: 30, idle_response_timeout: 5, default_ssl: false, + warn_nonstandard_port_without_ssl: false, sasl_ir: true, enforce_logindisabled: true, responses_without_block: :warn, @@ -430,6 +447,7 @@ def defaults_hash version_defaults[:future] = Config[0.7].dup.update( default_ssl: true, + warn_nonstandard_port_without_ssl: true, ).freeze version_defaults.freeze