From c9e05acb4cc5aa81b0da038861cb84769f9084b0 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 11 Nov 2022 09:48:59 -0300 Subject: [PATCH 1/3] fix(crl): make http timeout global for all listeners We make the CRL HTTP timeout the same for all listeners for simplicity of understanding and implementation. --- etc/emqx.conf | 5 +++-- priv/emqx.schema | 4 ++-- src/emqx_crl_cache.erl | 34 +++++++++++++++++++++++----------- test/emqx_crl_cache_SUITE.erl | 1 + 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/etc/emqx.conf b/etc/emqx.conf index 6f6ab764f..ccfaa4bb7 100644 --- a/etc/emqx.conf +++ b/etc/emqx.conf @@ -1562,11 +1562,12 @@ listener.ssl.external.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem ## Value: String ## listener.ssl.external.crl_cache_urls = http://my.crl.server/intermediate.crl.pem, http://my.other.crl.server/another.crl.pem -## The timeout for the HTTP request when fetching CRLs. +## The timeout for the HTTP request when fetching CRLs. This is +## global for all listeners. ## ## Value: Duration ## Default: 15 s -## listener.ssl.external.crl_cache_http_timeout = 15s +## crl_cache.http_timeout = 15s ## The period to refresh the CRLs from the servers. This is global ## for all URLs and listeners. diff --git a/priv/emqx.schema b/priv/emqx.schema index 40c89f05c..3fa61d490 100644 --- a/priv/emqx.schema +++ b/priv/emqx.schema @@ -1712,7 +1712,7 @@ end}. {datatype, string} ]}. -{mapping, "listener.ssl.$name.crl_cache_http_timeout", "emqx.listeners", [ +{mapping, "crl_cache.http_timeout", "emqx.crl_cache_http_timeout", [ {default, "15s"}, {datatype, {duration, ms}} ]}. @@ -2339,7 +2339,7 @@ end}. end, CRLCheck = case cuttlefish:conf_get(Prefix ++ ".enable_crl_check", Conf, false) of true -> - HTTPTimeout = cuttlefish:conf_get(Prefix ++ ".crl_cache_http_timeout", Conf, timer:seconds(15)), + HTTPTimeout = cuttlefish:conf_get("crl_cache.http_timeout", Conf, timer:seconds(15)), %% {crl_check, true} doesn't work [ {crl_check, peer} , {crl_cache, {ssl_crl_cache, {internal, [{http, HTTPTimeout}]}}} diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index a8f1531f8..79906701c 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -41,12 +41,13 @@ -define(LOG(Level, Format, Args), logger:log(Level, "[~p] " ++ Format, [?MODULE | Args])). --define(HTTP_TIMEOUT, timer:seconds(10)). +-define(HTTP_TIMEOUT, timer:seconds(15)). -define(RETRY_TIMEOUT, 5_000). -record(state, { refresh_timers = #{} :: #{binary() => timer:tref()} , refresh_interval = timer:minutes(15) :: timer:time() + , http_timeout = ?HTTP_TIMEOUT :: timer:time() }). %%-------------------------------------------------------------------- @@ -60,9 +61,13 @@ start_link() -> timer:minutes(15)), MinimumRefreshInverval = timer:minutes(1), RefreshIntervalMS = max(RefreshIntervalMS0, MinimumRefreshInverval), - start_link(#{urls => URLs, refresh_interval => RefreshIntervalMS}). + HTTPTimeoutMS = emqx:get_env(crl_cache_http_timeout, ?HTTP_TIMEOUT), + start_link(#{ urls => URLs + , refresh_interval => RefreshIntervalMS + , http_timeout => HTTPTimeoutMS + }). -start_link(Opts = #{urls := _, refresh_interval := _}) -> +start_link(Opts = #{urls := _, refresh_interval := _, http_timeout := _}) -> gen_server:start_link({local, ?MODULE}, ?MODULE, Opts, []). refresh(URL) -> @@ -75,9 +80,15 @@ evict(URL) -> %% gen_server behaviour %%-------------------------------------------------------------------- -init(#{urls := URLs, refresh_interval := RefreshIntervalMS}) -> +init(Config) -> + #{ urls := URLs + , refresh_interval := RefreshIntervalMS + , http_timeout := HTTPTimeoutMS + } = Config, State = lists:foldl(fun(URL, Acc) -> ensure_timer(URL, Acc, 0) end, - #state{refresh_interval = RefreshIntervalMS}, + #state{ refresh_interval = RefreshIntervalMS + , http_timeout = HTTPTimeoutMS + }, URLs), {ok, State}. @@ -95,7 +106,7 @@ handle_cast({evict, URL}, State0 = #state{refresh_timers = RefreshTimers0}) -> }), {noreply, State}; handle_cast({refresh, URL}, State0) -> - case do_http_fetch_and_cache(URL) of + case do_http_fetch_and_cache(URL, State0#state.http_timeout) of {error, Error} -> ?tp(crl_refresh_failure, #{error => Error, url => URL}), ?LOG(error, "failed to fetch crl response for ~p; error: ~p", @@ -109,12 +120,14 @@ handle_cast(_Cast, State) -> {noreply, State}. handle_info({timeout, TRef, {refresh, URL}}, - State = #state{refresh_timers = RefreshTimers}) -> + State = #state{ refresh_timers = RefreshTimers + , http_timeout = HTTPTimeoutMS + }) -> case maps:get(URL, RefreshTimers, undefined) of TRef -> ?tp(crl_refresh_timer, #{url => URL}), ?LOG(debug, "refreshing crl response for ~p", [URL]), - case do_http_fetch_and_cache(URL) of + case do_http_fetch_and_cache(URL, HTTPTimeoutMS) of {error, Error} -> ?LOG(error, "failed to fetch crl response for ~p; error: ~p", [URL, Error]), @@ -142,10 +155,9 @@ http_get(URL, HTTPTimeout) -> [{body_format, binary}] ). -do_http_fetch_and_cache(URL) -> +do_http_fetch_and_cache(URL, HTTPTimeoutMS) -> ?tp(crl_http_fetch, #{crl_url => URL}), - %% FIXME: read from config - Resp = ?MODULE:http_get(URL, ?HTTP_TIMEOUT), + Resp = ?MODULE:http_get(URL, HTTPTimeoutMS), case Resp of {ok, {{_, 200, _}, _, Body}} -> case parse_crls(Body) of diff --git a/test/emqx_crl_cache_SUITE.erl b/test/emqx_crl_cache_SUITE.erl index d736c5a87..4c282a376 100644 --- a/test/emqx_crl_cache_SUITE.erl +++ b/test/emqx_crl_cache_SUITE.erl @@ -257,6 +257,7 @@ t_init_refresh(Config) -> URL2 = "http://localhost/crl2.pem", Opts = #{ urls => [URL1, URL2] , refresh_interval => timer:minutes(15) + , http_timeout => timer:seconds(15) }, ok = snabbkaffe:start_trace(), {ok, SubRef} = snabbkaffe:subscribe( From b08d1651ad917f69ed92b4b026fb058c1b0bd785 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 11 Nov 2022 10:47:38 -0300 Subject: [PATCH 2/3] docs: remove comment The server will still fetch CRLs on the fly. --- etc/emqx.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/etc/emqx.conf b/etc/emqx.conf index ccfaa4bb7..2303402d3 100644 --- a/etc/emqx.conf +++ b/etc/emqx.conf @@ -1550,7 +1550,6 @@ listener.ssl.external.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem ## listener.ssl.external.ocsp_refresh_http_timeout = 15s ## Whether to enable CRL verification and caching for this listener. -## If set to true, requires specifying the CRL server URLs. ## ## Value: boolean ## Default: false From 0ca74925150933a9f3d892f0cc443e580631a5fd Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 11 Nov 2022 12:21:03 -0300 Subject: [PATCH 3/3] feat(crl): add refresh config API --- src/emqx_crl_cache.erl | 57 +++++++++++++++++------ test/emqx_crl_cache_SUITE.erl | 86 +++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 13 deletions(-) diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index 79906701c..957890f46 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -23,6 +23,7 @@ , start_link/1 , refresh/1 , evict/1 + , refresh_config/0 ]). %% gen_server callbacks @@ -48,6 +49,7 @@ { refresh_timers = #{} :: #{binary() => timer:tref()} , refresh_interval = timer:minutes(15) :: timer:time() , http_timeout = ?HTTP_TIMEOUT :: timer:time() + , extra = #{} :: map() %% for future use }). %%-------------------------------------------------------------------- @@ -55,20 +57,11 @@ %%-------------------------------------------------------------------- start_link() -> - Listeners = emqx:get_env(listeners, []), - URLs = collect_urls(Listeners), - RefreshIntervalMS0 = emqx:get_env(crl_cache_refresh_interval, - timer:minutes(15)), - MinimumRefreshInverval = timer:minutes(1), - RefreshIntervalMS = max(RefreshIntervalMS0, MinimumRefreshInverval), - HTTPTimeoutMS = emqx:get_env(crl_cache_http_timeout, ?HTTP_TIMEOUT), - start_link(#{ urls => URLs - , refresh_interval => RefreshIntervalMS - , http_timeout => HTTPTimeoutMS - }). + Config = gather_config(), + start_link(Config). -start_link(Opts = #{urls := _, refresh_interval := _, http_timeout := _}) -> - gen_server:start_link({local, ?MODULE}, ?MODULE, Opts, []). +start_link(Config = #{urls := _, refresh_interval := _, http_timeout := _}) -> + gen_server:start_link({local, ?MODULE}, ?MODULE, Config, []). refresh(URL) -> gen_server:cast(?MODULE, {refresh, URL}). @@ -76,6 +69,11 @@ refresh(URL) -> evict(URL) -> gen_server:cast(?MODULE, {evict, URL}). +%% to pick up changes from the config +-spec refresh_config() -> ok. +refresh_config() -> + gen_server:cast(?MODULE, refresh_config). + %%-------------------------------------------------------------------- %% gen_server behaviour %%-------------------------------------------------------------------- @@ -116,6 +114,21 @@ handle_cast({refresh, URL}, State0) -> ?LOG(debug, "fetched crl response for ~p", [URL]), {noreply, ensure_timer(URL, State0)} end; +handle_cast(refresh_config, State0) -> + #{ urls := URLs + , http_timeout := HTTPTimeoutMS + , refresh_interval := RefreshIntervalMS + } = gather_config(), + State = lists:foldl(fun(URL, Acc) -> ensure_timer(URL, Acc, 0) end, + State0#state{ refresh_interval = RefreshIntervalMS + , http_timeout = HTTPTimeoutMS + }, + URLs), + ?tp(crl_cache_refresh_config, #{ refresh_interval => RefreshIntervalMS + , http_timeout => HTTPTimeoutMS + , urls => URLs + }), + State; handle_cast(_Cast, State) -> {noreply, State}. @@ -186,6 +199,7 @@ ensure_timer(URL, State = #state{refresh_interval = Timeout}) -> ensure_timer(URL, State, Timeout). ensure_timer(URL, State = #state{refresh_timers = RefreshTimers0}, Timeout) -> + ?tp(crl_cache_ensure_timer, #{url => URL, timeout => Timeout}), MTimer = maps:get(URL, RefreshTimers0, undefined), emqx_misc:cancel_timer(MTimer), RefreshTimers = RefreshTimers0#{URL => emqx_misc:start_timer( @@ -209,3 +223,20 @@ collect_urls(Listeners) -> end, CRLOpts1), lists:usort(CRLURLs). + +-spec gather_config() -> #{ urls := [string()] + , refresh_interval := timer:time() + , http_timeout := timer:time() + }. +gather_config() -> + Listeners = emqx:get_env(listeners, []), + URLs = collect_urls(Listeners), + RefreshIntervalMS0 = emqx:get_env(crl_cache_refresh_interval, + timer:minutes(15)), + MinimumRefreshInverval = timer:minutes(1), + RefreshIntervalMS = max(RefreshIntervalMS0, MinimumRefreshInverval), + HTTPTimeoutMS = emqx:get_env(crl_cache_http_timeout, ?HTTP_TIMEOUT), + #{ urls => URLs + , refresh_interval => RefreshIntervalMS + , http_timeout => HTTPTimeoutMS + }. diff --git a/test/emqx_crl_cache_SUITE.erl b/test/emqx_crl_cache_SUITE.erl index 4c282a376..da5b0a17f 100644 --- a/test/emqx_crl_cache_SUITE.erl +++ b/test/emqx_crl_cache_SUITE.erl @@ -56,6 +56,29 @@ init_per_testcase(t_not_cached_and_unreachable, Config) -> [ {crl_pem, CRLPem} , {crl_der, CRLDer} | Config]; +init_per_testcase(t_refresh_config, Config) -> + DataDir = ?config(data_dir, Config), + CRLFile = filename:join([DataDir, "crl.pem"]), + {ok, CRLPem} = file:read_file(CRLFile), + [{'CertificateList', CRLDer, not_encrypted}] = public_key:pem_decode(CRLPem), + TestPid = self(), + ok = meck:new(emqx_crl_cache, [non_strict, passthrough, no_history, no_link]), + meck:expect(emqx_crl_cache, http_get, + fun(URL, _HTTPTimeout) -> + TestPid ! {http_get, URL}, + {ok, {{"HTTP/1.0", 200, 'OK'}, [], CRLPem}} + end), + OldListeners = emqx:get_env(listeners), + OldRefreshInterval = emqx:get_env(crl_cache_refresh_interval), + OldHTTPTimeout = emqx:get_env(crl_cache_http_timeout), + ok = setup_crl_options(Config, #{is_cached => false}), + [ {crl_pem, CRLPem} + , {crl_der, CRLDer} + , {old_configs, [ {listeners, OldListeners} + , {crl_cache_refresh_interval, OldRefreshInterval} + , {crl_cache_http_timeout, OldHTTPTimeout} + ]} + | Config]; init_per_testcase(_TestCase, Config) -> DataDir = ?config(data_dir, Config), CRLFile = filename:join([DataDir, "crl.pem"]), @@ -86,6 +109,7 @@ end_per_testcase(TestCase, Config) ]), application:stop(cowboy), clear_crl_cache(), + ok = snabbkaffe:stop(), ok; end_per_testcase(t_not_cached_and_unreachable, _Config) -> emqx_ct_helpers:stop_apps([]), @@ -95,10 +119,34 @@ end_per_testcase(t_not_cached_and_unreachable, _Config) -> ]} ]), clear_crl_cache(), + ok = snabbkaffe:stop(), + ok; +end_per_testcase(t_refresh_config, Config) -> + OldConfigs = ?config(old_configs, Config), + meck:unload([emqx_crl_cache]), + emqx_ct_helpers:stop_apps([]), + emqx_ct_helpers:change_emqx_opts( + ssl_twoway, [ {crl_options, [ {crl_check_enabled, false} + , {crl_cache_urls, []} + ]} + ]), + clear_crl_cache(), + lists:foreach( + fun({Key, MValue}) -> + case MValue of + undefined -> ok; + Value -> application:set_env(emqx, Key, Value) + end + end, + OldConfigs), + application:stop(cowboy), + clear_crl_cache(), + ok = snabbkaffe:stop(), ok; end_per_testcase(_TestCase, _Config) -> meck:unload([emqx_crl_cache]), clear_crl_cache(), + ok = snabbkaffe:stop(), ok. %%-------------------------------------------------------------------- @@ -422,6 +470,44 @@ t_filled_cache(Config) -> emqtt:disconnect(C), ok. +t_refresh_config(_Config) -> + URLs = [ "http://localhost:9878/some.crl.pem" + , "http://localhost:9878/another.crl.pem" + ], + SortedURLs = lists:sort(URLs), + emqx_ct_helpers:change_emqx_opts( + ssl_twoway, [ {crl_options, [ {crl_check_enabled, true} + , {crl_cache_urls, URLs} + ]} + ]), + %% has to be more than 1 minute + NewRefreshInterval = timer:seconds(64), + NewHTTPTimeout = timer:seconds(7), + application:set_env(emqx, crl_cache_refresh_interval, NewRefreshInterval), + application:set_env(emqx, crl_cache_http_timeout, NewHTTPTimeout), + ?check_trace( + ?wait_async_action( + emqx_crl_cache:refresh_config(), + #{?snk_kind := crl_cache_refresh_config}, + _Timeout = 10_000), + fun(Res, Trace) -> + ?assertMatch({ok, {ok, _}}, Res), + ?assertMatch( + [#{ urls := SortedURLs + , refresh_interval := NewRefreshInterval + , http_timeout := NewHTTPTimeout + }], + ?of_kind(crl_cache_refresh_config, Trace), + #{ expected => #{ urls => SortedURLs + , refresh_interval => NewRefreshInterval + , http_timeout => NewHTTPTimeout + } + }), + ?assertEqual(SortedURLs, ?projection(url, ?of_kind(crl_cache_ensure_timer, Trace))), + ok + end), + ok. + %% If the CRL is not cached when the client tries to connect and the %% CRL server is unreachable, the client will be denied connection. t_not_cached_and_unreachable(Config) ->