Merge pull request #11028 from id/0612-fix-incompatible-tls-options-and-version-gap

fix(tls): fix incompatible tls options and issue with tls version gap

apps/emqx/rebar.config

@@ -25,7 +25,7 @@
     {emqx_utils, {path, "../emqx_utils"}},
     {lc, {git, "https://github.com/emqx/lc.git", {tag, "0.3.2"}}},
     {gproc, {git, "https://github.com/emqx/gproc", {tag, "0.9.0.1"}}},
-    {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.0"}}},
+    {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.2"}}},
     {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.9.6"}}},
     {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.15.2"}}},
     {gen_rpc, {git, "https://github.com/emqx/gen_rpc", {tag, "2.8.1"}}},

apps/emqx/src/emqx_schema.erl

@@ -2679,10 +2679,30 @@ validate_ciphers(Ciphers) ->
 validate_tls_versions(Collection, Versions) ->
     AvailableVersions = available_tls_vsns(Collection),
     case lists:filter(fun(V) -> not lists:member(V, AvailableVersions) end, Versions) of
-        [] -> ok;
+        [] -> validate_tls_version_gap(Versions);
         Vs -> {error, {unsupported_tls_versions, Vs}}
     end.
 
+%% See also `validate_version_gap/1` in OTP ssl.erl,
+%% e.g: https://github.com/emqx/otp/blob/emqx-OTP-25.1.2/lib/ssl/src/ssl.erl#L2566.
+%% Do not allow configuration of TLS 1.3 with a gap where TLS 1.2 is not supported
+%% as that configuration can trigger the built in version downgrade protection
+%% mechanism and the handshake can fail with an Illegal Parameter alert.
+validate_tls_version_gap(Versions) ->
+    case lists:member('tlsv1.3', Versions) of
+        true when length(Versions) >= 2 ->
+            case lists:member('tlsv1.2', Versions) of
+                true ->
+                    ok;
+                false ->
+                    {error,
+                        "Using multiple versions that include tlsv1.3 but "
+                        "exclude tlsv1.2 is not allowed"}
+            end;
+        _ ->
+            ok
+    end.
+
 validations() ->
     [
         {check_process_watermark, fun check_process_watermark/1},

apps/emqx/src/emqx_tls_lib.erl

@@ -478,7 +478,7 @@ to_server_opts(Type, Opts) ->
     Versions = integral_versions(Type, maps:get(versions, Opts, undefined)),
     Ciphers = integral_ciphers(Versions, maps:get(ciphers, Opts, undefined)),
     Path = fun(Key) -> resolve_cert_path_for_read_strict(maps:get(Key, Opts, undefined)) end,
-    filter(
+    ensure_valid_options(
         maps:to_list(Opts#{
             keyfile => Path(keyfile),
             certfile => Path(certfile),
@@ -511,7 +511,7 @@ 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(
+            ensure_valid_options(
                 [
                     {keyfile, KeyFile},
                     {certfile, CertFile},
@@ -556,33 +556,72 @@ resolve_cert_path_for_read_strict(Path) ->
 resolve_cert_path_for_read(Path) ->
     emqx_schema:naive_env_interpolation(Path).
 
-filter([], _) ->
-    [];
-filter([{_, undefined} | T], Versions) ->
-    filter(T, Versions);
-filter([{_, ""} | T], Versions) ->
-    filter(T, Versions);
-filter([{K, V} | T], Versions) ->
+ensure_valid_options(Options, Versions) ->
+    ensure_valid_options(Options, Versions, []).
+
+ensure_valid_options([], _, Acc) ->
+    lists:reverse(Acc);
+ensure_valid_options([{_, undefined} | T], Versions, Acc) ->
+    ensure_valid_options(T, Versions, Acc);
+ensure_valid_options([{_, ""} | T], Versions, Acc) ->
+    ensure_valid_options(T, Versions, Acc);
+ensure_valid_options([{K, V} | T], Versions, Acc) ->
     case tls_option_compatible_versions(K) of
         all ->
-            [{K, V} | filter(T, Versions)];
+            ensure_valid_options(T, Versions, [{K, V} | Acc]);
         CompatibleVersions ->
-            case CompatibleVersions -- (CompatibleVersions -- Versions) of
-                [] ->
-                    filter(T, Versions);
-                _ ->
-                    [{K, V} | filter(T, Versions)]
+            Enabled = sets:from_list(Versions),
+            Compatible = sets:from_list(CompatibleVersions),
+            case sets:size(sets:intersection(Enabled, Compatible)) > 0 of
+                true ->
+                    ensure_valid_options(T, Versions, [{K, V} | Acc]);
+                false ->
+                    ?SLOG(warning, #{
+                        msg => "drop_incompatible_tls_option", option => K, versions => Versions
+                    }),
+                    ensure_valid_options(T, Versions, Acc)
             end
     end.
 
+%% see otp/lib/ssl/src/ssl.erl, `assert_option_dependency/4`
+tls_option_compatible_versions(beast_mitigation) ->
+    [dtlsv1, 'tlsv1'];
+tls_option_compatible_versions(padding_check) ->
+    [dtlsv1, 'tlsv1'];
+tls_option_compatible_versions(client_renegotiation) ->
+    [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
+tls_option_compatible_versions(reuse_session) ->
+    [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
 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) ->
+tls_option_compatible_versions(next_protocol_advertised) ->
     [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
-tls_option_compatible_versions(client_renegotiation) ->
+tls_option_compatible_versions(client_preferred_next_protocols) ->
+    [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
+tls_option_compatible_versions(psk_identity) ->
+    [dtlsv1, 'dtlsv1.2', 'tlsv1', 'tlsv1.1', 'tlsv1.2'];
+tls_option_compatible_versions(srp_identity) ->
+    [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(early_data) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(certificate_authorities) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(cookie) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(key_update_at) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(anti_replay) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(session_tickets) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(supported_groups) ->
+    ['tlsv1.3'];
+tls_option_compatible_versions(use_ticket) ->
+    ['tlsv1.3'];
 tls_option_compatible_versions(_) ->
     all.
 

apps/emqx/test/emqx_schema_tests.erl

@@ -94,6 +94,18 @@ ssl_opts_tls_psk_test() ->
     Checked = validate(Sc, #{<<"versions">> => [<<"tlsv1.2">>]}),
     ?assertMatch(#{versions := ['tlsv1.2']}, Checked).
 
+ssl_opts_version_gap_test_() ->
+    Sc = emqx_schema:server_ssl_opts_schema(#{}, false),
+    RanchSc = emqx_schema:server_ssl_opts_schema(#{}, true),
+    Reason = "Using multiple versions that include tlsv1.3 but exclude tlsv1.2 is not allowed",
+    [
+        ?_assertThrow(
+            {_, [#{kind := validation_error, reason := Reason}]},
+            validate(S, #{<<"versions">> => [<<"tlsv1.1">>, <<"tlsv1.3">>]})
+        )
+     || S <- [Sc, RanchSc]
+    ].
+
 bad_cipher_test() ->
     Sc = emqx_schema:server_ssl_opts_schema(#{}, false),
     Reason = {bad_ciphers, ["foo"]},

changes/ce/fix-11028.en.md

@@ -0,0 +1,7 @@
+Disallow using multiple TLS versions in the listener config that include tlsv1.3 but exclude tlsv1.2.
+
+Using TLS configuration with such version gap caused connection errors.
+Additionally, drop and log TLS options that are incompatible with the selected TLS version(s).
+
+Note: any old listener configuration with the version gap described above will fail to load
+after applying this fix and must be manually fixed.

mix.exs

@@ -52,7 +52,7 @@ defmodule EMQXUmbrella.MixProject do
       {:ehttpc, github: "emqx/ehttpc", tag: "0.4.10", override: true},
       {:gproc, github: "emqx/gproc", tag: "0.9.0.1", override: true},
       {:jiffy, github: "emqx/jiffy", tag: "1.0.5", override: true},
-      {:cowboy, github: "emqx/cowboy", tag: "2.9.0", override: true},
+      {:cowboy, github: "emqx/cowboy", tag: "2.9.2", override: true},
       {:esockd, github: "emqx/esockd", tag: "5.9.6", override: true},
       {:rocksdb, github: "emqx/erlang-rocksdb", tag: "1.7.2-emqx-11", override: true},
       {:ekka, github: "emqx/ekka", tag: "0.15.2", override: true},
@@ -92,7 +92,7 @@ defmodule EMQXUmbrella.MixProject do
        github: "ninenines/cowlib", ref: "c6553f8308a2ca5dcd69d845f0a7d098c40c3363", override: true},
       # in conflict by cowboy_swagger and cowboy
       {:ranch,
-       github: "ninenines/ranch", ref: "a692f44567034dacf5efcaa24a24183788594eb7", override: true},
+       github: "emqx/ranch", ref: "de8ba2a00817c0a6eb1b8f20d6fb3e44e2c9a5aa", override: true},
       # in conflict by grpc and eetcd
       {:gpb, "4.19.7", override: true, runtime: false},
       {:hackney, github: "emqx/hackney", tag: "1.18.1-1", override: true}

rebar.config

@@ -59,7 +59,7 @@
     , {ehttpc, {git, "https://github.com/emqx/ehttpc", {tag, "0.4.10"}}}
     , {gproc, {git, "https://github.com/emqx/gproc", {tag, "0.9.0.1"}}}
     , {jiffy, {git, "https://github.com/emqx/jiffy", {tag, "1.0.5"}}}
-    , {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.0"}}}
+    , {cowboy, {git, "https://github.com/emqx/cowboy", {tag, "2.9.2"}}}
     , {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.9.6"}}}
     , {rocksdb, {git, "https://github.com/emqx/erlang-rocksdb", {tag, "1.7.2-emqx-11"}}}
     , {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.15.2"}}}