Merge pull request #4386 from esl/MIM-2315-fail_if_no_peer_cert

MIM-2315 Always set `fail_if_no_peer_cert` for just_tls
esl · Nov 4, 2024 · 89617b2 · 89617b2
2 parents 6fdd662 + 7f0299a
commit 89617b2
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 14 deletions.
diff --git a/big_tests/tests/connect_SUITE.erl b/big_tests/tests/connect_SUITE.erl
@@ -75,7 +75,9 @@ groups() ->
         {tls, [parallel], auth_bind_pipelined_cases() ++
                           protocol_test_cases() ++
                           cipher_test_cases()},
-        {just_tls, tls_groups()},
+        {verify_peer, [], [verify_peer_disconnects_when_client_has_no_cert,
+                           verify_peer_ignores_when_client_has_no_cert]},
+        {just_tls, [{group, verify_peer} | tls_groups()]},
         {fast_tls, tls_groups()},
         {session_replacement, [], [same_resource_replaces_session,
                                    clean_close_of_replaced_session,
@@ -207,6 +209,10 @@ end_per_testcase(replaced_session_cannot_terminate_different_nodes = CaseName, C
     distributed_helper:remove_node_from_cluster(mim2(), Config),
     mongoose_helper:restore_config(Config),
     escalus:end_per_testcase(CaseName, Config);
+end_per_testcase(verify_peer_disconnects_when_client_has_no_cert, Config) ->
+    mongoose_helper:restore_config(Config),
+    catch escalus_event:stop(Config),
+    catch escalus_cleaner:stop(Config);
 end_per_testcase(CaseName, Config) ->
     mongoose_helper:restore_config(Config),
     escalus:end_per_testcase(CaseName, Config).
@@ -215,6 +221,46 @@ end_per_testcase(CaseName, Config) ->
 %% Tests
 %%--------------------------------------------------------------------
 
+verify_peer_disconnects_when_client_has_no_cert(Config) ->
+    %% Server disconnects only when `disconnect_on_failure` is set to `true`.
+    %% It is true by default, so we make sure `disconnect_on_failure` is not in config.
+    %% `verify_mode` needs to be set to `peer`.
+    ServerTLSOpts0 = tls_opts(starttls_required, Config),
+    ServerTLSOpts = maps:remove(disconnect_on_failure, ServerTLSOpts0#{verify_mode => peer}),
+    configure_c2s_listener(Config, #{tls => ServerTLSOpts}),
+    process_flag(trap_exit, true),
+    UserSpec0 = escalus_users:get_userspec(Config, ?SECURE_USER),
+    UserSpec = [{ssl_opts, [{verify, verify_none}]}|UserSpec0],
+    try
+        escalus_connection:start(UserSpec) of
+        {error, {connection_step_failed, {{escalus_session, maybe_use_ssl}, _, _}, _}} ->
+            ok;
+        _Result ->
+            error({client_connected, Config})
+    catch
+        C:E ->
+            error({C, E, Config})
+    end.
+
+verify_peer_ignores_when_client_has_no_cert(Config) ->
+    %% Server bypasses TLS client cert verification when `disconnect_on_failure` is set to `false`.
+    ServerTLSOpts0 = tls_opts(starttls_required, Config),
+    ServerTLSOpts = ServerTLSOpts0#{disconnect_on_failure => false},
+    configure_c2s_listener(Config, #{tls => ServerTLSOpts}),
+    process_flag(trap_exit, true),
+    UserSpec0 = escalus_users:get_userspec(Config, ?SECURE_USER),
+    UserSpec = [{ssl_opts, [{verify, verify_none}]}|UserSpec0],
+    try
+        escalus_connection:start(UserSpec) of
+            {ok, _, _} ->
+                ok;
+            Other ->
+                error({client_disconnected, Config, Other})
+    catch
+        C:E ->
+            error({C, E, Config})
+    end.
+
 bad_xml(Config) ->
     %% given
     Spec = escalus_users:get_userspec(Config, alice),
@@ -709,7 +755,8 @@ configure_c2s_listener(Config, ExtraC2SOpts, RemovedC2SKeys) ->
     mongoose_helper:restart_listener(mim(), NewC2SListener).
 
 tls_opts(Mode, Config) ->
-    ExtraOpts = #{mode => Mode, cacertfile => ?CACERT_FILE, certfile => ?CERT_FILE, dhfile => ?DH_FILE},
+    ExtraOpts = #{mode => Mode, verify_mode => none,
+                  cacertfile => ?CACERT_FILE, certfile => ?CERT_FILE, dhfile => ?DH_FILE},
     Module = proplists:get_value(tls_module, Config, fast_tls),
     maps:merge(default_c2s_tls(Module), ExtraOpts).
 

diff --git a/big_tests/tests/sasl_external_SUITE.erl b/big_tests/tests/sasl_external_SUITE.erl
@@ -111,6 +111,7 @@ init_per_group(ca_signed, Config) ->
      {verify_mode, "\n  tls.verify_mode = \"peer\""} | Config];
 init_per_group(self_signed, Config) ->
     [{signed, self},
+     {ssl_options, "\n  tls.disconnect_on_failure = false"},
      {verify_mode, "\n  tls.verify_mode = \"selfsigned_peer\""} | Config];
 init_per_group(standard, Config) ->
     modify_config_and_restart("\"standard\"", Config),

diff --git a/src/just_tls.erl b/src/just_tls.erl
@@ -41,16 +41,20 @@
           {ok, mongoose_tls:tls_socket()} | {error, any()}.
 tcp_to_tls(TCPSocket, Options) ->
     inet:setopts(TCPSocket, [{active, false}]),
-    {Ref, SSLOpts} = format_opts_with_ref(Options, false),
-    Ret = case Options of
-              #{connect := true} ->
-                  % Currently unused as ejabberd_s2s_out uses fast_tls,
-                  % and outgoing pools use Erlang SSL directly
-                  ssl:connect(TCPSocket, SSLOpts);
-              #{} ->
-                  ssl:handshake(TCPSocket, SSLOpts, 5000)
-          end,
-    VerifyResults = receive_verify_results(Ref),
+    {Ref1, Ret} = case Options of
+                    #{connect := true} ->
+                        % Currently unused as ejabberd_s2s_out uses fast_tls,
+                        % and outgoing pools use Erlang SSL directly
+                        % Do not set `fail_if_no_peer_cert_opt` for SSL client
+                        % as it is a server only option.
+                        {Ref, SSLOpts} = format_opts_with_ref(Options, false),
+                        {Ref, ssl:connect(TCPSocket, SSLOpts)};
+                    #{} ->
+                        FailIfNoPeerCert = fail_if_no_peer_cert_opt(Options),
+                        {Ref, SSLOpts} = format_opts_with_ref(Options, FailIfNoPeerCert),
+                        {Ref, ssl:handshake(TCPSocket, SSLOpts, 5000)}
+                 end,
+    VerifyResults = receive_verify_results(Ref1),
     case Ret of
         {ok, SSLSocket} ->
             {ok, #tls_socket{ssl_socket = SSLSocket, verify_results = VerifyResults}};
@@ -106,13 +110,15 @@ get_peer_certificate(#tls_socket{verify_results = [Err | _]}) ->
 %% -callback close(tls_socket()) -> ok.
 close(#tls_socket{ssl_socket = SSLSocket}) -> ssl:close(SSLSocket).
 
-%% @doc Prepare SSL options for direct use of ssl:connect/2 or ssl:handshake/2
-%% The `disconnect_on_failure' option is not supported
+%% @doc Prepare SSL options for direct use of ssl:connect/2 (client side)
+%% The `disconnect_on_failure' option is expected to be unset or true
 -spec make_ssl_opts(mongoose_tls:options()) -> [ssl:tls_option()].
 make_ssl_opts(Opts) ->
     {dummy_ref, SSLOpts} = format_opts_with_ref(Opts, false),
     SSLOpts.
 
+%% @doc Prepare SSL options for direct use of ssl:handshake/2 (server side)
+%% The `disconnect_on_failure' option is expected to be unset or true
 -spec make_cowboy_ssl_opts(mongoose_tls:options()) -> [ssl:tls_option()].
 make_cowboy_ssl_opts(Opts) ->
     FailIfNoPeerCert = fail_if_no_peer_cert_opt(Opts),
@@ -156,6 +162,8 @@ error_to_list(_Error) ->
 verify_opt(#{verify_mode := none}) -> verify_none;
 verify_opt(#{}) -> verify_peer.
 
+%% accept empty peer certificate if explicitly requested not to fail
+fail_if_no_peer_cert_opt(#{disconnect_on_failure := false}) -> false;
 fail_if_no_peer_cert_opt(#{verify_mode := peer}) -> true;
 fail_if_no_peer_cert_opt(#{verify_mode := selfsigned_peer}) -> true;
 fail_if_no_peer_cert_opt(#{}) -> false.