From 53932f99278a146613251a8152ec7736df54a631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 13 Oct 2023 12:47:08 +0200 Subject: [PATCH 1/5] Add tests for STARTTLS - When STARTTLS is disabled, no features should be advertised, and TLS upgrade should be rejected - STARTTLS features should be correctly advertised before and after performing the TLS upgrade Also: reorganize tests, making group and test names more intuitive testf --- big_tests/tests/connect_SUITE.erl | 133 +++++++++++++++++++----------- 1 file changed, 85 insertions(+), 48 deletions(-) diff --git a/big_tests/tests/connect_SUITE.erl b/big_tests/tests/connect_SUITE.erl index cb539505223..4c0a9beb7a2 100644 --- a/big_tests/tests/connect_SUITE.erl +++ b/big_tests/tests/connect_SUITE.erl @@ -57,17 +57,21 @@ all() -> groups() -> [ - {c2s_noproc, [], [bad_xml, - invalid_host, - invalid_stream_namespace, - deny_pre_xmpp_1_0_stream]}, - {starttls, [], [should_fail_to_authenticate_without_starttls, - should_not_send_other_features_with_starttls_required, - auth_bind_pipelined_starttls_skipped_error | protocol_test_cases()]}, + {starttls_disabled, [parallel], [correct_features_are_advertised_for_disabled_starttls, + starttls_should_fail_when_disabled]}, + {starttls_optional, [parallel], [bad_xml, + invalid_host, + invalid_stream_namespace, + deny_pre_xmpp_1_0_stream, + correct_features_are_advertised_for_optional_starttls]}, + {starttls_required, [], [{group, feature_order}, + metrics_test, + should_fail_to_authenticate_without_starttls, + auth_bind_pipelined_starttls_skipped_error | protocol_test_cases()]}, {tls, [parallel], auth_bind_pipelined_cases() ++ protocol_test_cases() ++ cipher_test_cases()}, - {feature_order, [parallel], [stream_features_test, + {feature_order, [parallel], [correct_features_are_advertised_for_required_starttls, tls_authenticate, bind_server_generated_resource, cannot_connect_with_proxy_header]}, @@ -88,10 +92,9 @@ groups() -> tls_groups()-> [ - {group, starttls}, - {group, c2s_noproc}, - {group, feature_order}, - metrics_test, %% must follow right after feature_order group + {group, starttls_disabled}, + {group, starttls_optional}, + {group, starttls_required}, {group, tls} ]. @@ -142,16 +145,19 @@ end_per_suite(Config) -> restore_c2s_listener(Config), escalus:end_per_suite(Config). -init_per_group(c2s_noproc, Config) -> +init_per_group(starttls_optional, Config) -> configure_c2s_listener(Config, #{tls => tls_opts(starttls, Config)}), Config; init_per_group(session_replacement, Config) -> configure_c2s_listener(Config, #{tls => tls_opts(starttls, Config)}), logger_ct_backend:start(), Config; -init_per_group(starttls, Config) -> +init_per_group(starttls_required, Config) -> configure_c2s_listener(Config, #{tls => tls_opts(starttls_required, Config)}), Config; +init_per_group(starttls_disabled, Config) -> + configure_c2s_listener(Config, #{}, [tls]), + Config; init_per_group(tls, Config) -> configure_c2s_listener(Config, #{tls => tls_opts(tls, Config)}), Users = proplists:get_value(escalus_users, Config, []), @@ -160,9 +166,6 @@ init_per_group(tls, Config) -> NewUsers = lists:keystore(?SECURE_USER, 1, Users, JoeSpec2), Config2 = lists:keystore(escalus_users, 1, Config, {escalus_users, NewUsers}), [{c2s_port, ct:get_config({hosts, mim, c2s_port})} | Config2]; -init_per_group(feature_order, Config) -> - configure_c2s_listener(Config, #{tls => tls_opts(starttls_required, Config)}), - Config; init_per_group(just_tls, Config)-> [{tls_module, just_tls} | Config]; init_per_group(fast_tls, Config)-> @@ -312,17 +315,6 @@ should_fail_to_authenticate_without_starttls(Config) -> AuthReply) end. -should_not_send_other_features_with_starttls_required(Config) -> - UserSpec = escalus_users:get_userspec(Config, ?SECURE_USER), - {ok, Conn, _} = escalus_connection:start(UserSpec, [start_stream]), - Features = case escalus_connection:get_stanza(Conn, wait_for_features) of - #xmlel{name = <<"stream:features">>, children = Children} -> Children; - #xmlel{name = <<"features">>, children = Children} -> Children - end, - ?assertMatch([#xmlel{name = <<"starttls">>, - children = [#xmlel{name = <<"required">>}]}], - Features). - clients_can_connect_with_advertised_ciphers(Config) -> ?assert(length(ciphers_working_with_ssl_clients(Config)) > 0). @@ -342,27 +334,69 @@ clients_can_connect_with_advertised_ciphers(Config) -> ?assertEqual(["ECDHE-RSA-AES256-GCM-SHA384"], ciphers_working_with_ssl_clients(Config1)). -%% Tests features advertisement -stream_features_test(Config) -> +correct_features_are_advertised_for_disabled_starttls(Config) -> + UserSpec = escalus_fresh:freshen_spec(Config, alice), + Steps = [start_stream, + stream_features, + {?MODULE, verify_features_without_starttls}, + authenticate], + escalus_connection:start(UserSpec, Steps). + +correct_features_are_advertised_for_optional_starttls(Config) -> + UserSpec = escalus_fresh:freshen_spec(Config, ?SECURE_USER), + Steps = [start_stream, + stream_features, + {?MODULE, verify_features_with_optional_starttls}, + maybe_use_ssl, + {?MODULE, verify_features_without_starttls}, + authenticate], + escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], Steps). + +correct_features_are_advertised_for_required_starttls(Config) -> UserSpec = escalus_fresh:freshen_spec(Config, ?SECURE_USER), - List = [start_stream, stream_features, {?MODULE, verify_features}], - escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], List), - ok. - -verify_features(Conn, Features) -> - %% should not advertise compression before tls - ?assertEqual({compression, false}, get_feature(compression, Features)), - %% start tls. Starttls should be then removed from list and compression should be added - Conn1 = escalus_session:starttls(Conn), - {Conn2, Features2} = escalus_session:stream_features(Conn1, []), - ?assertEqual({starttls, false}, get_feature(starttls, Features2)), - ?assertEqual({compression, false}, get_feature(compression, Features2)), - %% start authentication - escalus_session:authenticate(Conn2, Features2). + Steps = [start_stream, + stream_features, + {?MODULE, verify_features_with_required_starttls}, + maybe_use_ssl, + {?MODULE, verify_features_without_starttls}, + authenticate], + escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], Steps). + +verify_features_without_starttls(Conn, Features) -> + ?assertEqual({starttls, false}, get_feature(starttls, Features)), + ?assertMatch({sasl_mechanisms, [_|_]}, get_feature(sasl_mechanisms, Features)), + {Conn, Features}. + +verify_features_with_optional_starttls(Conn, Features) -> + ?assertEqual({starttls, true}, get_feature(starttls, Features)), + ?assertMatch({sasl_mechanisms, [_|_]}, get_feature(sasl_mechanisms, Features)), + {Conn, Features}. + +verify_features_with_required_starttls(Conn, Features) -> + AdvertisedFeatures = lists:filter(fun is_present/1, Features), + ?assertEqual([{starttls, true}], AdvertisedFeatures), + {Conn, Features}. + +is_present({_, Value}) -> + Value =/= false andalso Value =/= [] andalso Value =/= undefined. get_feature(Feature, FeatureList) -> lists:keyfind(Feature, 1, FeatureList). +starttls_should_fail_when_disabled(Config) -> + UserSpec = escalus_fresh:freshen_spec(Config, alice), + List = [start_stream, stream_features], + {ok, Conn, _Features} = + escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], List), + + %% Client tries to start tls anyway, and fails + escalus_connection:send(Conn, escalus_stanza:starttls()), + Result = escalus_connection:get_stanza(Conn, failure), + %% As defined in https://datatracker.ietf.org/doc/html/rfc6120#section-5.4.2.2, cause 2 + ?assertEqual(<<"failure">>, Result#xmlel.name), + escalus:assert(has_ns, [?NS_TLS], Result), + escalus_connection:wait_for_close(Conn, timer:seconds(5)). + metrics_test(Config) -> MongooseMetrics = [{[global, data, xmpp, received, xml_stanza_size], changed}, {[global, data, xmpp, sent, xml_stanza_size], changed}, @@ -378,9 +412,9 @@ metrics_test(Config) -> tls_authenticate(Config) -> %% Given UserSpec = escalus_fresh:create_fresh_user(Config, ?SECURE_USER), - ConnetctionSteps = [start_stream, stream_features, maybe_use_ssl, authenticate], + ConnectionSteps = [start_stream, stream_features, maybe_use_ssl, authenticate], %% when - {ok, Conn, _} = escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], ConnetctionSteps), + {ok, Conn, _} = escalus_connection:start(UserSpec ++ [{ssl_opts, [{verify, verify_none}]}], ConnectionSteps), % then true = escalus_tcp:is_using_ssl(Conn#client.rcv_pid). @@ -665,9 +699,12 @@ assert_cert_file_exists() -> ejabberd_node_utils:file_exists(?CERT_FILE) orelse ct:fail("cert file ~s not exists", [?CERT_FILE]). -configure_c2s_listener(Config, ExtraC2SOpts) -> +configure_c2s_listener(Config, ExtraC2sOpts) -> + configure_c2s_listener(Config, ExtraC2sOpts, []). + +configure_c2s_listener(Config, ExtraC2SOpts, RemovedC2SKeys) -> C2SListener = ?config(c2s_listener, Config), - NewC2SListener = maps:merge(C2SListener, ExtraC2SOpts), + NewC2SListener = maps:without(RemovedC2SKeys, maps:merge(C2SListener, ExtraC2SOpts)), ct:pal("C2S listener: ~p", [NewC2SListener]), mongoose_helper:restart_listener(mim(), NewC2SListener). From 7a085cb3ee469743fcdcad5d2c34e151d1129e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 13 Oct 2023 18:31:09 +0200 Subject: [PATCH 2/5] Parallelize more tests in connect_SUITE --- big_tests/tests/connect_SUITE.erl | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/big_tests/tests/connect_SUITE.erl b/big_tests/tests/connect_SUITE.erl index 4c0a9beb7a2..600dc8e84aa 100644 --- a/big_tests/tests/connect_SUITE.erl +++ b/big_tests/tests/connect_SUITE.erl @@ -64,17 +64,17 @@ groups() -> invalid_stream_namespace, deny_pre_xmpp_1_0_stream, correct_features_are_advertised_for_optional_starttls]}, - {starttls_required, [], [{group, feature_order}, - metrics_test, - should_fail_to_authenticate_without_starttls, - auth_bind_pipelined_starttls_skipped_error | protocol_test_cases()]}, + {starttls_required, [], [{group, starttls_required_parallel}, metrics_test]}, + {starttls_required_parallel, [parallel], [correct_features_are_advertised_for_required_starttls, + tls_authenticate, + bind_server_generated_resource, + cannot_connect_with_proxy_header, + should_fail_to_authenticate_without_starttls, + auth_bind_pipelined_starttls_skipped_error + | protocol_test_cases()]}, {tls, [parallel], auth_bind_pipelined_cases() ++ protocol_test_cases() ++ cipher_test_cases()}, - {feature_order, [parallel], [correct_features_are_advertised_for_required_starttls, - tls_authenticate, - bind_server_generated_resource, - cannot_connect_with_proxy_header]}, {just_tls, tls_groups()}, {fast_tls, tls_groups()}, {session_replacement, [], [same_resource_replaces_session, @@ -298,9 +298,10 @@ should_pass_with_tlsv1_2(Config) -> should_fail_to_authenticate_without_starttls(Config) -> %% GIVEN - UserSpec = escalus_users:get_userspec(Config, ?SECURE_USER), + UserSpec = escalus_fresh:freshen_spec(Config, ?SECURE_USER), ConnectionSteps = [start_stream, stream_features], {ok, Conn, Features} = escalus_connection:start(UserSpec, ConnectionSteps), + UserName = escalus_utils:get_username(Conn), %% WHEN try escalus_session:authenticate(Conn, Features) of @@ -309,7 +310,7 @@ should_fail_to_authenticate_without_starttls(Config) -> error(authentication_without_tls_suceeded) catch throw:{auth_failed, User, AuthReply} -> - ?assertEqual(atom_to_binary(?SECURE_USER, utf8), User), + ?assertEqual(UserName, User), escalus:assert(is_stream_error, [<<"policy-violation">>, <<"Use of STARTTLS required">>], AuthReply) From 0c4cc3e5556837e0c8a4b138f2a1d81ab71ee627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 13 Oct 2023 12:51:24 +0200 Subject: [PATCH 3/5] Advertise STARTTLS only when enabled (no TLS section = disabled) --- src/c2s/mongoose_c2s_stanzas.erl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/c2s/mongoose_c2s_stanzas.erl b/src/c2s/mongoose_c2s_stanzas.erl index 12ffe63b8e2..bd01806ae6d 100644 --- a/src/c2s/mongoose_c2s_stanzas.erl +++ b/src/c2s/mongoose_c2s_stanzas.erl @@ -57,12 +57,12 @@ stream_features_before_auth(StateData) -> %% http://xmpp.org/rfcs/rfc6120.html#tls-rules-mtn determine_features(_, _, _, #{tls := #{mode := starttls_required}}, false) -> [starttls_stanza(required)]; -determine_features(StateData, HostType, LServer, _, true) -> - InitialFeatures = maybe_sasl_mechanisms(StateData), +determine_features(StateData, HostType, LServer, #{tls := #{mode := starttls}}, false) -> + InitialFeatures = [starttls_stanza(optional) | maybe_sasl_mechanisms(StateData)], StreamFeaturesParams = #{c2s_data => StateData, lserver => LServer}, mongoose_hooks:c2s_stream_features(HostType, StreamFeaturesParams, InitialFeatures); determine_features(StateData, HostType, LServer, _, _) -> - InitialFeatures = [starttls_stanza(optional) | maybe_sasl_mechanisms(StateData)], + InitialFeatures = maybe_sasl_mechanisms(StateData), StreamFeaturesParams = #{c2s_data => StateData, lserver => LServer}, mongoose_hooks:c2s_stream_features(HostType, StreamFeaturesParams, InitialFeatures). From 37706b52dbcb563c041e9816f178afe98f8e0f1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 13 Oct 2023 12:53:56 +0200 Subject: [PATCH 4/5] Do not accept STARTTLS if it is disabled Previously STARTTLS would be attempted, resulting in a crash. Now the result is a failure, as defined in RFC 6120, section 5.4.2.2 --- src/c2s/mongoose_c2s.erl | 8 ++++++-- src/c2s/mongoose_c2s_stanzas.erl | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/c2s/mongoose_c2s.erl b/src/c2s/mongoose_c2s.erl index 953b957cf42..f45532bf4a2 100644 --- a/src/c2s/mongoose_c2s.erl +++ b/src/c2s/mongoose_c2s.erl @@ -377,7 +377,7 @@ get_xml_lang(StreamStart) -> -spec handle_starttls(data(), exml:element(), mongoose_acc:t(), retries()) -> fsm_res(). handle_starttls(StateData = #c2s_data{socket = TcpSocket, parser = Parser, - listener_opts = LOpts}, El, SaslAcc, Retries) -> + listener_opts = LOpts = #{tls := _}}, El, SaslAcc, Retries) -> send_xml(StateData, mongoose_c2s_stanzas:tls_proceed()), %% send last negotiation chunk via tcp case mongoose_c2s_socket:tcp_to_tls(TcpSocket, LOpts) of {ok, TlsSocket} -> @@ -398,7 +398,11 @@ handle_starttls(StateData = #c2s_data{socket = TcpSocket, {stop, {shutdown, tls_timeout}}; {error, {tls_alert, TlsAlert}} -> {stop, TlsAlert} - end. + end; +handle_starttls(StateData, _El, _SaslAcc, _Retries) -> + %% As defined in https://datatracker.ietf.org/doc/html/rfc6120#section-5.4.2.2, cause 2 + send_element_from_server_jid(StateData, mongoose_c2s_stanzas:tls_failure()), + {stop, {shutdown, tls_failure}}. -spec handle_auth_start(data(), exml:element(), mongoose_acc:t(), retries()) -> fsm_res(). handle_auth_start(StateData, El, SaslAcc, Retries) -> diff --git a/src/c2s/mongoose_c2s_stanzas.erl b/src/c2s/mongoose_c2s_stanzas.erl index bd01806ae6d..88ca4d38efb 100644 --- a/src/c2s/mongoose_c2s_stanzas.erl +++ b/src/c2s/mongoose_c2s_stanzas.erl @@ -8,6 +8,7 @@ stream_header/1, stream_features_before_auth/1, tls_proceed/0, + tls_failure/0, stream_features_after_auth/1, sasl_success_stanza/1, sasl_failure_stanza/1, @@ -91,6 +92,11 @@ tls_proceed() -> #xmlel{name = <<"proceed">>, attrs = [{<<"xmlns">>, ?NS_TLS}]}. +-spec tls_failure() -> exml:element(). +tls_failure() -> + #xmlel{name = <<"failure">>, + attrs = [{<<"xmlns">>, ?NS_TLS}]}. + -spec stream_features_after_auth(mongoose_c2s:data()) -> exml:element(). stream_features_after_auth(StateData) -> case mongoose_c2s:get_listener_opts(StateData) of From ebe60bb1d5b16df751be8021f3dcdef1a493ace1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chrz=C4=85szcz?= Date: Fri, 13 Oct 2023 16:25:24 +0200 Subject: [PATCH 5/5] Make docs for TLS more obvious --- doc/configuration/listen.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/configuration/listen.md b/doc/configuration/listen.md index 68f62d58321..dcaadbf35bf 100644 --- a/doc/configuration/listen.md +++ b/doc/configuration/listen.md @@ -163,8 +163,8 @@ Same syntax as for `auth.methods` option. ## TLS options for C2S -By default the C2S listener does not use TLS. -To use TLS, you need to add a TOML subsection called `tls` to the listener options. +To enable TLS, a TOML subsection called `tls` has to be present in the listener options. +To disable TLS, make sure that the section is not present, and no TLS options are set. You can set the following options in this section: ### `listen.c2s.tls.mode`