Warn when using deprecated SASL mechanisms

Mark obolete SASL mechanisms as deprecated (fixes rubyGH-55):
* Warn every time a deprecated mechanism is used.
* Warnings can be disabled with `warn_deprecation: false`
* delay loading stdgem dependencies until `#initialize`.  Fixes rubyGH-56.
* This is a backwards-compatible alternative to the approach in rubyGH-58
  (don't require and add the deprecated authenticators automatically).
  We can use that incompatible approach in a later version.

Additionally:
* Adds basic tests for every authenticator (to avoid another rubyGH-52!)
* Fixes a frozen string bug in DigestMD5Authenticator.
* By making these optional, there's no reason to require the `digest` or
  `strscan` gems anymore; fixes rubyGH-56.

The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.

Co-authored-by: Stephen Paul Weber <[email protected]>

lib/net/imap.rb

@@ -378,28 +378,37 @@ def starttls(options = {}, verify = true)
     # Sends an AUTHENTICATE command to authenticate the client.
     # The +auth_type+ parameter is a string that represents
     # the authentication mechanism to be used. Currently Net::IMAP
-    # supports the authentication mechanisms:
-    #
-    #   LOGIN:: login using cleartext user and password.
-    #   CRAM-MD5:: login with cleartext user and encrypted password
-    #              (see [RFC-2195] for a full description).  This
-    #              mechanism requires that the server have the user's
-    #              password stored in clear-text password.
-    #
-    # For both of these mechanisms, there should be two +args+: username
-    # and (cleartext) password.  A server may not support one or the other
-    # of these mechanisms; check #capability for a capability of
-    # the form "AUTH=LOGIN" or "AUTH=CRAM-MD5".
-    #
-    # Authentication is done using the appropriate authenticator object:
-    # see +add_authenticator+ for more information on plugging in your own
-    # authenticator.
+    # supports the following mechanisms:
+    #
+    # PLAIN:: Login using cleartext user and password.  Secure with TLS.
+    #         See Net::IMAP::PlainAuthenticator.
+    # CRAM-MD5::   DEPRECATED: Use PLAIN (or DIGEST-MD5) with TLS.
+    # DIGEST-MD5:: DEPRECATED by RFC6331. Must be secured using TLS.
+    #              See Net::IMAP::DigestMD5Authenticator.
+    # LOGIN::      DEPRECATED: Use PLAIN.
+    #
+    # Most mechanisms require two args: authentication identity (e.g. username)
+    # and credentials (e.g. a password).  But each mechanism requires and allows
+    # different arguments; please consult the documentation for the specific
+    # mechanisms you are using.  <em>Several obsolete mechanisms are available
+    # for backwards compatibility.  Using deprecated mechanisms will issue
+    # warnings.</em>
+    #
+    # Servers do not support all mechanisms and clients must not attempt to use
+    # a mechanism unless "AUTH=#{mechanism}" is listed as a #capability.
+    # Clients must not attempt to authenticate or #login when +LOGINDISABLED+ is
+    # listed with the capabilities.  Server capabilities, especially auth
+    # mechanisms, do change after calling #starttls so they need to be checked
+    # again.
     #
     # For example:
     #
-    #    imap.authenticate('LOGIN', user, password)
+    #    imap.authenticate('PLAIN', user, password)
     #
     # A Net::IMAP::NoResponseError is raised if authentication fails.
+    #
+    # See +Net::IMAP::Authenticators+ for more information on plugging in your
+    # own authenticator.
     def authenticate(auth_type, *args)
       authenticator = self.class.authenticator(auth_type, *args)
       send_command("AUTHENTICATE", auth_type) do |resp|

lib/net/imap/authenticators.rb

@@ -19,13 +19,15 @@ def add_authenticator(auth_type, authenticator)
 
   # Builds an authenticator for Net::IMAP#authenticate.  +args+ will be passed
   # directly to the chosen authenticator's +#initialize+.
-  def authenticator(auth_type, *args)
-    auth_type = auth_type.upcase
-    unless authenticators.has_key?(auth_type)
-      raise ArgumentError,
-        format('unknown auth type - "%s"', auth_type)
+  def authenticator(mechanism, *authargs, **properties, &callback)
+    authenticator = authenticators.fetch(mechanism.upcase) do
+      raise ArgumentError, 'unknown auth type - "%s"' % mechanism
+    end
+    if authenticator.respond_to?(:new)
+      authenticator.new(*authargs, **properties, &callback)
+    else
+      authenticator.call(*authargs, **properties, &callback)
     end
-    authenticators[auth_type].new(*args)
   end
 
   private
@@ -38,7 +40,8 @@ def authenticators
 
 Net::IMAP.extend Net::IMAP::Authenticators
 
-require_relative "authenticators/login"
 require_relative "authenticators/plain"
+
+require_relative "authenticators/login"
 require_relative "authenticators/cram_md5"
 require_relative "authenticators/digest_md5"

lib/net/imap/authenticators/cram_md5.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-require "digest/md5"
-
 # Authenticator for the "+CRAM-MD5+" SASL mechanism, specified in
 # RFC2195[https://tools.ietf.org/html/rfc2195].  See Net::IMAP#authenticate.
 #
@@ -23,7 +21,11 @@ def process(challenge)
 
   private
 
-  def initialize(user, password)
+  def initialize(user, password, warn_deprecation: true, **_ignored)
+    if warn_deprecation
+      warn "WARNING: CRAM-MD5 mechanism is deprecated." # TODO: recommend SCRAM
+    end
+    require "digest/md5"
     @user = user
     @password = password
   end

lib/net/imap/authenticators/digest_md5.rb

@@ -1,8 +1,5 @@
 # frozen_string_literal: true
 
-require "digest/md5"
-require "strscan"
-
 # Net::IMAP authenticator for the "`DIGEST-MD5`" SASL mechanism type, specified
 # in RFC2831(https://tools.ietf.org/html/rfc2831).  See Net::IMAP#authenticate.
 #
@@ -29,8 +26,8 @@ def process(challenge)
         sparams[k] = v
       end
 
-      raise DataFormatError, "Bad Challenge: '#{challenge}'" unless c.rest.size == 0
-      raise Error, "Server does not support auth (qop = #{sparams['qop'].join(',')})" unless sparams['qop'].include?("auth")
+      raise Net::IMAP::DataFormatError, "Bad Challenge: '#{challenge}'" unless c.eos?
+      raise Net::IMAP::Error, "Server does not support auth (qop = #{sparams['qop'].join(',')})" unless sparams['qop'].include?("auth")
 
       response = {
         :nonce => sparams['nonce'],
@@ -77,11 +74,18 @@ def process(challenge)
     end
   end
 
-  def initialize(user, password, authname = nil)
+  def initialize(user, password, authname = nil, warn_deprecation: true)
+    if warn_deprecation
+      warn "WARNING: DIGEST-MD5 SASL mechanism was deprecated by RFC6331."
+      # TODO: recommend SCRAM instead.
+    end
+    require "digest/md5"
+    require "strscan"
     @user, @password, @authname = user, password, authname
     @nc, @stage = {}, STAGE_ONE
   end
 
+
   private
 
   STAGE_ONE = :stage_one
@@ -100,7 +104,7 @@ def nc(nonce)
   def qdval(k, v)
     return if k.nil? or v.nil?
     if %w"username authzid realm nonce cnonce digest-uri qop".include? k
-      v.gsub!(/([\\"])/, "\\\1")
+      v = v.gsub(/([\\"])/, "\\\1")
       return '%s="%s"' % [k, v]
     else
       return '%s=%s' % [k, v]

lib/net/imap/authenticators/login.rb

@@ -33,7 +33,10 @@ def process(data)
   STATE_USER = :USER
   STATE_PASSWORD = :PASSWORD
 
-  def initialize(user, password)
+  def initialize(user, password, warn_deprecation: true, **_ignored)
+    if warn_deprecation
+      warn "WARNING: LOGIN SASL mechanism is deprecated. Use PLAIN instead."
+    end
     @user = user
     @password = password
     @state = STATE_USER

net-imap.gemspec

@@ -32,6 +32,6 @@ Gem::Specification.new do |spec|
   spec.require_paths = ["lib"]
 
   spec.add_dependency "net-protocol"
-  spec.add_dependency "digest"
-  spec.add_dependency "strscan"
+  spec.add_development_dependency "digest"
+  spec.add_development_dependency "strscan"
 end

test/net/imap/test_imap_authenticators.rb

@@ -5,19 +5,128 @@
 
 class IMAPAuthenticatorsTest < Test::Unit::TestCase
 
-  PLAIN = Net::IMAP::PlainAuthenticator
+  # ----------------------
+  # PLAIN
+  # ----------------------
 
-  def test_plain
-    assert_equal("\0authc\0passwd",
-                 PLAIN.new("authc", "passwd").process(nil))
+  def plain(*args, **kwargs, &block)
+    Net::IMAP.authenticator("PLAIN", *args, **kwargs, &block)
+  end
+
+  def test_plain_authenticator_matches_mechanism
+    assert_kind_of(Net::IMAP::PlainAuthenticator, plain("user", "pass"))
+  end
+
+  def test_plain_response
+    assert_equal("\0authc\0passwd", plain("authc", "passwd").process(nil))
     assert_equal("authz\0user\0pass",
-                 PLAIN.new("user", "pass", authzid: "authz").process(nil))
+                 plain("user", "pass", authzid: "authz").process(nil))
   end
 
   def test_plain_no_null_chars
-    assert_raise(ArgumentError) { PLAIN.new("bad\0user", "pass") }
-    assert_raise(ArgumentError) { PLAIN.new("user", "bad\0pass") }
-    assert_raise(ArgumentError) { PLAIN.new("u", "p", authzid: "bad\0authz") }
+    assert_raise(ArgumentError) { plain("bad\0user", "pass") }
+    assert_raise(ArgumentError) { plain("user", "bad\0pass") }
+    assert_raise(ArgumentError) { plain("u", "p", authzid: "bad\0authz") }
+  end
+
+  # ----------------------
+  # LOGIN (obsolete)
+  # ----------------------
+
+  def login(*args, warn_deprecation: false, **kwargs, &block)
+    Net::IMAP.authenticator(
+      "LOGIN", *args, warn_deprecation: warn_deprecation, **kwargs, &block
+    )
+  end
+
+  def test_login_authenticator_matches_mechanism
+    assert_kind_of(Net::IMAP::LoginAuthenticator, login("n", "p"))
+  end
+
+  def test_login_authenticator_deprecated
+    assert_warn(/LOGIN.+deprecated.+PLAIN/) do
+      Net::IMAP.authenticator("LOGIN", "user", "pass")
+    end
+  end
+
+  def test_login_responses
+    auth_session = login("username", "password")
+    assert_equal("username", auth_session.process("username?"))
+    assert_equal("password", auth_session.process("password?"))
+  end
+
+  # ----------------------
+  # CRAM-MD5 (obsolete)
+  # ----------------------
+
+  def cram_md5(*args, warn_deprecation: false, **kwargs, &block)
+    Net::IMAP.authenticator(
+      "CRAM-MD5", *args, warn_deprecation: warn_deprecation, **kwargs, &block
+    )
+  end
+
+  def test_cram_md5_authenticator_matches_mechanism
+    assert_kind_of(Net::IMAP::CramMD5Authenticator, cram_md5("n", "p"))
+  end
+
+  def test_cram_md5_authenticator_deprecated
+    assert_warn(/CRAM-MD5.+deprecated./) do
+      Net::IMAP.authenticator("CRAM-MD5", "user", "pass")
+    end
+  end
+
+  def test_cram_md5_authenticator
+    auth = cram_md5("username", "password")
+    assert_match("username e2ce8ff3d1b914ddf339aa9f55198f86",
+                 auth.process("fake-server-challence-string"))
+  end
+
+  # ----------------------
+  # DIGEST-MD5 (obsolete)
+  # ----------------------
+
+  def digest_md5(*args, warn_deprecation: false, **kwargs, &block)
+    Net::IMAP.authenticator(
+      "DIGEST-MD5", *args, warn_deprecation: warn_deprecation, **kwargs, &block
+    )
+  end
+
+  def test_digest_md5_authenticator_matches_mechanism
+    assert_kind_of(Net::IMAP::DigestMD5Authenticator, digest_md5("n", "p", "z"))
+  end
+
+  def test_digest_md5_authenticator_deprecated
+    assert_warn(/DIGEST-MD5.+deprecated.+RFC6331/) do
+      Net::IMAP.authenticator("DIGEST-MD5", "user", "pass")
+    end
+  end
+
+  def test_digest_md5_authenticator
+    auth = digest_md5("cid", "password", "zid")
+    assert_match(
+      %r{\A
+        nonce="OA6MG9tEQGm2hh",
+        username="cid",
+        realm="somerealm",
+        cnonce="[a-zA-Z0-9+/]{12,}={0,3}", # RFC2831: >= 64 bits of entropy
+        digest-uri="imap/somerealm",
+        qop="auth",
+        maxbuf=65535,
+        nc=00000001,
+        charset=utf-8,
+        authzid="zid",
+        response=[a-f0-9]+
+      \Z}x,
+      auth.process(
+        %w[
+          realm="somerealm"
+          nonce="OA6MG9tEQGm2hh"
+          qop="auth"
+          charset=utf-8
+          algorithm=md5-sess
+        ].join(",")
+      )
+    )
   end
 
 end