Merge pull request #13922 from zmstone/1003-crl-cache-with-full-url

fix: cache CRL with full distribution point URL

apps/emqx/src/emqx_crl_cache.erl

@@ -74,7 +74,7 @@
     %% for future use
     extra = #{} :: map()
 }).
--type url() :: uri_string:uri_string().
+-type url() :: string().
 -type state() :: #state{}.
 
 %%--------------------------------------------------------------------

apps/emqx/src/emqx_ssl_crl_cache.erl

@@ -18,7 +18,7 @@
 %% %CopyrightEnd%
 
 %%--------------------------------------------------------------------
-%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved.
+%% Copyright (c) 2023-2024 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.
@@ -39,6 +39,25 @@
 
 %%----------------------------------------------------------------------
 %% Purpose: Simple default CRL cache
+%%
+%% The cache is a part of an opaque term named DB created by `ssl_manager'
+%% from calling `ssl_pkix_db:create/1'.
+%%
+%% Insert and delete operations are abstracted by `ssl_manager'.
+%% Read operation is done by passing-through the DB term to
+%% `ssl_pkix_db:lookup/2'.
+%%
+%% The CRL cache in the DB term is essentially an ETS table.
+%% The table is created as `ssl_otp_crl_cache', but not
+%% a named table. You can find the table reference from `ets:i()'.
+%%
+%% The cache key in the original OTP implementation was the path part of the
+%% CRL distribution point URL. e.g. if the URL is `http://foo.bar.com/crl.pem'
+%% the cache key would be `"crl.pem"'.
+%% There is however no type spec for the APIs, nor there is any check
+%% on the format, making it possible to use the full URL binary
+%% string as key instead --- which can avoid cache key clash when
+%% different DPs share the same path.
 %%----------------------------------------------------------------------
 
 -module(emqx_ssl_crl_cache).
@@ -142,8 +161,9 @@ delete({der, CRLs}) ->
     ssl_manager:delete_crls({?NO_DIST_POINT, CRLs});
 delete(URI) ->
     case uri_string:normalize(URI, [return_map]) of
-        #{scheme := "http", path := Path} ->
-            ssl_manager:delete_crls(string:trim(Path, leading, "/"));
+        #{scheme := "http", path := _} ->
+            Key = cache_key(URI),
+            ssl_manager:delete_crls(Key);
         _ ->
             {error, {only_http_distribution_points_supported, URI}}
     end.
@@ -153,8 +173,9 @@ delete(URI) ->
 %%--------------------------------------------------------------------
 do_insert(URI, CRLs) ->
     case uri_string:normalize(URI, [return_map]) of
-        #{scheme := "http", path := Path} ->
-            ssl_manager:insert_crls(string:trim(Path, leading, "/"), CRLs);
+        #{scheme := "http", path := _} ->
+            Key = cache_key(URI),
+            ssl_manager:insert_crls(Key, CRLs);
         _ ->
             {error, {only_http_distribution_points_supported, URI}}
     end.
@@ -218,8 +239,7 @@ http_get(URL, Rest, CRLDbInfo, Timeout) ->
 cache_lookup(_, undefined) ->
     [];
 cache_lookup(URL, {{Cache, _}, _}) ->
-    #{path := Path} = uri_string:normalize(URL, [return_map]),
-    case ssl_pkix_db:lookup(string:trim(Path, leading, "/"), Cache) of
+    case ssl_pkix_db:lookup(cache_key(URL), Cache) of
         undefined ->
             [];
         [CRLs] ->
@@ -235,3 +255,6 @@ handle_http(URI, Rest, {_, [{http, Timeout}]} = CRLDbInfo) ->
     CRLs;
 handle_http(_, Rest, CRLDbInfo) ->
     get_crls(Rest, CRLDbInfo).
+
+cache_key(URL) ->
+    iolist_to_binary(URL).

apps/emqx/test/emqx_crl_cache_SUITE.erl

@@ -465,6 +465,7 @@ t_manual_refresh(Config) ->
     emqx_config_handler:start_link(),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
+    URLBin = iolist_to_binary(URL),
     ok = snabbkaffe:start_trace(),
     ?wait_async_action(
         ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
@@ -472,10 +473,7 @@ t_manual_refresh(Config) ->
         5_000
     ),
     ok = snabbkaffe:stop(),
-    ?assertEqual(
-        [{"crl.pem", [CRLDer]}],
-        ets:tab2list(Ref)
-    ),
+    ?assertEqual([{URLBin, [CRLDer]}], ets:tab2list(Ref)),
     emqx_config_handler:stop(),
     ok.
 
@@ -579,13 +577,14 @@ t_evict(_Config) ->
     emqx_config_handler:start_link(),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
+    URLBin = iolist_to_binary(URL),
     ?wait_async_action(
         ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
         #{?snk_kind := crl_cache_insert},
         5_000
     ),
     Ref = get_crl_cache_table(),
-    ?assertMatch([{"crl.pem", _}], ets:tab2list(Ref)),
+    ?assertMatch([{URLBin, _}], ets:tab2list(Ref)),
     {ok, {ok, _}} = ?wait_async_action(
         emqx_crl_cache:evict(URL),
         #{?snk_kind := crl_cache_evict}

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

@@ -514,35 +514,38 @@ t_persistent_sessions5(Config) ->
             %% Disconnect persistent sessions
             lists:foreach(fun stop_and_commit/1, [C1, C2]),
 
+            %% the order of the durable session list is not stable
+            %% so we make sure one request is to list all in-mem,
+            %% and then the next is to list all durable.
             P3 =
                 ?retry(200, 10, begin
-                    P3_ = list_request(#{limit => 3, page => 1}, Config),
+                    P3a = list_request(#{limit => 2, page => 1}, Config),
                     ?assertMatch(
                         {ok,
                             {?HTTP200, _, #{
-                                <<"data">> := [_, _, _],
+                                <<"data">> := [_, _],
                                 <<"meta">> := #{
                                     <<"count">> := 4
                                 }
                             }}},
-                        P3_
+                        P3a
                     ),
-                    P3_
+                    P3a
                 end),
             P4 =
                 ?retry(200, 10, begin
-                    P4_ = list_request(#{limit => 3, page => 2}, Config),
+                    P4a = list_request(#{limit => 2, page => 2}, Config),
                     ?assertMatch(
                         {ok,
                             {?HTTP200, _, #{
-                                <<"data">> := [_],
+                                <<"data">> := [_, _],
                                 <<"meta">> := #{
                                     <<"count">> := 4
                                 }
                             }}},
-                        P4_
+                        P4a
                     ),
-                    P4_
+                    P4a
                 end),
             {ok, {_, _, #{<<"data">> := R3}}} = P3,
             {ok, {_, _, #{<<"data">> := R4}}} = P4,

changes/ce/fix-13922.en.md

@@ -0,0 +1,3 @@
+Change CRL (Certificate Revocation List) cache with full DP (distribution point) URL.
+
+Prior to this fix, the cache key was the path part of the URL, which leads to clash if there happen to be more than one DP sharing the same path.