remove skf from config service as creds are rolled

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,15 +141,15 @@ 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) ->
     Device#device_v7.dev_nonces.
 
 -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
 %% ------------------------------------------------------------------

src/device/router_device_worker.erl

@@ -1332,21 +1332,21 @@ handle_join(
     ),
     {ok, DevAddr} = router_device_devaddr:allocate(Device0, PubKeyBin),
 
-    <<DevAddrInt:32/integer-unsigned-big>> = 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) ->
+        <<Int:32/integer-unsigned-big>> = 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.
+    <<DevAddrInt:32/integer-unsigned-big>> = 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.

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]),

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,