[GitHub] [couchdb] rnewson commented on a change in pull request #2875: Convert aegis key cach to LRU with hard expiration time

[GitBox <gi...@apache.org>]

rnewson commented on a change in pull request #2875:
URL: https://github.com/apache/couchdb/pull/2875#discussion_r421582281



##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -197,10 +205,21 @@ handle_call(_Msg, _From, St) ->
     {noreply, St}.
 
 
+handle_cast({accessed, UUID}, St) ->
+    NewSt = bump_last_accessed(St, UUID),
+    {noreply, NewSt};
+
+
 handle_cast(_Msg, St) ->
     {noreply, St}.
 
 
+handle_info(maybe_remove_expired, St) ->
+    remove_expired_entries(St),
+    CheckInterval = timer:seconds(expiration_check_interval()),

Review comment:
       there's a cleaner api for time unit conversion. is this done for compatibility reasons? (it's not clear from the funciton name that it does seconds to milliseconds)

##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -109,8 +115,10 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
         uuid := UUID
     } = Db,
 
-    case ets:member(?KEY_CHECK, UUID) of
-        true ->
+    Now = fabric2_util:now(sec),
+
+    case ets:lookup(?KEY_CHECK, UUID) of
+        [{UUID, ExpiresAt}] when ExpiresAt >= Now ->

Review comment:
       this pattern is reproduced twice in the module, can we extract it?

##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -120,7 +128,7 @@ decrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
                 {error, Reason} ->
                     erlang:error(Reason)
             end;
-        false ->
+        _ ->
             process_flag(sensitive, true),

Review comment:
       shouldn't we evict the entry here? 

##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -95,7 +101,7 @@ encrypt(#{} = Db, Key, Value) when is_binary(Key), is_binary(Value) ->
                 {error, Reason} ->
                     erlang:error(Reason)
             end;
-        false ->
+        _ ->

Review comment:
       Now that I look at it, we'll be exposing users to a high latency period every 30 minutes that they _don't_ revoke access to their key, which we expect to be the case almost every time.
   
   Like in epep, we had "fresh" "stale" and "expired" to cover this. A fresh item is used directly, as your first clause does. An "expired" one is not used (gets removed from cache if present, expensive external call must happen again). but a "stale" entry _is_ used and also kicks off a check that we still have access (if we do, nothing happens, if we don't, we evict our entry).

##########
File path: src/aegis/src/aegis_server.erl
##########
@@ -259,17 +278,125 @@ do_decrypt(DbKey, #{uuid := UUID}, Key, Value) ->
 
 %% cache functions
 
-insert(Cache, UUID, DbKey) ->
-    Entry = #entry{uuid = UUID, encryption_key = DbKey},
+insert(St, UUID, DbKey) ->
+    #{
+        cache := Cache,
+        by_access := ByAccess,
+        counter := Counter
+    } = St,
+
+    Now = fabric2_util:now(sec),
+    ExpiresAt = Now + max_age(),
+
+    Entry = #entry{
+        uuid = UUID,
+        encryption_key = DbKey,
+        counter = Counter,
+        last_accessed = Now,
+        expires_at = ExpiresAt
+    },
+
     true = ets:insert(Cache, Entry),
-    true = ets:insert(?KEY_CHECK, {UUID, true}),
-    ok.
+    true = ets:insert_new(ByAccess, Entry),
+    true = ets:insert(?KEY_CHECK, {UUID, ExpiresAt}),
+
+    maybe_evict_old_entries(St),

Review comment:
       how can this ever be more than 1 item? "old" here is misleading as it implies time is a factor, but we're really evicted the least recently used item to make way for this one. It seems we could make that clearer by checking ets:size here and removing the (single) oldest item if it goes over.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org