From 86278c99a6c5ba0c3b649260cb56251f9c167d2a Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 7 Nov 2022 15:35:07 -0300 Subject: [PATCH 01/16] fix: enforce minimum refresh interval for ocsp and crl caches --- src/emqx_crl_cache.erl | 6 ++++-- src/emqx_ocsp_cache.erl | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index 33c9d0374..49bdec126 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -55,8 +55,10 @@ start_link() -> URLs = emqx:get_env(crl_cache_urls, []), - RefreshIntervalMS = emqx:get_env(crl_cache_refresh_interval, - timer:minutes(15)), + RefreshIntervalMS0 = emqx:get_env(crl_cache_refresh_interval, + timer:minutes(15)), + MinimumRefreshInverval = timer:minutes(1), + RefreshIntervalMS = max(RefreshIntervalMS0, MinimumRefreshInverval), start_link(#{urls => URLs, refresh_interval => RefreshIntervalMS}). start_link(Opts = #{urls := _, refresh_interval := _}) -> diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index d1e11c558..9246f6df9 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -141,7 +141,9 @@ handle_call({http_fetch, ListenerID}, _From, State) -> handle_call({register_listener, ListenerID}, _From, State0) -> ?LOG(debug, "registering ocsp cache for ~p", [ListenerID]), #{opts := Opts} = emqx_listeners:find_by_id(ListenerID), - RefreshInterval = proplists:get_value(ocsp_refresh_interval, Opts), + RefreshInterval0 = proplists:get_value(ocsp_refresh_interval, Opts), + MinimumRefrshInterval = timer:minutes(1), + RefreshInterval = max(RefreshInterval0, MinimumRefrshInterval), State = State0#{{refresh_interval, ListenerID} => RefreshInterval}, {reply, ok, ensure_timer(ListenerID, State, 0)}; handle_call(Call, _From, State) -> From 26d2ed3d319aeda05b9c17feeb1fbca2ebe881b6 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 7 Nov 2022 16:42:51 -0300 Subject: [PATCH 02/16] fix(crl): allow specifying CRL URLs per listener --- priv/emqx.schema | 27 ++++++++++++++++----------- src/emqx_crl_cache.erl | 11 ++++++++++- test/emqx_crl_cache_SUITE.erl | 33 ++++++++++++++++++++++----------- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/priv/emqx.schema b/priv/emqx.schema index 361374ee5..5c4ed24e2 100644 --- a/priv/emqx.schema +++ b/priv/emqx.schema @@ -1702,6 +1702,11 @@ end}. {datatype, {enum, [true, false]}} ]}. +{mapping, "listener.ssl.$name.crl_cache_urls", "emqx.listeners", [ + {default, ""}, + {datatype, string} +]}. + {mapping, "listener.ssl.$name.crl_cache_http_timeout", "emqx.listeners", [ {default, "15s"}, {datatype, {duration, ms}} @@ -1712,16 +1717,6 @@ end}. {datatype, {duration, ms}} ]}. -{mapping, "crl_cache.urls", "emqx.crl_cache_urls", [ - {default, ""}, - {datatype, string} -]}. - -{translation, "emqx.crl_cache_urls", fun(Conf) -> - Val = cuttlefish:conf_get("crl_cache.urls", Conf), - string:tokens(Val, ", ") -end}. - %%-------------------------------------------------------------------- %% MQTT/WebSocket Listeners @@ -2364,7 +2359,16 @@ end}. {hibernate_after, cuttlefish:conf_get(Prefix ++ ".hibernate_after", Conf, undefined)} ]) end, - + CRLOpts = + fun(Prefix) -> + CRLURLs = case cuttlefish:conf_get(Prefix ++ ".crl_cache_urls", Conf, undefined) of + undefined -> undefined; + URLs -> string:tokens(URLs, ", ") + end, + Filter([ {crl_cache_enabled, cuttlefish:conf_get(Prefix ++ ".enable_crl_cache", Conf, false)} + , {crl_cache_urls, CRLURLs} + ]) + end, Listen_fix = fun({Ip, Port}) -> case inet:parse_address(Ip) of {ok, R} -> {R, Port}; _ -> {Ip, Port} @@ -2400,6 +2404,7 @@ end}. , opts => [ {deflate_options, DeflateOpts(Prefix)} , {tcp_options, TcpOpts(Prefix)} , {ssl_options, SslOpts(Prefix)} + , {crl_options, CRLOpts(Prefix)} | LisOpts(Prefix) ] } diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index 49bdec126..d4e35ede4 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -54,7 +54,8 @@ %%-------------------------------------------------------------------- start_link() -> - URLs = emqx:get_env(crl_cache_urls, []), + Listeners = emqx:get_env(listeners, []), + URLs = collect_urls(Listeners), RefreshIntervalMS0 = emqx:get_env(crl_cache_refresh_interval, timer:minutes(15)), MinimumRefreshInverval = timer:minutes(1), @@ -177,3 +178,11 @@ ensure_timer(URL, State = #state{refresh_timers = RefreshTimers0}, Timeout) -> Timeout, {refresh, URL})}, State#state{refresh_timers = RefreshTimers}. + +collect_urls(Listeners) -> + lists:usort([URL + || #{proto := ssl, opts := Opts} <- Listeners, + {crl_options, CRLOpts} <- Opts, + proplists:get_bool(crl_cache_enabled, CRLOpts), + {crl_cache_urls, URLs} <- CRLOpts, + URL <- URLs]). diff --git a/test/emqx_crl_cache_SUITE.erl b/test/emqx_crl_cache_SUITE.erl index 9fb9a03ea..685e76684 100644 --- a/test/emqx_crl_cache_SUITE.erl +++ b/test/emqx_crl_cache_SUITE.erl @@ -79,13 +79,21 @@ end_per_testcase(TestCase, Config) ServerPid = ?config(http_server, Config), emqx_crl_cache_http_server:stop(ServerPid), emqx_ct_helpers:stop_apps([]), - application:set_env(emqx, crl_cache_urls, []), + emqx_ct_helpers:change_emqx_opts( + ssl_twoway, [ {crl_options, [ {crl_cache_enabled, false} + , {crl_cache_urls, []} + ]} + ]), application:stop(cowboy), clear_crl_cache(), ok; end_per_testcase(t_not_cached_and_unreachable, _Config) -> emqx_ct_helpers:stop_apps([]), - application:set_env(emqx, crl_cache_urls, []), + emqx_ct_helpers:change_emqx_opts( + ssl_twoway, [ {crl_options, [ {crl_cache_enabled, false} + , {crl_cache_urls, []} + ]} + ]), clear_crl_cache(), ok; end_per_testcase(_TestCase, _Config) -> @@ -177,16 +185,19 @@ setup_crl_options(Config, #{is_cached := IsCached}) -> end, Handler = fun(emqx) -> - application:set_env(emqx, crl_cache_urls, URLs), emqx_ct_helpers:change_emqx_opts( - ssl_twoway, [{ssl_options, [ {certfile, Certfile} - , {keyfile, Keyfile} - , {verify, verify_peer} - %% {crl_check, true} does not work; probably bug in OTP - , {crl_check, peer} - , {crl_cache, - {ssl_crl_cache, {internal, [{http, timer:seconds(15)}]}}} - ]}]), + ssl_twoway, [ {ssl_options, [ {certfile, Certfile} + , {keyfile, Keyfile} + , {verify, verify_peer} + %% {crl_check, true} does not work; probably bug in OTP + , {crl_check, peer} + , {crl_cache, + {ssl_crl_cache, {internal, [{http, timer:seconds(15)}]}}} + ]} + , {crl_options, [ {crl_cache_enabled, true} + , {crl_cache_urls, URLs} + ]} + ]), %% emqx_ct_helpers:change_emqx_opts has cacertfile hardcoded.... ok = force_cacertfile(Cacertfile), ok; From 11175b55f805c9f45a9010c67bccd6de8cac7cb9 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Mon, 7 Nov 2022 17:50:02 -0300 Subject: [PATCH 03/16] fix(ocsp): allow disabling ocsp without touching url --- etc/emqx.conf | 6 ++++++ priv/emqx.schema | 6 ++++++ src/emqx_ocsp_cache.erl | 17 +++++++++++------ test/emqx_ocsp_cache_SUITE.erl | 29 +++++++++++++++++++++++------ 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/etc/emqx.conf b/etc/emqx.conf index 143f93a8f..38f8bd198 100644 --- a/etc/emqx.conf +++ b/etc/emqx.conf @@ -1521,6 +1521,12 @@ listener.ssl.external.certfile = {{ platform_etc_dir }}/certs/cert.pem ## Value: File listener.ssl.external.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem +## Wheter to enable OCSP for the listener. +## +## Value: boolean +## Default: false +## listener.ssl.external.enable_ocsp = true + ## URL for the OCSP responder to check the server certificate against. ## ## Value: String diff --git a/priv/emqx.schema b/priv/emqx.schema index 5c4ed24e2..ffc5130a2 100644 --- a/priv/emqx.schema +++ b/priv/emqx.schema @@ -1679,6 +1679,11 @@ end}. {datatype, {duration, ms}} ]}. +{mapping, "listener.ssl.$name.enable_ocsp", "emqx.listeners", [ + {default, false}, + {datatype, {enum, [true, false]}} +]}. + {mapping, "listener.ssl.$name.ocsp_responder_url", "emqx.listeners", [ {datatype, string} ]}. @@ -2237,6 +2242,7 @@ end}. {supported_subprotocols, string:tokens(cuttlefish:conf_get(Prefix ++ ".supported_subprotocols", Conf, ""), ", ")}, {peer_cert_as_username, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_username", Conf, undefined)}, {peer_cert_as_clientid, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_clientid", Conf, undefined)}, + {ocsp_enabled, cuttlefish:conf_get(Prefix ++ ".enable_ocsp", Conf, undefined)}, {ocsp_responder_url, cuttlefish:conf_get(Prefix ++ ".ocsp_responder_url", Conf, undefined)}, {ocsp_issuer_pem, cuttlefish:conf_get(Prefix ++ ".ocsp_issuer_pem", Conf, undefined)}, {ocsp_refresh_interval, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_interval", Conf, undefined)}, diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index 9246f6df9..281614edb 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -48,6 +48,11 @@ -define(CALL_TIMEOUT, 20_000). -define(RETRY_TIMEOUT, 5_000). -define(REFRESH_TIMER(LID), {refresh_timer, LID}). +-ifdef(TEST). +-define(MIN_REFRESH_INTERVAL, timer:seconds(5)). +-else. +-define(MIN_REFRESH_INTERVAL, timer:minutes(1)). +-endif. %%-------------------------------------------------------------------- %% API @@ -93,10 +98,10 @@ inject_sni_fun(Listener = #{proto := Proto, name := Name, opts := Options0}) -> %% because otherwise an anonymous function will end up in %% `app.*.config'... ListenerID = emqx_listeners:identifier(Listener), - case proplists:get_value(ocsp_responder_url, Options0, undefined) of - undefined -> + case proplists:get_bool(ocsp_enabled, Options0) of + false -> Options0; - _URL -> + true -> SSLOpts0 = proplists:get_value(ssl_options, Options0, []), SNIFun = fun(SN) -> emqx_ocsp_cache:sni_fun(SN, ListenerID) end, Options1 = proplists:delete(ssl_options, Options0), @@ -142,8 +147,7 @@ handle_call({register_listener, ListenerID}, _From, State0) -> ?LOG(debug, "registering ocsp cache for ~p", [ListenerID]), #{opts := Opts} = emqx_listeners:find_by_id(ListenerID), RefreshInterval0 = proplists:get_value(ocsp_refresh_interval, Opts), - MinimumRefrshInterval = timer:minutes(1), - RefreshInterval = max(RefreshInterval0, MinimumRefrshInterval), + RefreshInterval = max(RefreshInterval0, ?MIN_REFRESH_INTERVAL), State = State0#{{refresh_interval, ListenerID} => RefreshInterval}, {reply, ok, ensure_timer(ListenerID, State, 0)}; handle_call(Call, _From, State) -> @@ -177,7 +181,8 @@ code_change(_Vsn, State, _Extra) -> ListenersToPatch = lists:filter( fun(#{opts := Opts}) -> - undefined =/= proplists:get_value(ocsp_responder_url, Opts) + undefined =/= proplists:get_value(ocsp_responder_url, Opts) andalso + false =/= proplists:get_bool(ocsp_enabled, Opts) end, emqx:get_env(listeners, [])), PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun(L)} || L <- ListenersToPatch], diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index d5d52718d..ac2fd2b90 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -96,10 +96,10 @@ init_per_testcase(t_openssl_client, Config) -> , {cacertfile, CACert} ]), Opts1 = proplists:delete(ssl_options, Opts0), - Opts2 = [ {ocsp_responder_url, "http://127.0.0.1:9877"} - , {ocsp_issuer_pem, IssuerPem} - , {ssl_options, SSLOpts2} - | Opts1], + Opts2 = emqx_misc:merge_opts(Opts1, [ {ocsp_enabled, true} + , {ocsp_responder_url, "http://127.0.0.1:9877"} + , {ocsp_issuer_pem, IssuerPem} + , {ssl_options, SSLOpts2}]), Listeners = [ SSLListener0#{opts => Opts2} | Listeners1], application:set_env(emqx, listeners, Listeners), @@ -109,7 +109,18 @@ init_per_testcase(t_openssl_client, Config) -> end, OCSPResponderPort = spawn_openssl_ocsp_responder(Config), {os_pid, OCSPOSPid} = erlang:port_info(OCSPResponderPort, os_pid), - ensure_port_open(9877), + %%%%%%%% Warning!!! + %% Apparently, openssl 3.0.7 introduced a bug in the responder + %% that makes it hang forever if one probes the port with + %% `gen_tcp:open' / `gen_tcp:close'... Comment this out if + %% openssl gets updated in CI or in your local machine. + case openssl_version() of + "3." ++ _ -> + %% hope that the responder has started... + ok; + _ -> + ensure_port_open(9877) + end, ct:sleep(1_000), emqx_ct_helpers:start_apps([], Handler), ct:sleep(1_000), @@ -134,6 +145,7 @@ init_per_testcase(_TestCase, Config) -> , name => "test_ocsp" , opts => [ {ssl_options, [{certfile, filename:join(DataDir, "server.pem")}]} + , {ocsp_enabled, true} , {ocsp_responder_url, "http://localhost:9877"} , {ocsp_issuer_pem, filename:join(DataDir, "ocsp-issuer.pem")} @@ -291,6 +303,11 @@ get_sni_fun(ListenerID) -> SSLOpts = proplists:get_value(ssl_options, Opts), proplists:get_value(sni_fun, SSLOpts). +openssl_version() -> + "OpenSSL " ++ Res = os:cmd("openssl version"), + {match, [Version]} = re:run(Res, "^([^ ]+)", [{capture, first, list}]), + Version. + %%-------------------------------------------------------------------- %% Test cases %%-------------------------------------------------------------------- @@ -417,7 +434,7 @@ t_refresh_periodically(_Config) -> false end, _NEvents = 2, - _Timeout = 5_000), + _Timeout = 10_000), ok = emqx_ocsp_cache:register_listener(ListenerID), ?assertMatch({ok, [_, _]}, snabbkaffe:receive_events(SubRef)), assert_http_get(2), From c23c534525d4ca0e9033996d4bcd89fce62e8db7 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 09:34:45 -0300 Subject: [PATCH 04/16] refactor: fix typo, add more detail, rename option to be more clear --- etc/emqx.conf | 5 +++-- priv/emqx.schema | 4 ++-- src/emqx_ocsp_cache.erl | 4 ++-- test/emqx_ocsp_cache_SUITE.erl | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/etc/emqx.conf b/etc/emqx.conf index 38f8bd198..9d26e7b46 100644 --- a/etc/emqx.conf +++ b/etc/emqx.conf @@ -1521,11 +1521,12 @@ listener.ssl.external.certfile = {{ platform_etc_dir }}/certs/cert.pem ## Value: File listener.ssl.external.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem -## Wheter to enable OCSP for the listener. +## Whether to enable OCSP stapling for the listener. If set to true, +## requires definining the OCSP responder URL. ## ## Value: boolean ## Default: false -## listener.ssl.external.enable_ocsp = true +## listener.ssl.external.enable_ocsp_stapling = true ## URL for the OCSP responder to check the server certificate against. ## diff --git a/priv/emqx.schema b/priv/emqx.schema index ffc5130a2..f1d68d4e8 100644 --- a/priv/emqx.schema +++ b/priv/emqx.schema @@ -1679,7 +1679,7 @@ end}. {datatype, {duration, ms}} ]}. -{mapping, "listener.ssl.$name.enable_ocsp", "emqx.listeners", [ +{mapping, "listener.ssl.$name.enable_ocsp_stapling", "emqx.listeners", [ {default, false}, {datatype, {enum, [true, false]}} ]}. @@ -2242,7 +2242,7 @@ end}. {supported_subprotocols, string:tokens(cuttlefish:conf_get(Prefix ++ ".supported_subprotocols", Conf, ""), ", ")}, {peer_cert_as_username, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_username", Conf, undefined)}, {peer_cert_as_clientid, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_clientid", Conf, undefined)}, - {ocsp_enabled, cuttlefish:conf_get(Prefix ++ ".enable_ocsp", Conf, undefined)}, + {ocsp_stapling_enabled, cuttlefish:conf_get(Prefix ++ ".enable_ocsp_stapling", Conf, undefined)}, {ocsp_responder_url, cuttlefish:conf_get(Prefix ++ ".ocsp_responder_url", Conf, undefined)}, {ocsp_issuer_pem, cuttlefish:conf_get(Prefix ++ ".ocsp_issuer_pem", Conf, undefined)}, {ocsp_refresh_interval, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_interval", Conf, undefined)}, diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index 281614edb..3f339e7e8 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -98,7 +98,7 @@ inject_sni_fun(Listener = #{proto := Proto, name := Name, opts := Options0}) -> %% because otherwise an anonymous function will end up in %% `app.*.config'... ListenerID = emqx_listeners:identifier(Listener), - case proplists:get_bool(ocsp_enabled, Options0) of + case proplists:get_bool(ocsp_stapling_enabled, Options0) of false -> Options0; true -> @@ -182,7 +182,7 @@ code_change(_Vsn, State, _Extra) -> lists:filter( fun(#{opts := Opts}) -> undefined =/= proplists:get_value(ocsp_responder_url, Opts) andalso - false =/= proplists:get_bool(ocsp_enabled, Opts) + false =/= proplists:get_bool(ocsp_stapling_enabled, Opts) end, emqx:get_env(listeners, [])), PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun(L)} || L <- ListenersToPatch], diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index ac2fd2b90..f85945fca 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -96,7 +96,7 @@ init_per_testcase(t_openssl_client, Config) -> , {cacertfile, CACert} ]), Opts1 = proplists:delete(ssl_options, Opts0), - Opts2 = emqx_misc:merge_opts(Opts1, [ {ocsp_enabled, true} + Opts2 = emqx_misc:merge_opts(Opts1, [ {ocsp_stapling_enabled, true} , {ocsp_responder_url, "http://127.0.0.1:9877"} , {ocsp_issuer_pem, IssuerPem} , {ssl_options, SSLOpts2}]), @@ -145,7 +145,7 @@ init_per_testcase(_TestCase, Config) -> , name => "test_ocsp" , opts => [ {ssl_options, [{certfile, filename:join(DataDir, "server.pem")}]} - , {ocsp_enabled, true} + , {ocsp_stapling_enabled, true} , {ocsp_responder_url, "http://localhost:9877"} , {ocsp_issuer_pem, filename:join(DataDir, "ocsp-issuer.pem")} From ca973db2fccc01986460e1f7f9843bccc377b4e2 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 09:58:05 -0300 Subject: [PATCH 05/16] refactor: split list comprehension over fn calls --- src/emqx_crl_cache.erl | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index d4e35ede4..8dab8d5f1 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -180,9 +180,18 @@ ensure_timer(URL, State = #state{refresh_timers = RefreshTimers0}, Timeout) -> State#state{refresh_timers = RefreshTimers}. collect_urls(Listeners) -> - lists:usort([URL - || #{proto := ssl, opts := Opts} <- Listeners, - {crl_options, CRLOpts} <- Opts, - proplists:get_bool(crl_cache_enabled, CRLOpts), - {crl_cache_urls, URLs} <- CRLOpts, - URL <- URLs]). + CRLOpts0 = [CRLOpts || #{proto := ssl, opts := Opts} <- Listeners, + {crl_options, CRLOpts} <- Opts], + CRLOpts1 = + lists:filter( + fun(CRLOpts) -> + proplists:get_bool(crl_cache_enabled, CRLOpts) + end, + CRLOpts0), + CRLURLs = + lists:flatmap( + fun(CRLOpts) -> + proplists:get_value(crl_cache_urls, CRLOpts, []) + end, + CRLOpts1), + lists:usort(CRLURLs). From 2713af507af08f90bcdf88106ca662e83fdf6672 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 13:19:14 -0300 Subject: [PATCH 06/16] refactor: move OCSP options into its own section --- priv/emqx.schema | 14 +++++++++----- src/emqx_ocsp_cache.erl | 18 +++++++++++------- test/emqx_ocsp_cache_SUITE.erl | 22 +++++++++++++--------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/priv/emqx.schema b/priv/emqx.schema index f1d68d4e8..e2daf197f 100644 --- a/priv/emqx.schema +++ b/priv/emqx.schema @@ -2224,6 +2224,14 @@ end}. lists:flatten(OriginList) end end, + OCSPOpts = fun(Prefix) -> + Filter([ {ocsp_stapling_enabled, cuttlefish:conf_get(Prefix ++ ".enable_ocsp_stapling", Conf, undefined)} + , {ocsp_responder_url, cuttlefish:conf_get(Prefix ++ ".ocsp_responder_url", Conf, undefined)} + , {ocsp_issuer_pem, cuttlefish:conf_get(Prefix ++ ".ocsp_issuer_pem", Conf, undefined)} + , {ocsp_refresh_interval, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_interval", Conf, undefined)} + , {ocsp_refresh_http_timeout, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_http_timeout", Conf, undefined)} + ]) + end, LisOpts = fun(Prefix) -> Filter([{acceptors, cuttlefish:conf_get(Prefix ++ ".acceptors", Conf)}, @@ -2242,11 +2250,6 @@ end}. {supported_subprotocols, string:tokens(cuttlefish:conf_get(Prefix ++ ".supported_subprotocols", Conf, ""), ", ")}, {peer_cert_as_username, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_username", Conf, undefined)}, {peer_cert_as_clientid, cuttlefish:conf_get(Prefix ++ ".peer_cert_as_clientid", Conf, undefined)}, - {ocsp_stapling_enabled, cuttlefish:conf_get(Prefix ++ ".enable_ocsp_stapling", Conf, undefined)}, - {ocsp_responder_url, cuttlefish:conf_get(Prefix ++ ".ocsp_responder_url", Conf, undefined)}, - {ocsp_issuer_pem, cuttlefish:conf_get(Prefix ++ ".ocsp_issuer_pem", Conf, undefined)}, - {ocsp_refresh_interval, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_interval", Conf, undefined)}, - {ocsp_refresh_http_timeout, cuttlefish:conf_get(Prefix ++ ".ocsp_refresh_http_timeout", Conf, undefined)}, {compress, cuttlefish:conf_get(Prefix ++ ".compress", Conf, undefined)}, {idle_timeout, cuttlefish:conf_get(Prefix ++ ".idle_timeout", Conf, undefined)}, {max_frame_size, cuttlefish:conf_get(Prefix ++ ".max_frame_size", Conf, undefined)}, @@ -2411,6 +2414,7 @@ end}. , {tcp_options, TcpOpts(Prefix)} , {ssl_options, SslOpts(Prefix)} , {crl_options, CRLOpts(Prefix)} + , {ocsp_options, OCSPOpts(Prefix)} | LisOpts(Prefix) ] } diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index 3f339e7e8..fb7766873 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -98,7 +98,8 @@ inject_sni_fun(Listener = #{proto := Proto, name := Name, opts := Options0}) -> %% because otherwise an anonymous function will end up in %% `app.*.config'... ListenerID = emqx_listeners:identifier(Listener), - case proplists:get_bool(ocsp_stapling_enabled, Options0) of + OCSPOpts = proplists:get_value(ocsp_options, Options0, []), + case proplists:get_bool(ocsp_stapling_enabled, OCSPOpts) of false -> Options0; true -> @@ -146,7 +147,8 @@ handle_call({http_fetch, ListenerID}, _From, State) -> handle_call({register_listener, ListenerID}, _From, State0) -> ?LOG(debug, "registering ocsp cache for ~p", [ListenerID]), #{opts := Opts} = emqx_listeners:find_by_id(ListenerID), - RefreshInterval0 = proplists:get_value(ocsp_refresh_interval, Opts), + OCSPOpts = proplists:get_value(ocsp_options, Opts), + RefreshInterval0 = proplists:get_value(ocsp_refresh_interval, OCSPOpts), RefreshInterval = max(RefreshInterval0, ?MIN_REFRESH_INTERVAL), State = State0#{{refresh_interval, ListenerID} => RefreshInterval}, {reply, ok, ensure_timer(ListenerID, State, 0)}; @@ -181,8 +183,9 @@ code_change(_Vsn, State, _Extra) -> ListenersToPatch = lists:filter( fun(#{opts := Opts}) -> - undefined =/= proplists:get_value(ocsp_responder_url, Opts) andalso - false =/= proplists:get_bool(ocsp_stapling_enabled, Opts) + OCSPOpts = proplists:get_value(ocsp_options, Opts), + undefined =/= proplists:get_value(ocsp_responder_url, OCSPOpts, undefined) andalso + false =/= proplists:get_bool(ocsp_stapling_enabled, OCSPOpts) end, emqx:get_env(listeners, [])), PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun(L)} || L <- ListenersToPatch], @@ -229,9 +232,10 @@ read_server_cert(ServerCertPemPath0) -> do_http_fetch_and_cache(ListenerID) -> #{opts := Options} = emqx_listeners:find_by_id(ListenerID), - ResponderURL0 = proplists:get_value(ocsp_responder_url, Options, undefined), + OCSPOpts = proplists:get_value(ocsp_options, Options), + ResponderURL0 = proplists:get_value(ocsp_responder_url, OCSPOpts, undefined), ResponderURL = uri_string:normalize(ResponderURL0), - IssuerPemPath = proplists:get_value(ocsp_issuer_pem, Options, undefined), + IssuerPemPath = proplists:get_value(ocsp_issuer_pem, OCSPOpts, undefined), SSLOpts = proplists:get_value(ssl_options, Options, undefined), ServerCertPemPath = proplists:get_value(certfile, SSLOpts, undefined), IssuerPem = case file:read_file(IssuerPemPath) of @@ -240,7 +244,7 @@ do_http_fetch_and_cache(ListenerID) -> end, ServerCert = read_server_cert(ServerCertPemPath), Request = build_ocsp_request(IssuerPem, ServerCert), - HTTPTimeout = proplists:get_value(ocsp_refresh_http_timeout, Options), + HTTPTimeout = proplists:get_value(ocsp_refresh_http_timeout, OCSPOpts), ?tp(ocsp_http_fetch, #{ listener_id => ListenerID , responder_url => ResponderURL , timeout => HTTPTimeout diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index f85945fca..587125fc5 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -96,9 +96,11 @@ init_per_testcase(t_openssl_client, Config) -> , {cacertfile, CACert} ]), Opts1 = proplists:delete(ssl_options, Opts0), - Opts2 = emqx_misc:merge_opts(Opts1, [ {ocsp_stapling_enabled, true} - , {ocsp_responder_url, "http://127.0.0.1:9877"} - , {ocsp_issuer_pem, IssuerPem} + OCSPOpts = [ {ocsp_stapling_enabled, true} + , {ocsp_responder_url, "http://127.0.0.1:9877"} + , {ocsp_issuer_pem, IssuerPem} + ], + Opts2 = emqx_misc:merge_opts(Opts1, [ {ocsp_options, OCSPOpts} , {ssl_options, SSLOpts2}]), Listeners = [ SSLListener0#{opts => Opts2} | Listeners1], @@ -139,18 +141,20 @@ init_per_testcase(_TestCase, Config) -> end), {ok, CachePid} = emqx_ocsp_cache:start_link(), DataDir = ?config(data_dir, Config), + OCSPOpts = [ {ocsp_stapling_enabled, true} + , {ocsp_responder_url, "http://localhost:9877"} + , {ocsp_issuer_pem, + filename:join(DataDir, "ocsp-issuer.pem")} + , {ocsp_refresh_http_timeout, 15_000} + , {ocsp_refresh_interval, 1_000} + ], application:set_env( emqx, listeners, [#{ proto => ssl , name => "test_ocsp" , opts => [ {ssl_options, [{certfile, filename:join(DataDir, "server.pem")}]} - , {ocsp_stapling_enabled, true} - , {ocsp_responder_url, "http://localhost:9877"} - , {ocsp_issuer_pem, - filename:join(DataDir, "ocsp-issuer.pem")} - , {ocsp_refresh_http_timeout, 15_000} - , {ocsp_refresh_interval, 1_000} + , {ocsp_options, OCSPOpts} ] }]), snabbkaffe:start_trace(), From b0e8e9dc280235102a89c2ebc4d26a3a5e8497b1 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 13:53:27 -0300 Subject: [PATCH 07/16] refactor(crl): use cast for refreshing crls --- src/emqx_crl_cache.erl | 25 +++++++++-------- test/emqx_crl_cache_SUITE.erl | 53 +++++++++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl index 8dab8d5f1..d411bc52e 100644 --- a/src/emqx_crl_cache.erl +++ b/src/emqx_crl_cache.erl @@ -66,7 +66,7 @@ start_link(Opts = #{urls := _, refresh_interval := _}) -> gen_server:start_link({local, ?MODULE}, ?MODULE, Opts, []). refresh(URL) -> - gen_server:call(?MODULE, {refresh, URL}, ?HTTP_TIMEOUT + 2_000). + gen_server:cast(?MODULE, {refresh, URL}). evict(URL) -> gen_server:cast(?MODULE, {evict, URL}). @@ -81,16 +81,6 @@ init(#{urls := URLs, refresh_interval := RefreshIntervalMS}) -> URLs), {ok, State}. -handle_call({refresh, URL}, _From, State0) -> - case do_http_fetch_and_cache(URL) of - {error, Error} -> - ?LOG(error, "failed to fetch crl response for ~p; error: ~p", - [URL, Error]), - {reply, error, ensure_timer(URL, State0, ?RETRY_TIMEOUT)}; - {ok, CRLs} -> - ?LOG(debug, "fetched crl response for ~p", [URL]), - {reply, {ok, CRLs}, ensure_timer(URL, State0)} - end; handle_call(Call, _From, State) -> {reply, {error, {bad_call, Call}}, State}. @@ -104,6 +94,17 @@ handle_cast({evict, URL}, State0 = #state{refresh_timers = RefreshTimers0}) -> #{ url => URL }), {noreply, State}; +handle_cast({refresh, URL}, State0) -> + case do_http_fetch_and_cache(URL) of + {error, Error} -> + ?tp(crl_refresh_failure, #{error => Error, url => URL}), + ?LOG(error, "failed to fetch crl response for ~p; error: ~p", + [URL, Error]), + {noreply, ensure_timer(URL, State0, ?RETRY_TIMEOUT)}; + {ok, _CRLs} -> + ?LOG(debug, "fetched crl response for ~p", [URL]), + {noreply, ensure_timer(URL, State0)} + end; handle_cast(_Cast, State) -> {noreply, State}. @@ -151,7 +152,7 @@ do_http_fetch_and_cache(URL) -> {error, invalid_crl}; CRLs -> ssl_crl_cache:insert(URL, {der, CRLs}), - ?tp(crl_cache_insert, #{url => URL}), + ?tp(crl_cache_insert, #{url => URL, crls => CRLs}), {ok, CRLs} end; {ok, {{_, Code, _}, _, Body}} -> diff --git a/test/emqx_crl_cache_SUITE.erl b/test/emqx_crl_cache_SUITE.erl index 685e76684..f9090d85f 100644 --- a/test/emqx_crl_cache_SUITE.erl +++ b/test/emqx_crl_cache_SUITE.erl @@ -280,7 +280,12 @@ t_manual_refresh(Config) -> ?assertEqual([], ets:tab2list(Ref)), {ok, _} = emqx_crl_cache:start_link(), URL = "http://localhost/crl.pem", - ?assertEqual({ok, [CRLDer]}, emqx_crl_cache:refresh(URL)), + ok = snabbkaffe:start_trace(), + ?wait_async_action( + ?assertEqual(ok, emqx_crl_cache:refresh(URL)), + #{?snk_kind := crl_cache_insert}, + 5_000), + ok = snabbkaffe:stop(), ?assertEqual( [{"crl.pem", [CRLDer]}], ets:tab2list(Ref)), @@ -293,7 +298,18 @@ t_refresh_request_error(_Config) -> end), {ok, _} = emqx_crl_cache:start_link(), URL = "http://localhost/crl.pem", - ?assertEqual(error, emqx_crl_cache:refresh(URL)), + ?check_trace( + ?wait_async_action( + ?assertEqual(ok, emqx_crl_cache:refresh(URL)), + #{?snk_kind := crl_cache_insert}, + 5_000), + fun(Trace) -> + ?assertMatch( + [#{error := {bad_response, #{code := 404}}}], + ?of_kind(crl_refresh_failure, Trace)), + ok + end), + ok = snabbkaffe:stop(), ok. t_refresh_invalid_response(_Config) -> @@ -303,7 +319,18 @@ t_refresh_invalid_response(_Config) -> end), {ok, _} = emqx_crl_cache:start_link(), URL = "http://localhost/crl.pem", - ?assertEqual({ok, []}, emqx_crl_cache:refresh(URL)), + ?check_trace( + ?wait_async_action( + ?assertEqual(ok, emqx_crl_cache:refresh(URL)), + #{?snk_kind := crl_cache_insert}, + 5_000), + fun(Trace) -> + ?assertMatch( + [#{crls := []}], + ?of_kind(crl_cache_insert, Trace)), + ok + end), + ok = snabbkaffe:stop(), ok. t_refresh_http_error(_Config) -> @@ -313,7 +340,18 @@ t_refresh_http_error(_Config) -> end), {ok, _} = emqx_crl_cache:start_link(), URL = "http://localhost/crl.pem", - ?assertEqual(error, emqx_crl_cache:refresh(URL)), + ?check_trace( + ?wait_async_action( + ?assertEqual(ok, emqx_crl_cache:refresh(URL)), + #{?snk_kind := crl_cache_insert}, + 5_000), + fun(Trace) -> + ?assertMatch( + [#{error := {http_error, timeout}}], + ?of_kind(crl_refresh_failure, Trace)), + ok + end), + ok = snabbkaffe:stop(), ok. t_unknown_messages(_Config) -> @@ -326,7 +364,12 @@ t_unknown_messages(_Config) -> t_evict(_Config) -> {ok, _} = emqx_crl_cache:start_link(), URL = "http://localhost/crl.pem", - {ok, [_]} = emqx_crl_cache:refresh(URL), + ok = snabbkaffe:start_trace(), + ?wait_async_action( + ?assertEqual(ok, emqx_crl_cache:refresh(URL)), + #{?snk_kind := crl_cache_insert}, + 5_000), + ok = snabbkaffe:stop(), Ref = get_crl_cache_table(), ?assertMatch([{"crl.pem", _}], ets:tab2list(Ref)), snabbkaffe:start_trace(), From cd053a28d47e72daaa500fc46bb39e15fa11eabf Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 14:21:18 -0300 Subject: [PATCH 08/16] docs(crl): add config docs for CRL options --- etc/emqx.conf | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/etc/emqx.conf b/etc/emqx.conf index 9d26e7b46..c9bcb7b59 100644 --- a/etc/emqx.conf +++ b/etc/emqx.conf @@ -1549,6 +1549,32 @@ listener.ssl.external.cacertfile = {{ platform_etc_dir }}/certs/cacert.pem ## Value: Duration ## 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 +## listener.ssl.external.enable_crl_cache = true + +## Comma-separated URL list for CRL servers to fetch and cache CRLs +## from. Must include the path to the CRL file(s). +## +## 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. +## +## Value: Duration +## Default: 15 s +## listener.ssl.external.crl_cache_http_timeout = 15s + +## The period to refresh the CRLs from the servers. This is global +## for all URLs and listeners. +## +## Value: Duration +## Default: 15 m +## crl_cache.refresh_interval = 15m + ## The Ephemeral Diffie-Helman key exchange is a very effective way of ## ensuring Forward Secrecy by exchanging a set of keys that never hit ## the wire. Since the DH key is effectively signed by the private key, From 2e5e6af6774058ab11274f9c8ed7132aba242b69 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 15:18:47 -0300 Subject: [PATCH 09/16] fix(listener): pass along the listener name The rigid `start_listener/3` does not allow to pass the original `listener()` map forward, which contains the listener name needed for OCSP SNI function to later find its required information. To avoid changing the arity of this exported function, we stuff the listener name temporarily along with the esockd options... --- src/emqx_listeners.erl | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/emqx_listeners.erl b/src/emqx_listeners.erl index 1fb4fbd68..b899dc58e 100644 --- a/src/emqx_listeners.erl +++ b/src/emqx_listeners.erl @@ -41,7 +41,8 @@ , format_listen_on/1 ]). --type(listener() :: #{ name := binary() +-type(listener_name() :: binary()). +-type(listener() :: #{ name := listener_name() , proto := esockd:proto() , listen_on := esockd:listen_on() , opts := [esockd:option()] @@ -105,10 +106,10 @@ ensure_all_started([L | Rest], Results) -> format_listen_on(ListenOn) -> format(ListenOn). -spec(start_listener(listener()) -> ok). -start_listener(Listener = #{proto := Proto, name := Name, listen_on := ListenOn}) -> +start_listener(#{proto := Proto, name := Name, listen_on := ListenOn, opts := Opts0}) -> ID = identifier(Proto, Name), - Options = emqx_ocsp_cache:inject_sni_fun(Listener), - case start_listener(Proto, ListenOn, Options) of + Opts = [{listener_name, Name} | Opts0], + case start_listener(Proto, ListenOn, Opts) of {ok, _} -> console_print("Start ~s listener on ~s successfully.~n", [ID, format(ListenOn)]); {error, Reason} -> @@ -125,13 +126,22 @@ console_print(_Fmt, _Args) -> ok. -endif. %% Start MQTT/TCP listener --spec(start_listener(esockd:proto(), esockd:listen_on(), [esockd:option()]) +-spec(start_listener(esockd:proto(), esockd:listen_on(), [ esockd:option() + | {listener_name, listener_name()}]) -> {ok, pid()} | {error, term()}). start_listener(tcp, ListenOn, Options) -> start_mqtt_listener('mqtt:tcp', ListenOn, Options); %% Start MQTT/TLS listener -start_listener(Proto, ListenOn, Options) when Proto == ssl; Proto == tls -> +start_listener(Proto, ListenOn, Options0) when Proto == ssl; Proto == tls -> + Name = proplists:get_value(listener_name, Options0, <<"mqtt:ssl:external">>), + Options1 = proplists:delete(listener_name, Options0), + Listener = #{ name => Name + , proto => Proto + , listen_on => ListenOn + , opts => Options1 + }, + Options = emqx_ocsp_cache:inject_sni_fun(Listener), start_mqtt_listener('mqtt:ssl', ListenOn, Options); %% Start MQTT/WS listener From bb249834f773f339f5e08bcec9169af1a94f9f25 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 15:46:16 -0300 Subject: [PATCH 10/16] test(ocsp): add more tests --- test/emqx_ocsp_cache_SUITE.erl | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index 587125fc5..f3fcb4afe 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -427,6 +427,22 @@ t_register_listener(_Config) -> ?assertMatch([{_, <<"ocsp response">>}], ets:tab2list(?CACHE_TAB)), ok. +t_register_twice(_Config) -> + ListenerID = <<"mqtt:ssl:test_ocsp">>, + {ok, {ok, _}} = + ?wait_async_action( + emqx_ocsp_cache:register_listener(ListenerID), + #{?snk_kind := ocsp_http_fetch_and_cache, listener_id := ListenerID}), + assert_http_get(1), + ?assertMatch([{_, <<"ocsp response">>}], ets:tab2list(?CACHE_TAB)), + %% should have no problem in registering the same listener again. + %% this prompts an immediate refresh. + {ok, {ok, _}} = + ?wait_async_action( + emqx_ocsp_cache:register_listener(ListenerID), + #{?snk_kind := ocsp_http_fetch_and_cache, listener_id := ListenerID}), + ok. + t_refresh_periodically(_Config) -> ListenerID = <<"mqtt:ssl:test_ocsp">>, %% should refresh periodically From 2dcecafce670c3aba306cbd74806b0b3021faa25 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 15:54:41 -0300 Subject: [PATCH 11/16] test(fix): account for non-openssl programs --- test/emqx_ocsp_cache_SUITE.erl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index f3fcb4afe..42c201ac3 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -308,7 +308,8 @@ get_sni_fun(ListenerID) -> proplists:get_value(sni_fun, SSLOpts). openssl_version() -> - "OpenSSL " ++ Res = os:cmd("openssl version"), + Res0 = os:cmd("openssl version"), + [_, Res] = string:split(Res0, " "), {match, [Version]} = re:run(Res, "^([^ ]+)", [{capture, first, list}]), Version. From ce282797bebf2d45f1ef239e79d2bf0cc4beddb0 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 16:12:40 -0300 Subject: [PATCH 12/16] refactor(ocsp): use listener id instead of name for sni fun matching --- src/emqx_listeners.erl | 18 +++++++----------- src/emqx_ocsp_cache.erl | 16 +++++++++------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/emqx_listeners.erl b/src/emqx_listeners.erl index b899dc58e..1a5b52788 100644 --- a/src/emqx_listeners.erl +++ b/src/emqx_listeners.erl @@ -42,6 +42,7 @@ ]). -type(listener_name() :: binary()). +-type(listener_id() :: binary()). -type(listener() :: #{ name := listener_name() , proto := esockd:proto() , listen_on := esockd:listen_on() @@ -71,7 +72,7 @@ find_by_id(Id) -> find_by_id(iolist_to_binary(Id), emqx:get_env(listeners, [])). %% @doc Return the ID of the given listener. --spec identifier(listener()) -> binary(). +-spec identifier(listener()) -> listener_id(). identifier(#{proto := Proto, name := Name}) -> identifier(Proto, Name). @@ -108,7 +109,7 @@ format_listen_on(ListenOn) -> format(ListenOn). -spec(start_listener(listener()) -> ok). start_listener(#{proto := Proto, name := Name, listen_on := ListenOn, opts := Opts0}) -> ID = identifier(Proto, Name), - Opts = [{listener_name, Name} | Opts0], + Opts = [{listener_id, ID} | Opts0], case start_listener(Proto, ListenOn, Opts) of {ok, _} -> console_print("Start ~s listener on ~s successfully.~n", [ID, format(ListenOn)]); @@ -127,21 +128,16 @@ console_print(_Fmt, _Args) -> ok. %% Start MQTT/TCP listener -spec(start_listener(esockd:proto(), esockd:listen_on(), [ esockd:option() - | {listener_name, listener_name()}]) + | {listener_id, binary()}]) -> {ok, pid()} | {error, term()}). start_listener(tcp, ListenOn, Options) -> start_mqtt_listener('mqtt:tcp', ListenOn, Options); %% Start MQTT/TLS listener start_listener(Proto, ListenOn, Options0) when Proto == ssl; Proto == tls -> - Name = proplists:get_value(listener_name, Options0, <<"mqtt:ssl:external">>), - Options1 = proplists:delete(listener_name, Options0), - Listener = #{ name => Name - , proto => Proto - , listen_on => ListenOn - , opts => Options1 - }, - Options = emqx_ocsp_cache:inject_sni_fun(Listener), + ListenerID = proplists:get_value(listener_id, Options0, <<"mqtt:ssl:external">>), + Options1 = proplists:delete(listener_id, Options0), + Options = emqx_ocsp_cache:inject_sni_fun(ListenerID, Options1), start_mqtt_listener('mqtt:ssl', ListenOn, Options); %% Start MQTT/WS listener diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index fb7766873..5c119031e 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -28,7 +28,7 @@ , sni_fun/2 , fetch_response/1 , register_listener/1 - , inject_sni_fun/1 + , inject_sni_fun/2 ]). %% gen_server API @@ -92,12 +92,11 @@ fetch_response(ListenerID) -> register_listener(ListenerID) -> gen_server:call(?MODULE, {register_listener, ListenerID}, ?CALL_TIMEOUT). --spec inject_sni_fun(emqx_listeners:listener()) -> [esockd:option()]. -inject_sni_fun(Listener = #{proto := Proto, name := Name, opts := Options0}) -> +-spec inject_sni_fun(emqx_listeners:listener_id(), [esockd:option()]) -> [esockd:option()]. +inject_sni_fun(ListenerID, Options0) -> %% We need to patch `sni_fun' here and not in `emqx.schema' %% because otherwise an anonymous function will end up in %% `app.*.config'... - ListenerID = emqx_listeners:identifier(Listener), OCSPOpts = proplists:get_value(ocsp_options, Options0, []), case proplists:get_bool(ocsp_stapling_enabled, OCSPOpts) of false -> @@ -110,8 +109,8 @@ inject_sni_fun(Listener = #{proto := Proto, name := Name, opts := Options0}) -> %% save to env {[ThisListener0], Listeners} = lists:partition( - fun(#{name := N, proto := P}) -> - N =:= Name andalso P =:= Proto + fun(L) -> + emqx_listeners:identifier(L) =:= ListenerID end, emqx:get_env(listeners)), ThisListener = ThisListener0#{opts => Options}, @@ -188,7 +187,10 @@ code_change(_Vsn, State, _Extra) -> false =/= proplists:get_bool(ocsp_stapling_enabled, OCSPOpts) end, emqx:get_env(listeners, [])), - PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun(L)} || L <- ListenersToPatch], + PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun( + emqx_listeners:identifier(L), + Opts)} + || L = #{opts := Opts} <- ListenersToPatch], lists:foreach( fun(L) -> emqx_listeners:update_listeners_env(update, L) From 32a8753e9db0ef8b5e9124fc2f923798b43b4317 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 8 Nov 2022 20:26:19 +0100 Subject: [PATCH 13/16] refactor: introduce emqx_const_v1 for immutable anonymous funs --- scripts/update_appup.escript | 13 ++++++++----- src/emqx.appup.src | 11 +++++++++++ src/emqx_const_v1.erl | 24 ++++++++++++++++++++++++ src/emqx_ocsp_cache.erl | 21 +-------------------- 4 files changed, 44 insertions(+), 25 deletions(-) create mode 100644 src/emqx_const_v1.erl diff --git a/scripts/update_appup.escript b/scripts/update_appup.escript index 2ae0520b7..bc1836235 100755 --- a/scripts/update_appup.escript +++ b/scripts/update_appup.escript @@ -313,7 +313,7 @@ do_merge_update_actions(App, {New0, Changed0, Deleted0}, OldActions) -> true -> []; false -> - [{load_module, M, brutal_purge, soft_purge, []} || M <- Changed, not is_secret_module(M)] ++ + [{load_module, M, brutal_purge, soft_purge, []} || M <- Changed, not is_const_module(M)] ++ [{add_module, M} || M <- New] end, {OldActionsWithStop, OldActionsAfterStop} = @@ -325,14 +325,17 @@ do_merge_update_actions(App, {New0, Changed0, Deleted0}, OldActions) -> true -> []; false -> - [{delete_module, M} || M <- Deleted, not is_secret_module(M)] + [{delete_module, M} || M <- Deleted, not is_const_module(M)] end ++ AppSpecific. -%% Do not reload or delet _secret modules -is_secret_module(Module) -> +is_const_module(Module) when is_atom(Module) -> + is_const_module(atom_to_list(Module)); +is_const_module("emqx_const_" ++ _) -> + true; +is_const_module(Module) -> Suffix = "_secret", - case string:right(atom_to_list(Module), length(Suffix)) of + case string:right(Module, length(Suffix)) of Suffix -> true; _ -> false end. diff --git a/src/emqx.appup.src b/src/emqx.appup.src index ccb8d6132..c3d1d5312 100644 --- a/src/emqx.appup.src +++ b/src/emqx.appup.src @@ -4,6 +4,7 @@ [{"4.4.10", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, {load_module,emqx_access_rule,brutal_purge,soft_purge,[]}, @@ -19,6 +20,7 @@ {"4.4.9", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -41,6 +43,7 @@ {"4.4.8", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -64,6 +67,7 @@ {"4.4.7", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -87,6 +91,7 @@ {"4.4.6", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -110,6 +115,7 @@ {"4.4.5", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -135,6 +141,7 @@ {"4.4.4", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -167,6 +174,7 @@ {"4.4.3", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -206,6 +214,7 @@ {"4.4.2", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_listeners,brutal_purge,soft_purge,[]}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, @@ -246,6 +255,7 @@ {"4.4.1", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, {load_module,emqx_router_helper,brutal_purge,soft_purge,[]}, @@ -291,6 +301,7 @@ {"4.4.0", [{add_module,emqx_ocsp_cache}, {add_module,emqx_crl_cache}, + {add_module,emqx_const_v1}, {add_module,emqx_secret}, {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]}, {load_module,emqx_router_helper,brutal_purge,soft_purge,[]}, diff --git a/src/emqx_const_v1.erl b/src/emqx_const_v1.erl new file mode 100644 index 000000000..3fa2f6e72 --- /dev/null +++ b/src/emqx_const_v1.erl @@ -0,0 +1,24 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2022 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%% +%% @doc Never update this module, create a v2 instead. +%%-------------------------------------------------------------------- + +-module(emqx_const_v1). + +-export([make_sni_fun/1]). + +make_sni_fun(ListenerID) -> + fun(SN) -> emqx_ocsp_cache:sni_fun(SN, ListenerID) end. diff --git a/src/emqx_ocsp_cache.erl b/src/emqx_ocsp_cache.erl index 5c119031e..578b79358 100644 --- a/src/emqx_ocsp_cache.erl +++ b/src/emqx_ocsp_cache.erl @@ -103,7 +103,7 @@ inject_sni_fun(ListenerID, Options0) -> Options0; true -> SSLOpts0 = proplists:get_value(ssl_options, Options0, []), - SNIFun = fun(SN) -> emqx_ocsp_cache:sni_fun(SN, ListenerID) end, + SNIFun = emqx_const_v1:make_sni_fun(ListenerID), Options1 = proplists:delete(ssl_options, Options0), Options = [{ssl_options, [{sni_fun, SNIFun} | SSLOpts0]} | Options1], %% save to env @@ -177,25 +177,6 @@ handle_info(_Info, State) -> {noreply, State}. code_change(_Vsn, State, _Extra) -> - %% we need to re-create the `sni_fun' lambda that the SSL - %% listeners are holding onto to avoid them becoming `badfun''s. - ListenersToPatch = - lists:filter( - fun(#{opts := Opts}) -> - OCSPOpts = proplists:get_value(ocsp_options, Opts), - undefined =/= proplists:get_value(ocsp_responder_url, OCSPOpts, undefined) andalso - false =/= proplists:get_bool(ocsp_stapling_enabled, OCSPOpts) - end, - emqx:get_env(listeners, [])), - PatchedListeners = [L#{opts => ?MODULE:inject_sni_fun( - emqx_listeners:identifier(L), - Opts)} - || L = #{opts := Opts} <- ListenersToPatch], - lists:foreach( - fun(L) -> - emqx_listeners:update_listeners_env(update, L) - end, - PatchedListeners), {ok, State}. %%-------------------------------------------------------------------- From 2b30f95dee51ea332bb96c2dac07ef7e37c70b99 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 16:45:03 -0300 Subject: [PATCH 14/16] test(ocsp): another fix for macos openssl versions Co-authored-by: Ivan Dyachkov --- test/emqx_ocsp_cache_SUITE.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index 42c201ac3..a84f6f19b 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -308,7 +308,7 @@ get_sni_fun(ListenerID) -> proplists:get_value(sni_fun, SSLOpts). openssl_version() -> - Res0 = os:cmd("openssl version"), + Res0 = string:trim(os:cmd("openssl version"), trailing), [_, Res] = string:split(Res0, " "), {match, [Version]} = re:run(Res, "^([^ ]+)", [{capture, first, list}]), Version. From fb81c35cd7d0d6e2c86aff5c599c7d54c7271801 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 8 Nov 2022 20:26:19 +0100 Subject: [PATCH 15/16] refactor: introduce emqx_const_v1 for immutable anonymous funs --- test/emqx_ocsp_cache_SUITE.erl | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/emqx_ocsp_cache_SUITE.erl b/test/emqx_ocsp_cache_SUITE.erl index a84f6f19b..f61282a4f 100644 --- a/test/emqx_ocsp_cache_SUITE.erl +++ b/test/emqx_ocsp_cache_SUITE.erl @@ -485,13 +485,6 @@ t_sni_fun_http_error(_Config) -> emqx_ocsp_cache:sni_fun(ServerName, ListenerID)), ok. -t_code_change(_Config) -> - ListenerID = <<"mqtt:ssl:test_ocsp">>, - SNIFun0 = get_sni_fun(ListenerID), - ?assertMatch({ok, _}, emqx_ocsp_cache:code_change(vsn, state, extra)), - SNIFun1 = get_sni_fun(ListenerID), - ?assertNotEqual(SNIFun0, SNIFun1). - t_openssl_client(Config) -> TLSVsn = ?config(tls_vsn, Config), WithStatusRequest = ?config(status_request, Config), From df58d19f89a8c64648150072b56f8724596c4215 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Tue, 8 Nov 2022 17:01:21 -0300 Subject: [PATCH 16/16] fix(listener_ocsp): assert listener id is present in options (ssl) --- src/emqx_listeners.erl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/emqx_listeners.erl b/src/emqx_listeners.erl index 1a5b52788..a2a497d8b 100644 --- a/src/emqx_listeners.erl +++ b/src/emqx_listeners.erl @@ -90,7 +90,8 @@ ensure_all_started() -> ensure_all_started([], []) -> ok; ensure_all_started([], Failed) -> error(Failed); ensure_all_started([L | Rest], Results) -> - #{proto := Proto, listen_on := ListenOn, opts := Options} = L, + #{proto := Proto, listen_on := ListenOn, opts := Options0} = L, + Options = [{listener_id, identifier(L)} | Options0], NewResults = case start_listener(Proto, ListenOn, Options) of {ok, _Pid} -> @@ -135,7 +136,7 @@ start_listener(tcp, ListenOn, Options) -> %% Start MQTT/TLS listener start_listener(Proto, ListenOn, Options0) when Proto == ssl; Proto == tls -> - ListenerID = proplists:get_value(listener_id, Options0, <<"mqtt:ssl:external">>), + ListenerID = proplists:get_value(listener_id, Options0), Options1 = proplists:delete(listener_id, Options0), Options = emqx_ocsp_cache:inject_sni_fun(ListenerID, Options1), start_mqtt_listener('mqtt:ssl', ListenOn, Options);