Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Listeners — Remove fast_tls #4458

Merged
merged 13 commits into from
Jan 14, 2025
Merged

Listeners — Remove fast_tls #4458

merged 13 commits into from
Jan 14, 2025

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Jan 9, 2025

With this, we unify the configuration for XMPP listeners, that is, c2s, s2s, and components, by a lot of keys. This also reworks the s2s section, where we were setting keys common to incoming and outgoing connections like use_starttls or certfile. Now outgoing connections are configured in the [HostType.]s2s section, and incoming connections in the listener.s2s section, and both set their TLS configuration independently in the same way c2s or components do. We also remove the possibility of configuring S2S without TLS nor starttls; rather, as C2S does, we only allow the configuration to have starttls as optional, that is enough to use plaintext if desired. However plaintext connections should probably never ever be used.

just_tls is still a wrapper around SSL connections because of the verify_fun key, which uses some adhoc message passing. It would be good to rework that some day in the future for further simplification. However now mongoose_tls is not needed and was merged into just_tls.

Global distribution was using fast_tls for both incoming and outgoing connections too, so it needed as well some further rework.

Also implement TLSv1.3 tls-exporter channel binding mech, this is also done in escalus in esl/escalus#274.

@mongoose-im

This comment was marked as outdated.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 88.13559% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.15%. Comparing base (7ca81f8) to head (2a270c4).
Report is 19 commits behind head on feature/listeners.

Files with missing lines Patch % Lines
src/s2s/mongoose_transport.erl 76.47% 4 Missing ⚠️
src/c2s/mongoose_c2s_ranch.erl 71.42% 2 Missing ⚠️
src/sasl/cyrsasl_scram.erl 75.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           feature/listeners    #4458      +/-   ##
=====================================================
- Coverage              85.40%   85.15%   -0.26%     
=====================================================
  Files                    554      553       -1     
  Lines                  34021    33910     -111     
=====================================================
- Hits                   29056    28875     -181     
- Misses                  4965     5035      +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NelsonVides NelsonVides force-pushed the listeners/ssl branch 2 times, most recently from 7f46cdd to 398ebce Compare January 9, 2025 15:26
@mongoose-im

This comment was marked as outdated.

@NelsonVides NelsonVides force-pushed the listeners/ssl branch 2 times, most recently from bab9e06 to 59a9eb0 Compare January 9, 2025 15:50
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

Base automatically changed from s2s/listeners to feature/listeners January 14, 2025 08:47
@NelsonVides NelsonVides marked this pull request as ready for review January 14, 2025 08:48
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uff, added a couple of comments

{noreply, NewState, hibernate_or_timeout(NewState)};
handle_info(TcpOrSslInfo, State)
when is_tuple(TcpOrSslInfo) andalso
tcp_closed =:= element(1, TcpOrSslInfo) orelse tcp_error =:= element(1, TcpOrSslInfo) orelse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

element(1,X) kinda ugly, could use ; instead of orelse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll rework this anyway when we rework S2S so I wouldn't take it too serious, but more or less. element/2 is so that I can catch al the tcp|ssl_closed|error tuples in a single clause, these things are all treated the same way by the current implementation.

-spec get_tls_last_message(socket()) -> {ok, binary()} | {error, term()}.
get_tls_last_message(_Socket) ->
-spec export_key_materials(socket(), _, _, _, _) -> {error, term()}.
export_key_materials(_Socket, _, _, _, _) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having names instead of _ is not a bad idea ;)

do_setopts_and_receive_data(Socket, Buffer, RawData, State);
handle_info({tcp_closed, _Socket}, State) ->
handle_info({Tag, _Socket}, State) when Tag == tcp_closed; Tag == ssl_closed ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here with explicit tuples is better, than with element(1, X)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because this case is treating error from close differently, and the size of the tuples are different. Anyway, can fix the other one 👌🏽

@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 14, 2025

elasticsearch_and_cassandra_27 / elasticsearch_and_cassandra_mnesia / 2a270c4
Reports root/ big
OK: 472 / Failed: 0 / User-skipped: 49 / Auto-skipped: 0


small_tests_26 / small_tests / 2a270c4
Reports root / small


small_tests_27 / small_tests / 2a270c4
Reports root / small


small_tests_27_arm64 / small_tests / 2a270c4
Reports root / small


ldap_mnesia_26 / ldap_mnesia / 2a270c4
Reports root/ big
OK: 2294 / Failed: 1 / User-skipped: 887 / Auto-skipped: 0

pubsub_SUITE:tree+basic:create_node_errors_test
{error,{{badmatch,false},
    [{pubsub_tools,check_response,2,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,444}]},
     {pubsub_tools,receive_response,3,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,434}]},
     {pubsub_tools,receive_and_check_response,4,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,424}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1793}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1302}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1234}]}]}}

Report log


ldap_mnesia_27 / ldap_mnesia / 2a270c4
Reports root/ big
OK: 2295 / Failed: 0 / User-skipped: 902 / Auto-skipped: 0


dynamic_domains_mysql_redis_27 / mysql_redis / 2a270c4
Reports root/ big
OK: 4675 / Failed: 0 / User-skipped: 152 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 2a270c4
Reports root/ big
OK: 4695 / Failed: 0 / User-skipped: 117 / Auto-skipped: 0


internal_mnesia_27 / internal_mnesia / 2a270c4
Reports root/ big
OK: 2428 / Failed: 1 / User-skipped: 768 / Auto-skipped: 0

pubsub_SUITE:dag+basic:request_particular_item_test
{error,{{badmatch,false},
    [{pubsub_tools,check_response,2,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,444}]},
     {pubsub_tools,receive_response,3,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,434}]},
     {pubsub_tools,receive_and_check_response,4,
             [{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
            {line,424}]},
     {escalus_story,story,4,
            [{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
             {line,72}]},
     {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1794}]},
     {test_server,run_test_case_eval1,6,
            [{file,"test_server.erl"},{line,1303}]},
     {test_server,run_test_case_eval,9,
            [{file,"test_server.erl"},{line,1235}]}]}}

Report log


dynamic_domains_pgsql_mnesia_27 / pgsql_mnesia / 2a270c4
Reports root/ big
OK: 4710 / Failed: 0 / User-skipped: 117 / Auto-skipped: 0


pgsql_cets_27 / pgsql_cets / 2a270c4
Reports root/ big
OK: 4784 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0


pgsql_mnesia_26 / pgsql_mnesia / 2a270c4
Reports root/ big
OK: 5070 / Failed: 0 / User-skipped: 126 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_27 / odbc_mssql_mnesia / 2a270c4
Reports root/ big
OK: 4705 / Failed: 0 / User-skipped: 122 / Auto-skipped: 0


mysql_redis_27 / mysql_redis / 2a270c4
Reports root/ big
OK: 5064 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


cockroachdb_cets_27 / cockroachdb_cets / 2a270c4
Reports root/ big
OK: 4783 / Failed: 1 / User-skipped: 186 / Auto-skipped: 0

mod_ping_SUITE:server_ping_kill:server_ping_pong
{error,{test_case_failed,"Incorrect number of instrumentation events - matched: 0, expected: 1"}}

Report log


pgsql_mnesia_27 / pgsql_mnesia / 2a270c4
Reports root/ big
OK: 5085 / Failed: 0 / User-skipped: 126 / Auto-skipped: 0


mssql_mnesia_27 / odbc_mssql_mnesia / 2a270c4
Reports root/ big
OK: 5080 / Failed: 0 / User-skipped: 131 / Auto-skipped: 0


internal_mnesia_27 / internal_mnesia / 2a270c4
Reports root/ big
OK: 117 / Failed: 0 / User-skipped: 2 / Auto-skipped: 0


cockroachdb_cets_27 / cockroachdb_cets / 2a270c4
Reports root/ big
OK: 18 / Failed: 0 / User-skipped: 0 / Auto-skipped: 0


ldap_mnesia_26 / ldap_mnesia / 2a270c4
Reports root/ big
OK: 117 / Failed: 0 / User-skipped: 2 / Auto-skipped: 0

@arcusfelis arcusfelis merged commit 0b0083b into feature/listeners Jan 14, 2025
4 checks passed
@arcusfelis arcusfelis deleted the listeners/ssl branch January 14, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants