From 97e7e219e8c4485851a0493bd665910352d957e2 Mon Sep 17 00:00:00 2001 From: nick evans Date: Sun, 30 Jun 2024 14:45:06 -0400 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=94=92=20Don't=20disconnect=20for=20A?= =?UTF-8?q?uthenticationCanceled=20[=F0=9F=9A=A7=20tests]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This exception represents an intentional cancellation on the part of the client's authenticator. So the connection doesn't need to be dropped. --- lib/net/imap/sasl/authentication_exchange.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/net/imap/sasl/authentication_exchange.rb b/lib/net/imap/sasl/authentication_exchange.rb index 98bfcf37..5fb48b18 100644 --- a/lib/net/imap/sasl/authentication_exchange.rb +++ b/lib/net/imap/sasl/authentication_exchange.rb @@ -91,12 +91,12 @@ def initialize(client, mechanism, authenticator, sasl_ir: true) # Call #authenticate to execute an authentication exchange for #client # using #authenticator. Authentication failures will raise an - # exception. Any exceptions other than those in RESPONSE_ERRORS will - # drop the connection. + # exception. Any exceptions other than AuthenticationCanceled or those + # in client.response_errors will drop the connection. def authenticate client.run_command(mechanism, initial_response) { process _1 } .tap { raise AuthenticationIncomplete, _1 unless done? } - rescue *client.response_errors + rescue AuthenticationCanceled, *client.response_errors raise # but don't drop the connection rescue client.drop_connection From 6be8eba95d7c29fc5b8d8cd5c7e9fb201d578efc Mon Sep 17 00:00:00 2001 From: nick evans Date: Sun, 30 Jun 2024 14:49:31 -0400 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A5=85=20Cancel=20AUTHENTICATE=20if?= =?UTF-8?q?=20process=20raises=20an=20error=20[=F0=9F=9A=A7=20server=20err?= =?UTF-8?q?or]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exception will be re-raised after the protocol cancel response has been sent. --- lib/net/imap/sasl/authentication_exchange.rb | 21 ++++++++++++++++---- test/net/imap/fake_server/command_router.rb | 5 +++-- test/net/imap/test_imap.rb | 19 ++++++++++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/lib/net/imap/sasl/authentication_exchange.rb b/lib/net/imap/sasl/authentication_exchange.rb index 5fb48b18..3b628476 100644 --- a/lib/net/imap/sasl/authentication_exchange.rb +++ b/lib/net/imap/sasl/authentication_exchange.rb @@ -7,8 +7,6 @@ module SASL # AuthenticationExchange is used internally by Net::IMAP#authenticate. # But the API is still *experimental*, and may change. # - # TODO: catch exceptions in #process and send #cancel_response. - # TODO: raise an error if the command succeeds after being canceled. # TODO: use with more clients, to verify the API can accommodate them. # TODO: pass ClientAdapter#service to SASL.authenticator # @@ -81,6 +79,9 @@ def self.build(client, mechanism, *args, sasl_ir: true, **kwargs, &block) attr_reader :mechanism, :authenticator + # An exception that has been raised by authenticator.process. + attr_reader :process_error + def initialize(client, mechanism, authenticator, sasl_ir: true) @client = client @mechanism = Authenticators.normalize_name(mechanism) @@ -93,8 +94,17 @@ def initialize(client, mechanism, authenticator, sasl_ir: true) # using #authenticator. Authentication failures will raise an # exception. Any exceptions other than AuthenticationCanceled or those # in client.response_errors will drop the connection. + # + # When authenticator.process raises any StandardError + # (including AuthenticationCanceled), the authentication exchange will + # be canceled before re-raising the exception. The server will usually + # respond with an error response, and the client will most likely raise + # that error. This client error will supercede the original error. + # Unfortunately, the original error will not be the +#cause+ for the + # client error. But it will be available on #process_error. def authenticate client.run_command(mechanism, initial_response) { process _1 } + .tap { raise process_error if process_error } .tap { raise AuthenticationIncomplete, _1 unless done? } rescue AuthenticationCanceled, *client.response_errors raise # but don't drop the connection @@ -128,9 +138,12 @@ def initial_response end def process(challenge) - client.encode authenticator.process client.decode challenge - ensure @processed = true + return client.cancel_response if process_error + client.encode authenticator.process client.decode challenge + rescue => process_error + @process_error = process_error + client.cancel_response end end diff --git a/test/net/imap/fake_server/command_router.rb b/test/net/imap/fake_server/command_router.rb index 82430316..f170d8d9 100644 --- a/test/net/imap/fake_server/command_router.rb +++ b/test/net/imap/fake_server/command_router.rb @@ -97,8 +97,9 @@ def handler_for(command) response_b64 = resp.request_continuation("") || "" state.commands << {continuation: response_b64} end - response = Base64.decode64(response_b64) - response.empty? and return resp.fail_bad "canceled" + response_b64.strip == ?* and return resp.fail_bad "canceled" + response = Base64.decode64(response_b64) rescue :decode64_failed + response == :decode64_failed and return resp.fail_bad "invalid b64" # TODO: support mechanisms other than PLAIN. parts = response.split("\0") parts.length == 3 or return resp.fail_bad "invalid" diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index e569dcfb..1b2280a4 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -1066,6 +1066,25 @@ def test_id end end + test "#authenticate can be canceled with SASL::AuthenticationCanceled" do + with_fake_server( + preauth: false, cleartext_auth: true, + sasl_ir: false, sasl_mechanisms: %i[PLAIN] + ) do |server, imap| + registry = Net::IMAP::SASL::Authenticators.new(use_defaults: false) + registry.add_authenticator :plain, ->(*a, **kw, &b) { + ->(challenge) { + raise(Net::IMAP::SASL::AuthenticationCanceled, + "a: %p, kw: %p, b: %p" % [a, kw, b]) + } + } + assert_raise_with_message(Net::IMAP::BadResponseError, "canceled") do + imap.authenticate(:plain, hello: :world, registry: registry) + end + refute imap.disconnected? + end + end + test "#uid_expunge with EXPUNGE responses" do with_fake_server(select: "INBOX", extensions: %i[UIDPLUS]) do |server, imap| From a7e3137db362c102e31385cea66e2633c3959f47 Mon Sep 17 00:00:00 2001 From: nick evans Date: Thu, 4 Jul 2024 16:19:33 -0400 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=9A=A7=20Handle=20cancellations=20mor?= =?UTF-8?q?e=20carefully...?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/net/imap/sasl.rb | 10 +++- lib/net/imap/sasl/authentication_exchange.rb | 57 ++++++++++++++++++-- test/net/imap/test_imap.rb | 22 +++++--- 3 files changed, 77 insertions(+), 12 deletions(-) diff --git a/lib/net/imap/sasl.rb b/lib/net/imap/sasl.rb index 985b08dd..cb0b2247 100644 --- a/lib/net/imap/sasl.rb +++ b/lib/net/imap/sasl.rb @@ -103,7 +103,15 @@ module SASL # Indicates an authentication exchange that will be or has been canceled # by the client, not due to any error or failure during processing. - AuthenticationCanceled = Class.new(Error) + class AuthenticationCanceled < Error + # The error response from the server + attr_reader :response + + def initialize(message = "authentication canceled", response: nil) + super(message) + @response = response + end + end # Indicates an error when processing a server challenge, e.g: an invalid # or unparsable challenge. An underlying exception may be available as diff --git a/lib/net/imap/sasl/authentication_exchange.rb b/lib/net/imap/sasl/authentication_exchange.rb index 3b628476..df0d6699 100644 --- a/lib/net/imap/sasl/authentication_exchange.rb +++ b/lib/net/imap/sasl/authentication_exchange.rb @@ -82,6 +82,9 @@ def self.build(client, mechanism, *args, sasl_ir: true, **kwargs, &block) # An exception that has been raised by authenticator.process. attr_reader :process_error + # An exception that represents an error response from the server. + attr_reader :response_error + def initialize(client, mechanism, authenticator, sasl_ir: true) @client = client @mechanism = Authenticators.normalize_name(mechanism) @@ -103,9 +106,11 @@ def initialize(client, mechanism, authenticator, sasl_ir: true) # Unfortunately, the original error will not be the +#cause+ for the # client error. But it will be available on #process_error. def authenticate - client.run_command(mechanism, initial_response) { process _1 } - .tap { raise process_error if process_error } - .tap { raise AuthenticationIncomplete, _1 unless done? } + handle_cancellation do + client.run_command(mechanism, initial_response) { process _1 } + .tap { raise process_error if process_error } + .tap { raise AuthenticationIncomplete, _1 unless done? } + end rescue AuthenticationCanceled, *client.response_errors raise # but don't drop the connection rescue @@ -141,11 +146,53 @@ def process(challenge) @processed = true return client.cancel_response if process_error client.encode authenticator.process client.decode challenge - rescue => process_error - @process_error = process_error + rescue AuthenticationCanceled => error + @process_error = error + client.cancel_response + rescue => error + @process_error = begin + raise AuthenticationError, "error while processing server challenge" + rescue + $! + end client.cancel_response end + # | process | response | => result | + # |---------|----------|------------------------------------------| + # | success | success | success | + # | success | error | reraise response error | + # | error | success | raise incomplete error (cause = process) | + # | error | error | raise canceled error (cause = process) | + def handle_cancellation + result = begin + yield + rescue *client.response_errors => error + @response_error = error + raise unless process_error + end + raise_mutual_cancellation! if process_error && response_error + raise_incomplete_cancel!(result) if process_error && !response_error + result + end + + def raise_mutual_cancellation! + raise process_error # sets the cause + rescue + raise AuthenticationCanceled.new( + "authentication canceled (see error #cause and #response)", + response: response_error + ) + end + + def raise_incomplete_cancellation! + raise process_error # sets the cause + rescue + raise AuthenticationIncomplete.new( + response_error, "server ignored canceled authentication" + ) + end + end end end diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 1b2280a4..dd12fe6f 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -1073,14 +1073,24 @@ def test_id ) do |server, imap| registry = Net::IMAP::SASL::Authenticators.new(use_defaults: false) registry.add_authenticator :plain, ->(*a, **kw, &b) { - ->(challenge) { + obj = Object.new + obj.define_singleton_method(:process) do |challenge| raise(Net::IMAP::SASL::AuthenticationCanceled, - "a: %p, kw: %p, b: %p" % [a, kw, b]) - } + "a: %p, kw: %p, b: %p, c: %p" % [a, kw, b, challenge]) + end + obj } - assert_raise_with_message(Net::IMAP::BadResponseError, "canceled") do - imap.authenticate(:plain, hello: :world, registry: registry) - end + error = nil + assert_raise_with_message(Net::IMAP::SASL::AuthenticationCanceled, + /authentication canceled/i) do + imap.authenticate(:plain, foo: :bar, registry: registry) + rescue => error + raise # for assert_raise + end + assert_kind_of Net::IMAP::SASL::AuthenticationCanceled, error.cause + assert_equal 'a: [], kw: {:foo=>:bar}, b: nil, c: ""', error.cause.to_s + assert_kind_of Net::IMAP::BadResponseError, error.response + assert_equal "canceled", error.response.to_s refute imap.disconnected? end end