Merge pull request #10715 from fix/EMQX-9897/preserve-peercert-until-connected

fix(chan): postpone trimming conninfo after `connected` hook run

apps/emqx/include/asserts.hrl

@@ -29,3 +29,33 @@
         )
     )
 ).
+
+-define(drainMailbox(),
+    (fun F__Flush_() ->
+        receive
+            X__Msg_ -> [X__Msg_ | F__Flush_()]
+        after 0 -> []
+        end
+    end)()
+).
+
+-define(assertReceive(PATTERN),
+    ?assertReceive(PATTERN, 1000)
+).
+
+-define(assertReceive(PATTERN, TIMEOUT),
+    (fun() ->
+        receive
+            X__V = PATTERN -> X__V
+        after TIMEOUT ->
+            erlang:error(
+                {assertReceive, [
+                    {module, ?MODULE},
+                    {line, ?LINE},
+                    {expression, (??PATTERN)},
+                    {mailbox, ?drainMailbox()}
+                ]}
+            )
+        end
+    end)()
+).

apps/emqx/src/emqx_channel.erl

@@ -256,9 +256,7 @@ init(
     ),
     {NClientInfo, NConnInfo} = take_ws_cookie(ClientInfo, ConnInfo),
     #channel{
-        %% We remove the peercert because it duplicates to what's stored in the socket,
+        conninfo = NConnInfo,
-        %% Saving a copy here causes unnecessary wast of memory (about 1KB per connection).
-        conninfo = maps:put(peercert, undefined, NConnInfo),
         clientinfo = NClientInfo,
         topic_aliases = #{
             inbound => #{},
@@ -1989,10 +1987,21 @@ ensure_connected(
     NConnInfo = ConnInfo#{connected_at => erlang:system_time(millisecond)},
     ok = run_hooks('client.connected', [ClientInfo, NConnInfo]),
     Channel#channel{
-        conninfo = NConnInfo,
+        conninfo = trim_conninfo(NConnInfo),
         conn_state = connected
     }.
 
+trim_conninfo(ConnInfo) ->
+    maps:without(
+        [
+            %% NOTE
+            %% We remove the peercert because it duplicates what's stored in the socket,
+            %% otherwise it wastes about 1KB per connection.
+            peercert
+        ],
+        ConnInfo
+    ).
+
 %%--------------------------------------------------------------------
 %% Init Alias Maximum
 

apps/emqx/src/emqx_types.erl

@@ -129,7 +129,7 @@
     socktype := socktype(),
     sockname := peername(),
     peername := peername(),
-    peercert := nossl | undefined | esockd_peercert:peercert(),
+    peercert => nossl | undefined | esockd_peercert:peercert(),
     conn_mod := module(),
     proto_name => binary(),
     proto_ver => proto_ver(),

apps/emqx/test/emqx_access_control_SUITE.erl

@@ -116,7 +116,6 @@ clientinfo(InitProps) ->
             username => <<"username">>,
             password => <<"passwd">>,
             is_superuser => false,
-            peercert => undefined,
             mountpoint => undefined
         },
         InitProps

apps/emqx/test/emqx_channel_SUITE.erl

@@ -1211,7 +1211,6 @@ clientinfo(InitProps) ->
             clientid => <<"clientid">>,
             username => <<"username">>,
             is_superuser => false,
-            peercert => undefined,
             mountpoint => undefined
         },
         InitProps

apps/emqx/test/emqx_client_SUITE.erl

@@ -22,6 +22,8 @@
 -import(lists, [nth/2]).
 
 -include_lib("emqx/include/emqx_mqtt.hrl").
+-include_lib("emqx/include/emqx_hooks.hrl").
+-include_lib("emqx/include/asserts.hrl").
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("common_test/include/ct.hrl").
 -include_lib("snabbkaffe/include/snabbkaffe.hrl").
@@ -75,7 +77,8 @@ groups() ->
             t_username_as_clientid,
             t_certcn_as_clientid_default_config_tls,
             t_certcn_as_clientid_tlsv1_3,
-            t_certcn_as_clientid_tlsv1_2
+            t_certcn_as_clientid_tlsv1_2,
+            t_peercert_preserved_before_connected
         ]}
     ].
 
@@ -379,6 +382,42 @@ t_certcn_as_clientid_tlsv1_3(_) ->
 t_certcn_as_clientid_tlsv1_2(_) ->
     tls_certcn_as_clientid('tlsv1.2').
 
+t_peercert_preserved_before_connected(_) ->
+    ok = emqx_config:put_zone_conf(default, [mqtt], #{}),
+    ok = emqx_hooks:add(
+        'client.connect',
+        {?MODULE, on_hook, ['client.connect', self()]},
+        ?HP_HIGHEST
+    ),
+    ok = emqx_hooks:add(
+        'client.connected',
+        {?MODULE, on_hook, ['client.connected', self()]},
+        ?HP_HIGHEST
+    ),
+    ClientId = atom_to_binary(?FUNCTION_NAME),
+    SslConf = emqx_common_test_helpers:client_ssl_twoway(default),
+    {ok, Client} = emqtt:start_link([
+        {port, 8883},
+        {clientid, ClientId},
+        {ssl, true},
+        {ssl_opts, SslConf}
+    ]),
+    {ok, _} = emqtt:connect(Client),
+    _ = ?assertReceive({'client.connect', #{peercert := PC}} when is_binary(PC)),
+    _ = ?assertReceive({'client.connected', #{peercert := PC}} when is_binary(PC)),
+    [ConnPid] = emqx_cm:lookup_channels(ClientId),
+    ?assertMatch(
+        #{conninfo := ConnInfo} when not is_map_key(peercert, ConnInfo),
+        emqx_connection:info(ConnPid)
+    ).
+
+on_hook(ConnInfo, _, 'client.connect' = HP, Pid) ->
+    _ = Pid ! {HP, ConnInfo},
+    ok;
+on_hook(_ClientInfo, ConnInfo, 'client.connected' = HP, Pid) ->
+    _ = Pid ! {HP, ConnInfo},
+    ok.
+
 %%--------------------------------------------------------------------
 %% Helper functions
 %%--------------------------------------------------------------------
@@ -421,10 +460,4 @@ tls_certcn_as_clientid(TLSVsn, RequiredTLSVsn) ->
     {ok, _} = emqtt:connect(Client),
     #{clientinfo := #{clientid := CN}} = emqx_cm:get_chan_info(CN),
     confirm_tls_version(Client, RequiredTLSVsn),
-    %% verify that the peercert won't be stored in the conninfo
-    [ChannPid] = emqx_cm:lookup_channels(CN),
-    SysState = sys:get_state(ChannPid),
-    ChannelRecord = lists:keyfind(channel, 1, tuple_to_list(SysState)),
-    ConnInfo = lists:nth(2, tuple_to_list(ChannelRecord)),
-    ?assertMatch(#{peercert := undefined}, ConnInfo),
     emqtt:disconnect(Client).

apps/emqx/test/emqx_connection_SUITE.erl

@@ -676,7 +676,6 @@ channel(InitFields) ->
         clientid => <<"clientid">>,
         username => <<"username">>,
         is_superuser => false,
-        peercert => undefined,
         mountpoint => undefined
     },
     Conf = emqx_cm:get_session_confs(ClientInfo, #{

apps/emqx/test/emqx_ws_connection_SUITE.erl

@@ -33,17 +33,6 @@
     ]
 ).
 
--define(STATS_KEYS, [
-    recv_oct,
-    recv_cnt,
-    send_oct,
-    send_cnt,
-    recv_pkt,
-    recv_msg,
-    send_pkt,
-    send_msg
-]).
-
 -define(ws_conn, emqx_ws_connection).
 
 all() -> emqx_common_test_helpers:all(?MODULE).
@@ -618,7 +607,6 @@ channel(InitFields) ->
         clientid => <<"clientid">>,
         username => <<"username">>,
         is_superuser => false,
-        peercert => undefined,
         mountpoint => undefined
     },
     Conf = emqx_cm:get_session_confs(ClientInfo, #{

changes/ce/fix-10715.en.md

@@ -0,0 +1 @@
+Postpone trimming the connection information structure until after `client.connected` hooks have been executed. These hooks once again have access to the client's peer certificate.