Correctly handle the case when TLS is disabled #4150
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4150 +/- ##
=======================================
Coverage 84.05% 84.05%
=======================================
Files 561 561
Lines 34050 34053 +3
=======================================
+ Hits 28620 28623 +3
Misses 5430 5430
☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
fix-starttls-disabled
branch
from
f0e8786
to
27e1ce1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
fix-starttls-disabled
branch
from
27e1ce1
to
5ea5574
Compare
This comment was marked as outdated.
This comment was marked as outdated.
chrzaszcz
force-pushed
the
fix-starttls-disabled
branch
from
5ea5574
to
68c2714
Compare
This comment was marked as outdated.
This comment was marked as outdated.
- 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
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
chrzaszcz
force-pushed
the
fix-starttls-disabled
branch
from
d9afeaa
to
ebe60bb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
elasticsearch_and_cassandra_26 / elasticsearch_and_cassandra_mnesia / ebe60bb small_tests_25 / small_tests / ebe60bb small_tests_26_arm64 / small_tests / ebe60bb small_tests_26 / small_tests / ebe60bb ldap_mnesia_25 / ldap_mnesia / ebe60bb dynamic_domains_mysql_redis_26 / mysql_redis / ebe60bb carboncopy_SUITE:one2one:dropped_client_doesnt_create_duplicate_carbons{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_585@domain.example.com">>},
{<<"to">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_585@domain.example.com/res2">>},
{<<"xmlns">>,<<"jabber:client">>},
{<<"type">>,<<"chat">>}],
[{xmlel,<<"sent">>,
[{<<"xmlns">>,<<"urn:xmpp:carbons:2">>}],
[{xmlel,<<"forwarded">>,
[{<<"xmlns">>,<<"urn:xmpp:forward:0">>}],
[{xmlel,<<"message">>,
[{<<"from">>,
<<"alice_dropped_client_doesnt_create_duplicate_carbons_585@domain.example.com/res1">>},
{<<"type">>,<<"chat">>},
{<<"to">>,
<<"bob_dropped_client_doesnt_create_duplicate_carbons_585@domain.example.com/res1">>},
{<<"xmlns">>,<<"jabber:client">>}],
[{xmlel,<<"body">>,[],
[{xmlcdata,
<<"And pious action">>}]}]}]}]}]}]},
[{carboncopy_SUITE,
'-dropped_client_doesnt_create_duplicate_carbons/1-fun-0-',4,
[{file,
"/home/circleci/project/big_tests/tests/carboncopy_SUITE.erl"},
{line,189}]},
{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_c...dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / ebe60bb ldap_mnesia_26 / ldap_mnesia / ebe60bb dynamic_domains_mssql_mnesia_26 / odbc_mssql_mnesia / ebe60bb dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / ebe60bb internal_mnesia_26 / internal_mnesia / ebe60bb pgsql_cets_26 / pgsql_cets / ebe60bb pgsql_mnesia_25 / pgsql_mnesia / ebe60bb mysql_redis_26 / mysql_redis / ebe60bb pgsql_mnesia_26 / pgsql_mnesia / ebe60bb mssql_mnesia_26 / odbc_mssql_mnesia / ebe60bb dynamic_domains_mysql_redis_26 / mysql_redis / ebe60bb |
When the TLS section is missing, then according to the documentation STARTTLS should be rejected. This was not the case: the feature was advertised, and a TLS upgrade performed by a client (who was informed about the support) resulted in a crash.
This PR fixes these issues:
The tests are updated to check these conditions, and to correctly verify the features for optional and required STARTTLS as well.
The bugs were discovered accidentally when checking esl/mongooseim-docker#42. The statement about TLS disabled by default is changed in the docs, because it most likely confused the reporter of that issue.