From 95c85e0d408b89937bd331c21eb25311ca9c0891 Mon Sep 17 00:00:00 2001 From: Michael Jeffrey Date: Wed, 31 May 2023 17:47:59 -0700 Subject: [PATCH] remove skf from config service as creds are rolled --- src/device/router_device.erl | 24 ++++++++++-- src/device/router_device_worker.erl | 44 +++++++++++++++++++--- src/grpc/router_ics_skf_worker.erl | 8 +--- test/router_device_worker_SUITE.erl | 58 ++++++++++++++++++++++++++++- 4 files changed, 116 insertions(+), 18 deletions(-) diff --git a/src/device/router_device.erl b/src/device/router_device.erl index b33edc239..ce82c046c 100644 --- a/src/device/router_device.erl +++ b/src/device/router_device.erl @@ -46,7 +46,9 @@ deserialize/1 ]). -export([ - can_queue_payload/3 + can_queue_payload/3, + credentials_to_evict/1, + limit_credentials/1 ]). %% ------------------------------------------------------------------ @@ -61,6 +63,7 @@ -define(QUEUE_SIZE_LIMIT, 20). -define(MAX_32_BITS, 16#100000000). +-define(MAX_CREDENTIAL_COUNT, 25). -type device() :: #device_v7{}. @@ -111,7 +114,7 @@ keys(#device_v7{keys = Keys}) -> -spec keys(list({binary(), binary()}), device()) -> device(). keys(Keys, Device) -> - Device#device_v7{keys = lists:sublist(Keys, 25)}. + Device#device_v7{keys = limit_credentials(Keys)}. -spec nwk_s_key(device()) -> binary() | undefined. nwk_s_key(#device_v7{keys = []}) -> @@ -138,7 +141,7 @@ devaddrs(Device) -> -spec devaddrs([binary()], device()) -> device(). devaddrs(Devaddrs, Device) -> - Device#device_v7{devaddrs = lists:sublist(Devaddrs, 25)}. + Device#device_v7{devaddrs = limit_credentials(Devaddrs)}. -spec dev_nonces(device()) -> [binary()]. dev_nonces(Device) -> @@ -146,7 +149,7 @@ dev_nonces(Device) -> -spec dev_nonces([binary()], device()) -> device(). dev_nonces(Nonces, Device) -> - Device#device_v7{dev_nonces = lists:sublist(Nonces, 25)}. + Device#device_v7{dev_nonces = limit_credentials(Nonces)}. -spec fcnt(device()) -> undefined | non_neg_integer(). fcnt(Device) -> @@ -508,6 +511,19 @@ can_queue_payload(Payload, DownlinkRegion, Device) -> {Size =< MaxSize, Size, MaxSize, DownDRIndex} end. +-spec credentials_to_evict(Creds :: list(any())) -> list(any()). +credentials_to_evict(Creds) -> + case erlang:length(Creds) of + N when N > ?MAX_CREDENTIAL_COUNT -> + lists:nthtail(?MAX_CREDENTIAL_COUNT, Creds); + _ -> + [] + end. + +-spec limit_credentials(Creds :: list(any())) -> list(any()). +limit_credentials(Creds) -> + lists:sublist(Creds, ?MAX_CREDENTIAL_COUNT). + %% ------------------------------------------------------------------ %% RocksDB Device Functions %% ------------------------------------------------------------------ diff --git a/src/device/router_device_worker.erl b/src/device/router_device_worker.erl index d1d754a51..5fe4faaf4 100644 --- a/src/device/router_device_worker.erl +++ b/src/device/router_device_worker.erl @@ -1332,21 +1332,21 @@ handle_join( ), {ok, DevAddr} = router_device_devaddr:allocate(Device0, PubKeyBin), - <> = lorawan_utils:reverse(DevAddr), - ok = router_ics_skf_worker:update([{add, DevAddrInt, NwkSKey}]), - lager:debug("sending add skf ~p ~p", [DevAddrInt, NwkSKey]), - DeviceName = router_device:name(APIDevice), %% don't set the join nonce here yet as we have not chosen the best join request yet {AppEUI, DevEUI} = {lorawan_utils:reverse(AppEUI0), lorawan_utils:reverse(DevEUI0)}, Region = dualplan_region(Packet, HotspotRegion), DRIdx = packet_datarate_index(Region, Packet), + + NewKeys = [{NwkSKey, AppSKey} | router_device:keys(Device0)], + NewDevaddrs = [DevAddr | router_device:devaddrs(Device0)], + DeviceUpdates = [ {name, DeviceName}, {dev_eui, DevEUI}, {app_eui, AppEUI}, - {keys, [{NwkSKey, AppSKey} | router_device:keys(Device0)]}, - {devaddrs, [DevAddr | router_device:devaddrs(Device0)]}, + {keys, NewKeys}, + {devaddrs, NewDevaddrs}, {fcnt, undefined}, {fcntdown, 0}, %% only do channel correction for 915 right now @@ -1363,6 +1363,9 @@ handle_join( {region, Region} ], Device1 = router_device:update(DeviceUpdates, Device0), + + ok = handle_join_skf(NewKeys, NewDevaddrs), + lager:debug( "Join DevEUI ~s with AppEUI ~s tried to join with nonce ~p region ~p via ~s", [ @@ -1380,6 +1383,35 @@ handle_join( app_key = AppKey }}. +%% When adding new credentials during a join, we want to remove the evicted +%% credentials from the config service. If a joining takes more than 25 +%% attempts, we won't have the information to remove all the unused keys. +-spec handle_join_skf( + NewKeys :: list({NwkSKey :: binary(), AppSKey :: binary()}), + NewDevAddrs :: list(DevAddr :: binary()) +) -> ok. +handle_join_skf([{NwkSKey, _} | _] = NewKeys, [NewDevAddr | _] = NewDevAddrs) -> + DevAddrToInt = fun(D) -> + <> = lorawan_utils:reverse(D), + Int + end, + + %% remove evicted keys from the config service for every devaddr. + EvictedKeys = router_device:credentials_to_evict(NewKeys), + ToRemoveKeys = [{remove, DevAddrToInt(D), NSK} || {NSK, _} <- EvictedKeys, D <- NewDevAddrs], + + %% remove evicted devaddrs from the config service for every nwkskey. + EvictedAddrs = router_device:credentials_to_evict(NewDevAddrs), + ToRemoveAddrs = [{remove, DevAddrToInt(D), NSK} || {NSK, _} <- NewKeys, D <- EvictedAddrs], + + %% add the new devaddr, nskwkey. + <> = lorawan_utils:reverse(NewDevAddr), + Updates = lists:usort([{add, DevAddrInt, NwkSKey}] ++ ToRemoveKeys ++ ToRemoveAddrs), + ok = router_ics_skf_worker:update(Updates), + + lager:debug("sending update skf for join ~p ~p", [Updates]), + ok. + %% Dual-Plan Code %% Logic to support the dual frequency plan which allows an AS923_1 Hotspot %% to support both AS923_1 and AU915 end-devices. diff --git a/src/grpc/router_ics_skf_worker.erl b/src/grpc/router_ics_skf_worker.erl index 878e4a92e..a915f8c5f 100644 --- a/src/grpc/router_ics_skf_worker.erl +++ b/src/grpc/router_ics_skf_worker.erl @@ -281,12 +281,8 @@ handle_cast( Updates0 ), - case send_update_request(RouteID, Updates1) of - {ok, Resp} -> - lager:info("updating skf good success: ~p", [Resp]); - {error, Err} -> - lager:error("updating skf bad failure: ~p", [Err]) - end, + %% logging is already done in send_update_request/2 + _ = send_update_request(RouteID, Updates1), {noreply, State}; handle_cast(_Msg, State) -> lager:warning("rcvd unknown cast msg: ~p", [_Msg]), diff --git a/test/router_device_worker_SUITE.erl b/test/router_device_worker_SUITE.erl index 6e6009580..b590ba6cd 100644 --- a/test/router_device_worker_SUITE.erl +++ b/test/router_device_worker_SUITE.erl @@ -22,7 +22,8 @@ offer_cache_test/1, load_offer_cache_test/1, hotspot_bad_region_test/1, - uplink_bad_mtype/1 + uplink_bad_mtype/1, + evict_keys_join_test/1 ]). -include_lib("helium_proto/include/blockchain_state_channel_v1_pb.hrl"). @@ -73,7 +74,8 @@ all_tests() -> offer_cache_test, load_offer_cache_test, hotspot_bad_region_test, - uplink_bad_mtype + uplink_bad_mtype, + evict_keys_join_test ]. %%-------------------------------------------------------------------- @@ -504,6 +506,58 @@ drop_downlink_test(Config) -> ok. +evict_keys_join_test(Config) -> + %% If a device get's stuck in a join loop, we will only keep the last 25 + %% devaddrs and key sets. When there is a successful uplink after joining, + %% all the keys not used will be removed. But if there were more than 25 + %% attempts, we cannot remove keys that have been cycled out. + #{} = test_utils:join_device(Config), + + %% Waiting for reply from router to hotspot + test_utils:wait_state_channel_message(1250), + + %% Check that device is in cache now + DeviceID = ?CONSOLE_DEVICE_ID, + {ok, Device0} = router_device_cache:get(DeviceID), + + Device1 = router_device:update( + [ + {keys, [ + {crypto:strong_rand_bytes(16), crypto:strong_rand_bytes(16)} + || _ <- lists:seq(1, 25) + ]}, + {devaddrs, [crypto:strong_rand_bytes(4) || _ <- lists:seq(1, 25)]} + ], + Device0 + ), + {ok, DB, CF} = router_db:get_devices(), + {ok, Device1} = router_device_cache:save(Device1), + {ok, Device1} = router_device:save(DB, CF, Device1), + + {ok, DeviceWorkerPid} = router_devices_sup:lookup_device_worker(DeviceID), + %% Stop the device worker so it will pick up the newly saved device from the cache. + ok = gen_server:stop(DeviceWorkerPid), + + %% Joining the device should evict 1 of the keys and devaddrs. + + ok = meck:new(router_ics_skf_worker, [passthrough]), + + #{} = test_utils:join_device(Config), + + [{_Pid, {router_ics_skf_worker, update, [Updates] = _Args}, ok}] = meck:history( + router_ics_skf_worker + ), + + %% Expected updates + %% 1 Add + %% 1 Remove {EvictedKey, EvictedDevaddr} + %% 25 Remove {EvictedKey, RemainingDevaddr} + %% 25 Remove {RemainingKey, EvictedDevaddr} + ?assertEqual(1 + 1 + 25 + 25, length(Updates)), + + ok = meck:unload(router_ics_skf_worker), + ok. + replay_joins_test(Config) -> #{ app_key := AppKey,