From 6813ea8e7a76eb5348bda754fa3ff9ae4a2c4d4f Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 16 May 2023 13:25:33 +0300 Subject: [PATCH 1/5] test(client): dedicate separate testcase to peercert cleaning --- apps/emqx/test/emqx_client_SUITE.erl | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/apps/emqx/test/emqx_client_SUITE.erl b/apps/emqx/test/emqx_client_SUITE.erl index ca5f53070..ed50eb8fe 100644 --- a/apps/emqx/test/emqx_client_SUITE.erl +++ b/apps/emqx/test/emqx_client_SUITE.erl @@ -75,7 +75,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_no_peercert_after_connected ]} ]. @@ -379,6 +380,23 @@ t_certcn_as_clientid_tlsv1_3(_) -> t_certcn_as_clientid_tlsv1_2(_) -> tls_certcn_as_clientid('tlsv1.2'). +t_no_peercert_after_connected(_) -> + emqx_config:put_zone_conf(default, [mqtt], #{}), + 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), + [ConnPid] = emqx_cm:lookup_channels(ClientId), + ?assertMatch( + #{conninfo := ConnInfo} when not is_map_key(peercert, ConnInfo), + emqx_connection:info(ConnPid) + ). + %%-------------------------------------------------------------------- %% Helper functions %%-------------------------------------------------------------------- @@ -421,10 +439,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). From b341a0495541c06d5ebac5a9cab7431bfae39954 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 16 May 2023 13:26:22 +0300 Subject: [PATCH 2/5] fix(chan): postpone trimming conninfo until `connected` hooks run Some users expect to get the peer certificate in `connected` hooks, but the `conninfo` was trimmed before `connected` hooks run. --- apps/emqx/src/emqx_channel.erl | 17 +++++++++++++---- apps/emqx/src/emqx_types.erl | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) 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(), From 967b2e72e0245d3034d9cb11011b7bf01d311db9 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 16 May 2023 13:28:22 +0300 Subject: [PATCH 3/5] test(emqx): remove `peercert` from clientinfo fixtures According to typespec, there's no place for `peercert` in `clientinfo()`. --- apps/emqx/test/emqx_access_control_SUITE.erl | 1 - apps/emqx/test/emqx_channel_SUITE.erl | 1 - apps/emqx/test/emqx_connection_SUITE.erl | 1 - apps/emqx/test/emqx_ws_connection_SUITE.erl | 12 ------------ 4 files changed, 15 deletions(-) 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_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, #{ From 74c04b847c75d72ac879dc5ec8b9e15d309f76e6 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 16 May 2023 13:41:19 +0300 Subject: [PATCH 4/5] chore: add changelog entry Co-authored-by: Zaiming (Stone) Shi --- changes/ce/fix-10715.en.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/ce/fix-10715.en.md 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. From 3cd95f40e5a6df69397cbb761b2839abe5d83668 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 16 May 2023 18:05:43 +0300 Subject: [PATCH 5/5] test(chan): verify hooks receive peercert until connected --- apps/emqx/include/asserts.hrl | 30 ++++++++++++++++++++++++++++ apps/emqx/test/emqx_client_SUITE.erl | 27 ++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 3 deletions(-) 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/test/emqx_client_SUITE.erl b/apps/emqx/test/emqx_client_SUITE.erl index ed50eb8fe..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"). @@ -76,7 +78,7 @@ groups() -> t_certcn_as_clientid_default_config_tls, t_certcn_as_clientid_tlsv1_3, t_certcn_as_clientid_tlsv1_2, - t_no_peercert_after_connected + t_peercert_preserved_before_connected ]} ]. @@ -380,8 +382,18 @@ t_certcn_as_clientid_tlsv1_3(_) -> t_certcn_as_clientid_tlsv1_2(_) -> tls_certcn_as_clientid('tlsv1.2'). -t_no_peercert_after_connected(_) -> - emqx_config:put_zone_conf(default, [mqtt], #{}), +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([ @@ -391,12 +403,21 @@ t_no_peercert_after_connected(_) -> {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 %%--------------------------------------------------------------------