diff --git a/apps/emqx/include/asserts.hrl b/apps/emqx/include/asserts.hrl index 98d8e72fc..4936da1f9 100644 --- a/apps/emqx/include/asserts.hrl +++ b/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)() +). diff --git a/apps/emqx/src/emqx_channel.erl b/apps/emqx/src/emqx_channel.erl index 3fb6a5f6b..8efe02ea6 100644 --- a/apps/emqx/src/emqx_channel.erl +++ b/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, - %% Saving a copy here causes unnecessary wast of memory (about 1KB per connection). - conninfo = maps:put(peercert, undefined, NConnInfo), + conninfo = 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 diff --git a/apps/emqx/src/emqx_types.erl b/apps/emqx/src/emqx_types.erl index 96d75daba..b1e2c7534 100644 --- a/apps/emqx/src/emqx_types.erl +++ b/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(), diff --git a/apps/emqx/test/emqx_access_control_SUITE.erl b/apps/emqx/test/emqx_access_control_SUITE.erl index 4bf1e05fd..f5fdf223b 100644 --- a/apps/emqx/test/emqx_access_control_SUITE.erl +++ b/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 diff --git a/apps/emqx/test/emqx_channel_SUITE.erl b/apps/emqx/test/emqx_channel_SUITE.erl index 2b7280b32..e822bf5e7 100644 --- a/apps/emqx/test/emqx_channel_SUITE.erl +++ b/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 diff --git a/apps/emqx/test/emqx_client_SUITE.erl b/apps/emqx/test/emqx_client_SUITE.erl index ca5f53070..14617a152 100644 --- a/apps/emqx/test/emqx_client_SUITE.erl +++ b/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). diff --git a/apps/emqx/test/emqx_connection_SUITE.erl b/apps/emqx/test/emqx_connection_SUITE.erl index 0692ec8f5..de3672bf3 100644 --- a/apps/emqx/test/emqx_connection_SUITE.erl +++ b/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, #{ diff --git a/apps/emqx/test/emqx_ws_connection_SUITE.erl b/apps/emqx/test/emqx_ws_connection_SUITE.erl index 60abe3d3c..bc91bc0ef 100644 --- a/apps/emqx/test/emqx_ws_connection_SUITE.erl +++ b/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, #{ diff --git a/changes/ce/fix-10715.en.md b/changes/ce/fix-10715.en.md new file mode 100644 index 000000000..68df92df6 --- /dev/null +++ b/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.