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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(tls): issue when ssl listner is configured to use tls v1.3 only #10983

Merged
merged 3 commits into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 47 additions & 18 deletions apps/emqx/src/emqx_tls_lib.erl
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,8 @@ to_server_opts(Type, Opts) ->
cacertfile => Path(cacertfile),
ciphers => Ciphers,
versions => Versions
})
}),
Versions
).

%% @doc Convert hocon-checked tls client options (map()) to
Expand All @@ -510,19 +511,22 @@ to_client_opts(Type, Opts) ->
SNI = ensure_sni(Get(server_name_indication)),
Versions = integral_versions(Type, Get(versions)),
Ciphers = integral_ciphers(Versions, Get(ciphers)),
filter([
{keyfile, KeyFile},
{certfile, CertFile},
{cacertfile, CAFile},
{verify, Verify},
{server_name_indication, SNI},
{versions, Versions},
{ciphers, Ciphers},
{reuse_sessions, Get(reuse_sessions)},
{depth, Get(depth)},
{password, ensure_str(Get(password))},
{secure_renegotiate, Get(secure_renegotiate)}
]);
filter(
[
{keyfile, KeyFile},
{certfile, CertFile},
{cacertfile, CAFile},
{verify, Verify},
{server_name_indication, SNI},
{versions, Versions},
{ciphers, Ciphers},
{reuse_sessions, Get(reuse_sessions)},
{depth, Get(depth)},
{password, ensure_str(Get(password))},
{secure_renegotiate, Get(secure_renegotiate)}
],
Versions
);
false ->
[]
end.
Expand Down Expand Up @@ -552,10 +556,35 @@ resolve_cert_path_for_read_strict(Path) ->
resolve_cert_path_for_read(Path) ->
emqx_schema:naive_env_interpolation(Path).

filter([]) -> [];
filter([{_, undefined} | T]) -> filter(T);
filter([{_, ""} | T]) -> filter(T);
filter([H | T]) -> [H | filter(T)].
filter([], _) ->
[];
filter([{_, undefined} | T], Versions) ->
filter(T, Versions);
filter([{_, ""} | T], Versions) ->
filter(T, Versions);
filter([{K, V} | T], Versions) ->
case tls_option_compatible_versions(K) of
all ->
[{K, V} | filter(T, Versions)];
CompatibleVersions ->
case CompatibleVersions -- (CompatibleVersions -- Versions) of
[] ->
filter(T, Versions);
_ ->
[{K, V} | filter(T, Versions)]
end
end.

tls_option_compatible_versions(reuse_sessions) ->
[dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
tls_option_compatible_versions(secure_renegotiate) ->
[dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
tls_option_compatible_versions(user_lookup_fun) ->
[dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
tls_option_compatible_versions(client_renegotiation) ->
[dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
tls_option_compatible_versions(_) ->
all.

-spec fuzzy_map_get(atom() | binary(), map(), any()) -> any().
fuzzy_map_get(Key, Options, Default) ->
Expand Down
65 changes: 65 additions & 0 deletions apps/emqx/test/emqx_tls_lib_tests.erl
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,71 @@ ssl_file_deterministic_names_test() ->
),
_ = file:del_dir_r(filename:join(["/tmp", ?FUNCTION_NAME])).

to_client_opts_test() ->
VersionsAll = [tlsv1, 'tlsv1.1', 'tlsv1.2', 'tlsv1.3'],
Versions13Only = ['tlsv1.3'],
Options = #{
enable => true,
verify => "Verify",
server_name_indication => "SNI",
ciphers => "Ciphers",
depth => "depth",
password => "password",
versions => VersionsAll,
secure_renegotiate => "secure_renegotiate",
reuse_sessions => "reuse_sessions"
},
Expected1 = lists:usort(maps:keys(Options) -- [enable]),
?assertEqual(
Expected1, lists:usort(proplists:get_keys(emqx_tls_lib:to_client_opts(tls, Options)))
),
Expected2 =
lists:usort(
maps:keys(Options) --
[enable, reuse_sessions, secure_renegotiate]
),
?assertEqual(
Expected2,
lists:usort(
proplists:get_keys(
emqx_tls_lib:to_client_opts(tls, Options#{versions := Versions13Only})
)
)
),
Expected3 = lists:usort(maps:keys(Options) -- [enable, depth, password]),
?assertEqual(
Expected3,
lists:usort(
proplists:get_keys(
emqx_tls_lib:to_client_opts(tls, Options#{depth := undefined, password := ""})
)
)
).

to_server_opts_test() ->
VersionsAll = [tlsv1, 'tlsv1.1', 'tlsv1.2', 'tlsv1.3'],
Versions13Only = ['tlsv1.3'],
Options = #{
verify => "Verify",
ciphers => "Ciphers",
versions => VersionsAll,
user_lookup_fun => "funfunfun",
client_renegotiation => "client_renegotiation"
},
Expected1 = lists:usort(maps:keys(Options)),
?assertEqual(
Expected1, lists:usort(proplists:get_keys(emqx_tls_lib:to_server_opts(tls, Options)))
),
Expected2 = lists:usort(maps:keys(Options) -- [user_lookup_fun, client_renegotiation]),
?assertEqual(
Expected2,
lists:usort(
proplists:get_keys(
emqx_tls_lib:to_server_opts(tls, Options#{versions := Versions13Only})
)
)
).

bin(X) -> iolist_to_binary(X).

test_key() ->
Expand Down
3 changes: 3 additions & 0 deletions changes/ce/fix-10983.en.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix issue when mqtt clients could not connect over TLS if the listener was configured to use TLS v1.3 only.

The problem was that TLS connection was trying to use options incompatible with TLS v1.3.
12 changes: 8 additions & 4 deletions rel/i18n/emqx_schema.hocon
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ mqtt_max_topic_alias.label:
"""Max Topic Alias"""

common_ssl_opts_schema_user_lookup_fun.desc:
"""EMQX-internal callback that is used to lookup pre-shared key (PSK) identity."""
"""EMQX-internal callback that is used to lookup pre-shared key (PSK) identity.</br>
Has no effect when TLS version is configured (or negotiated) to 1.3"""

common_ssl_opts_schema_user_lookup_fun.label:
"""SSL PSK user lookup fun"""
Expand Down Expand Up @@ -1240,7 +1241,8 @@ The SSL application already takes measures to counter-act such attempts,
but client-initiated renegotiation can be strictly disabled by setting this option to false.
The default value is true. Note that disabling renegotiation can result in
long-lived connections becoming unusable due to limits on
the number of messages the underlying cipher suite can encipher."""
the number of messages the underlying cipher suite can encipher.</br>
Has no effect when TLS version is configured (or negotiated) to 1.3"""

server_ssl_opts_schema_client_renegotiation.label:
"""SSL client renegotiation"""
Expand Down Expand Up @@ -1326,7 +1328,8 @@ common_ssl_opts_schema_secure_renegotiate.desc:
"""SSL parameter renegotiation is a feature that allows a client and a server
to renegotiate the parameters of the SSL connection on the fly.
RFC 5746 defines a more secure way of doing this. By enabling secure renegotiation,
you drop support for the insecure renegotiation, prone to MitM attacks."""
you drop support for the insecure renegotiation, prone to MitM attacks.</br>
Has no effect when TLS version is configured (or negotiated) to 1.3"""

common_ssl_opts_schema_secure_renegotiate.label:
"""SSL renegotiate"""
Expand Down Expand Up @@ -1361,7 +1364,8 @@ mqtt_max_packet_size.label:
"""Max Packet Size"""

common_ssl_opts_schema_reuse_sessions.desc:
"""Enable TLS session reuse."""
"""Enable TLS session reuse.</br>
Has no effect when TLS version is configured (or negotiated) to 1.3"""

common_ssl_opts_schema_reuse_sessions.label:
"""TLS session reuse"""
Expand Down