diff --git a/apps/emqx/src/emqx_listeners.erl b/apps/emqx/src/emqx_listeners.erl index e325263c5..32599b3be 100644 --- a/apps/emqx/src/emqx_listeners.erl +++ b/apps/emqx/src/emqx_listeners.erl @@ -421,7 +421,7 @@ do_start_listener(Type, Name, Id, #{bind := ListenOn} = Opts) when ?ESOCKD_LISTE esockd:open( Id, ListenOn, - merge_default(esockd_opts(Id, Type, Name, Opts)) + merge_default(esockd_opts(Id, Type, Name, Opts, _OldOpts = undefined)) ); %% Start MQTT/WS listener do_start_listener(Type, Name, Id, Opts) when ?COWBOY_LISTENER(Type) -> @@ -465,7 +465,7 @@ do_update_listener(Type, Name, OldConf, NewConf = #{bind := ListenOn}) when Id = listener_id(Type, Name), case maps:get(bind, OldConf) of ListenOn -> - esockd:set_options({Id, ListenOn}, esockd_opts(Id, Type, Name, NewConf)); + esockd:set_options({Id, ListenOn}, esockd_opts(Id, Type, Name, NewConf, OldConf)); _Different -> %% TODO %% Again, we're not strictly required to drop live connections in this case. @@ -577,7 +577,7 @@ perform_listener_change(update, {{Type, Name, ConfOld}, {_, _, ConfNew}}) -> perform_listener_change(stop, {Type, Name, Conf}) -> stop_listener(Type, Name, Conf). -esockd_opts(ListenerId, Type, Name, Opts0) -> +esockd_opts(ListenerId, Type, Name, Opts0, OldOpts) -> Opts1 = maps:with([acceptors, max_connections, proxy_protocol, proxy_protocol_timeout], Opts0), Limiter = limiter(Opts0), Opts2 = @@ -609,7 +609,7 @@ esockd_opts(ListenerId, Type, Name, Opts0) -> tcp -> Opts3#{tcp_options => tcp_opts(Opts0)}; ssl -> - OptsWithCRL = inject_crl_config(Opts0), + OptsWithCRL = inject_crl_config(Opts0, OldOpts), OptsWithSNI = inject_sni_fun(ListenerId, OptsWithCRL), OptsWithRootFun = inject_root_fun(OptsWithSNI), OptsWithVerifyFun = inject_verify_fun(OptsWithRootFun), @@ -985,7 +985,7 @@ inject_sni_fun(_ListenerId, Conf) -> Conf. inject_crl_config( - Conf = #{ssl_options := #{enable_crl_check := true} = SSLOpts} + Conf = #{ssl_options := #{enable_crl_check := true} = SSLOpts}, _OldOpts ) -> HTTPTimeout = emqx_config:get([crl_cache, http_timeout], timer:seconds(15)), Conf#{ @@ -995,7 +995,16 @@ inject_crl_config( crl_cache => {emqx_ssl_crl_cache, {internal, [{http, HTTPTimeout}]}} } }; -inject_crl_config(Conf) -> +inject_crl_config(#{ssl_options := SSLOpts0} = Conf0, #{} = OldOpts) -> + %% Note: we must set crl options to `undefined' to unset them. Otherwise, + %% `esockd' will retain such options when `esockd:merge_opts/2' is called and the SSL + %% options were previously enabled. + WasEnabled = emqx_utils_maps:deep_get([ssl_options, enable_crl_check], OldOpts, false), + Undefine = fun(Acc, K) -> emqx_utils_maps:put_if(Acc, K, undefined, WasEnabled) end, + SSLOpts1 = Undefine(SSLOpts0, crl_check), + SSLOpts = Undefine(SSLOpts1, crl_cache), + Conf0#{ssl_options := SSLOpts}; +inject_crl_config(Conf, undefined = _OldOpts) -> Conf. maybe_unregister_ocsp_stapling_refresh( diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index e1de50385..1b9e89199 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -589,6 +589,14 @@ ensure_valid_options(Options, Versions) -> ensure_valid_options([], _, Acc) -> lists:reverse(Acc); +ensure_valid_options([{K, undefined} | T], Versions, Acc) when + K =:= crl_check; + K =:= crl_cache +-> + %% Note: we must set crl options to `undefined' to unset them. Otherwise, + %% `esockd' will retain such options when `esockd:merge_opts/2' is called and the SSL + %% options were previously enabled. + ensure_valid_options(T, Versions, [{K, undefined} | Acc]); ensure_valid_options([{_, undefined} | T], Versions, Acc) -> ensure_valid_options(T, Versions, Acc); ensure_valid_options([{_, ""} | T], Versions, Acc) -> diff --git a/apps/emqx/test/emqx_crl_cache_SUITE.erl b/apps/emqx/test/emqx_crl_cache_SUITE.erl index 1d7d0f1d5..51196439e 100644 --- a/apps/emqx/test/emqx_crl_cache_SUITE.erl +++ b/apps/emqx/test/emqx_crl_cache_SUITE.erl @@ -138,13 +138,14 @@ init_per_testcase(t_refresh_config = TestCase, Config) -> ]; init_per_testcase(TestCase, Config) when TestCase =:= t_update_listener; + TestCase =:= t_update_listener_enable_disable; TestCase =:= t_validations -> ct:timetrap({seconds, 30}), ok = snabbkaffe:start_trace(), %% when running emqx standalone tests, we can't use those %% features. - case does_module_exist(emqx_management) of + case does_module_exist(emqx_mgmt) of true -> DataDir = ?config(data_dir, Config), CRLFile = filename:join([DataDir, "intermediate-revoked.crl.pem"]), @@ -165,7 +166,7 @@ init_per_testcase(TestCase, Config) when {emqx_conf, #{config => #{listeners => #{ssl => #{default => ListenerConf}}}}}, emqx, emqx_management, - {emqx_dashboard, "dashboard.listeners.http { enable = true, bind = 18083 }"} + emqx_mgmt_api_test_util:emqx_dashboard() ], #{work_dir => emqx_cth_suite:work_dir(TestCase, Config)} ), @@ -206,6 +207,7 @@ read_crl(Filename) -> end_per_testcase(TestCase, Config) when TestCase =:= t_update_listener; + TestCase =:= t_update_listener_enable_disable; TestCase =:= t_validations -> Skip = proplists:get_bool(skip_does_not_apply, Config), @@ -1057,3 +1059,104 @@ do_t_validations(_Config) -> ), ok. + +%% Checks that if CRL is ever enabled and then disabled, clients can connect, even if they +%% would otherwise not have their corresponding CRLs cached and fail with `{bad_crls, +%% no_relevant_crls}`. +t_update_listener_enable_disable(Config) -> + case proplists:get_bool(skip_does_not_apply, Config) of + true -> + ct:pal("skipping as this test does not apply in this profile"), + ok; + false -> + do_t_update_listener_enable_disable(Config) + end. + +do_t_update_listener_enable_disable(Config) -> + DataDir = ?config(data_dir, Config), + Keyfile = filename:join([DataDir, "server.key.pem"]), + Certfile = filename:join([DataDir, "server.cert.pem"]), + Cacertfile = filename:join([DataDir, "ca-chain.cert.pem"]), + ClientCert = filename:join(DataDir, "client.cert.pem"), + ClientKey = filename:join(DataDir, "client.key.pem"), + + ListenerId = "ssl:default", + %% Enable CRL + {ok, {{_, 200, _}, _, ListenerData0}} = get_listener_via_api(ListenerId), + CRLConfig0 = + #{ + <<"ssl_options">> => + #{ + <<"keyfile">> => Keyfile, + <<"certfile">> => Certfile, + <<"cacertfile">> => Cacertfile, + <<"enable_crl_check">> => true, + <<"fail_if_no_peer_cert">> => true + } + }, + ListenerData1 = emqx_utils_maps:deep_merge(ListenerData0, CRLConfig0), + {ok, {_, _, ListenerData2}} = update_listener_via_api(ListenerId, ListenerData1), + ?assertMatch( + #{ + <<"ssl_options">> := + #{ + <<"enable_crl_check">> := true, + <<"verify">> := <<"verify_peer">>, + <<"fail_if_no_peer_cert">> := true + } + }, + ListenerData2 + ), + + %% Disable CRL + CRLConfig1 = + #{ + <<"ssl_options">> => + #{ + <<"keyfile">> => Keyfile, + <<"certfile">> => Certfile, + <<"cacertfile">> => Cacertfile, + <<"enable_crl_check">> => false, + <<"fail_if_no_peer_cert">> => true + } + }, + ListenerData3 = emqx_utils_maps:deep_merge(ListenerData2, CRLConfig1), + redbug:start( + [ + "esockd_server:get_listener_prop -> return", + "esockd_server:set_listener_prop -> return", + "esockd:merge_opts -> return", + "esockd_listener_sup:set_options -> return", + "emqx_listeners:inject_crl_config -> return" + ], + [{msgs, 100}] + ), + {ok, {_, _, ListenerData4}} = update_listener_via_api(ListenerId, ListenerData3), + ?assertMatch( + #{ + <<"ssl_options">> := + #{ + <<"enable_crl_check">> := false, + <<"verify">> := <<"verify_peer">>, + <<"fail_if_no_peer_cert">> := true + } + }, + ListenerData4 + ), + + %% Now the client that would be blocked tries to connect and should now be allowed. + {ok, C} = emqtt:start_link([ + {ssl, true}, + {ssl_opts, [ + {certfile, ClientCert}, + {keyfile, ClientKey}, + {verify, verify_none} + ]}, + {port, 8883} + ]), + ?assertMatch({ok, _}, emqtt:connect(C)), + emqtt:stop(C), + + ?assertNotReceive({http_get, _}), + + ok. diff --git a/changes/ce/fix-13541.en.md b/changes/ce/fix-13541.en.md new file mode 100644 index 000000000..48013cc19 --- /dev/null +++ b/changes/ce/fix-13541.en.md @@ -0,0 +1 @@ +Previously, if CRL checks were ever enabled for a listener, later disabling them via the configuration would not actually disable them until the listener restarted. This has been fixed.