Merge pull request #10550 from thalesmg/fix-ocsp-disabled-r50

fix(ocsp): disable periodic refresh when listener or stapling are disabled

apps/emqx/src/emqx_listeners.erl

@@ -441,6 +441,7 @@ post_config_update([listeners, Type, Name], {create, _Request}, NewConf, undefin
     start_listener(Type, Name, NewConf);
 post_config_update([listeners, Type, Name], {update, _Request}, NewConf, OldConf, _AppEnvs) ->
     try_clear_ssl_files(certs_dir(Type, Name), NewConf, OldConf),
+    ok = maybe_unregister_ocsp_stapling_refresh(Type, Name, NewConf),
     case NewConf of
         #{enabled := true} -> restart_listener(Type, Name, {OldConf, NewConf});
         _ -> ok
@@ -448,6 +449,7 @@ post_config_update([listeners, Type, Name], {update, _Request}, NewConf, OldConf
 post_config_update([listeners, _Type, _Name], '$remove', undefined, undefined, _AppEnvs) ->
     ok;
 post_config_update([listeners, Type, Name], '$remove', undefined, OldConf, _AppEnvs) ->
+    ok = unregister_ocsp_stapling_refresh(Type, Name),
     case stop_listener(Type, Name, OldConf) of
         ok ->
             _ = emqx_authentication:delete_chain(listener_id(Type, Name)),
@@ -460,10 +462,18 @@ post_config_update([listeners, Type, Name], {action, _Action, _}, NewConf, OldCo
     #{enabled := NewEnabled} = NewConf,
     #{enabled := OldEnabled} = OldConf,
     case {NewEnabled, OldEnabled} of
-        {true, true} -> restart_listener(Type, Name, {OldConf, NewConf});
+        {true, true} ->
-        {true, false} -> start_listener(Type, Name, NewConf);
+            ok = maybe_unregister_ocsp_stapling_refresh(Type, Name, NewConf),
-        {false, true} -> stop_listener(Type, Name, OldConf);
+            restart_listener(Type, Name, {OldConf, NewConf});
-        {false, false} -> stop_listener(Type, Name, OldConf)
+        {true, false} ->
+            ok = maybe_unregister_ocsp_stapling_refresh(Type, Name, NewConf),
+            start_listener(Type, Name, NewConf);
+        {false, true} ->
+            ok = unregister_ocsp_stapling_refresh(Type, Name),
+            stop_listener(Type, Name, OldConf);
+        {false, false} ->
+            ok = unregister_ocsp_stapling_refresh(Type, Name),
+            stop_listener(Type, Name, OldConf)
     end;
 post_config_update(_Path, _Request, _NewConf, _OldConf, _AppEnvs) ->
     ok.
@@ -813,3 +823,16 @@ inject_crl_config(
     };
 inject_crl_config(Conf) ->
     Conf.
+
+maybe_unregister_ocsp_stapling_refresh(
+    ssl = Type, Name, #{ssl_options := #{ocsp := #{enable_ocsp_stapling := false}}} = _Conf
+) ->
+    unregister_ocsp_stapling_refresh(Type, Name),
+    ok;
+maybe_unregister_ocsp_stapling_refresh(_Type, _Name, _Conf) ->
+    ok.
+
+unregister_ocsp_stapling_refresh(Type, Name) ->
+    ListenerId = listener_id(Type, Name),
+    emqx_ocsp_cache:unregister_listener(ListenerId),
+    ok.

apps/emqx/src/emqx_ocsp_cache.erl

@@ -30,6 +30,7 @@
     sni_fun/2,
     fetch_response/1,
     register_listener/2,
+    unregister_listener/1,
     inject_sni_fun/2
 ]).
 
@@ -107,6 +108,9 @@ fetch_response(ListenerID) ->
 register_listener(ListenerID, Opts) ->
     gen_server:call(?MODULE, {register_listener, ListenerID, Opts}, ?CALL_TIMEOUT).
 
+unregister_listener(ListenerID) ->
+    gen_server:cast(?MODULE, {unregister_listener, ListenerID}).
+
 -spec inject_sni_fun(emqx_listeners:listener_id(), map()) -> map().
 inject_sni_fun(ListenerID, Conf0) ->
     SNIFun = emqx_const_v1:make_sni_fun(ListenerID),
@@ -160,6 +164,18 @@ handle_call({register_listener, ListenerID, Conf}, _From, State0) ->
 handle_call(Call, _From, State) ->
     {reply, {error, {unknown_call, Call}}, State}.
 
+handle_cast({unregister_listener, ListenerID}, State0) ->
+    State2 =
+        case maps:take(?REFRESH_TIMER(ListenerID), State0) of
+            error ->
+                State0;
+            {TRef, State1} ->
+                emqx_utils:cancel_timer(TRef),
+                State1
+        end,
+    State = maps:remove({refresh_interval, ListenerID}, State2),
+    ?tp(ocsp_cache_listener_unregistered, #{listener_id => ListenerID}),
+    {noreply, State};
 handle_cast(_Cast, State) ->
     {noreply, State}.
 

apps/emqx/test/emqx_ocsp_cache_SUITE.erl

@@ -254,10 +254,15 @@ does_module_exist(Mod) ->
     end.
 
 assert_no_http_get() ->
+    Timeout = 0,
+    Error = should_be_cached,
+    assert_no_http_get(Timeout, Error).
+
+assert_no_http_get(Timeout, Error) ->
     receive
         {http_get, _URL} ->
-            error(should_be_cached)
+            error(Error)
-    after 0 ->
+    after Timeout ->
         ok
     end.
 
@@ -702,7 +707,9 @@ do_t_update_listener(Config) ->
                             %% the API converts that to an internally
                             %% managed file
                             <<"issuer_pem">> => IssuerPem,
-                            <<"responder_url">> => <<"http://localhost:9877">>
+                            <<"responder_url">> => <<"http://localhost:9877">>,
+                            %% for quicker testing; min refresh in tests is 5 s.
+                            <<"refresh_interval">> => <<"5s">>
                         }
                 }
         },
@@ -739,6 +746,70 @@ do_t_update_listener(Config) ->
         )
     ),
     assert_http_get(1, 5_000),
+
+    %% Disable OCSP Stapling; the periodic refreshes should stop
+    RefreshInterval = emqx_config:get([listeners, ssl, default, ssl_options, ocsp, refresh_interval]),
+    OCSPConfig1 =
+        #{
+            <<"ssl_options">> =>
+                #{
+                    <<"ocsp">> =>
+                        #{
+                            <<"enable_ocsp_stapling">> => false
+                        }
+                }
+        },
+    ListenerData3 = emqx_utils_maps:deep_merge(ListenerData2, OCSPConfig1),
+    {ok, {_, _, ListenerData4}} = update_listener_via_api(ListenerId, ListenerData3),
+    ?assertMatch(
+        #{
+            <<"ssl_options">> :=
+                #{
+                    <<"ocsp">> :=
+                        #{
+                            <<"enable_ocsp_stapling">> := false
+                        }
+                }
+        },
+        ListenerData4
+    ),
+
+    assert_no_http_get(2 * RefreshInterval, should_stop_refreshing),
+
+    ok.
+
+t_double_unregister(_Config) ->
+    ListenerID = <<"ssl:test_ocsp">>,
+    Conf = emqx_config:get_listener_conf(ssl, test_ocsp, []),
+    ?check_trace(
+        begin
+            {ok, {ok, _}} =
+                ?wait_async_action(
+                    emqx_ocsp_cache:register_listener(ListenerID, Conf),
+                    #{?snk_kind := ocsp_http_fetch_and_cache, listener_id := ListenerID},
+                    5_000
+                ),
+            assert_http_get(1),
+
+            {ok, {ok, _}} =
+                ?wait_async_action(
+                    emqx_ocsp_cache:unregister_listener(ListenerID),
+                    #{?snk_kind := ocsp_cache_listener_unregistered, listener_id := ListenerID},
+                    5_000
+                ),
+
+            %% Should be idempotent and not crash
+            {ok, {ok, _}} =
+                ?wait_async_action(
+                    emqx_ocsp_cache:unregister_listener(ListenerID),
+                    #{?snk_kind := ocsp_cache_listener_unregistered, listener_id := ListenerID},
+                    5_000
+                ),
+            ok
+        end,
+        []
+    ),
+
     ok.
 
 t_ocsp_responder_error_responses(_Config) ->