MB-61292: Add integrity check for stored keys

timofey-barmin · timofey-barmin · commit 73411c1e7a5e · 2024-12-20T01:23:38.000Z
This commit adds an integrity check for stored keys. The check is performed every time the stored keys are loaded from disk. The check is done by comparing the stored key proof with the proof that is calculated using token from the tokens file. Proof for Key with KeyId is calculated the following way: Proof := TokenUUID + ENCRYPT(SHA512(Token + KeyId) with Key) Token file contains all the tokens that are considered trusted for the integrity check. Tokens are stored in the following format: TokenUUID1:Token1\n TokenUUID2:Token2\n ... Token file is encrypted with current config dek. If configuration encryption is disabled, tokens are stored in plain, so there basically no integrity protection for keys in this case. When configuration encryption gets enabled, we generate new configuration key first, then we generate new token, and then we store that new token in the tokens file encrypted with the newly generated congifuration key. By doing so we are making sure that new token is never stored in plain text. Old tokens (that were stored in plain text) are removed from the tokens file once all the existing keys have their proofs recalculated with the latest token. Rotation of tokens is performed on any config key generation, or config encryption enabling/disabling. Change-Id: I3de47705964337f20b4eb035115210549979f5c5 Reviewed-on: https://review.couchbase.org/c/ns_server/+/220792 Tested-by: Timofey Barmin <timofey.barmin@couchbase.com> Tested-by: Build Bot <build@couchbase.com> Reviewed-by: Navdeep S Boparai <navdeep.boparai@couchbase.com> Well-Formed: Build Bot <build@couchbase.com>
diff --git a/apps/ns_babysitter/src/cb_gosecrets_runner.erl b/apps/ns_babysitter/src/cb_gosecrets_runner.erl
@@ -38,7 +38,10 @@
          decrypt_with_key/5,
          read_key/3,
          defaults/0,
-         key_path/2]).
+         key_path/2,
+         rotate_integrity_tokens/2,
+         remove_old_integrity_tokens/2,
+         get_key_id_in_use/1]).
 
 -record(state, {config :: file:filename(),
                 loop :: pid() | undefined,
@@ -135,6 +138,15 @@ decrypt_with_key(Name, Data, AD, KeyKind, KeyName) ->
     gen_server:call(Name, {decrypt_with_key, Data, AD, KeyKind, KeyName},
                     infinity).
 
+rotate_integrity_tokens(Name, KeyName) ->
+    gen_server:call(Name, {rotate_integrity_tokens, KeyName}, infinity).
+
+remove_old_integrity_tokens(Name, Paths) ->
+    gen_server:call(Name, {remove_old_integrity_tokens, Paths}, infinity).
+
+get_key_id_in_use(Name) ->
+    gen_server:call(Name, get_key_id_in_use, infinity).
+
 start_link(Logger, PasswordPromptAllowed) ->
     start_link(Logger, PasswordPromptAllowed, gosecrets_cfg_path()).
 
@@ -352,6 +364,12 @@ handle_call({encrypt_with_key, _Data, _AD, _KeyKind, _Name} = Cmd, _From,
 handle_call({decrypt_with_key, _Data, _AD, _KeyKind, _Name} = Cmd, _From,
             State) ->
     {reply, convert_empty_data(call_gosecrets(Cmd, State)), State};
+handle_call({rotate_integrity_tokens, _KeyName} = Cmd, _From, State) ->
+    {reply, call_gosecrets(Cmd, State), State};
+handle_call({remove_old_integrity_tokens, _Paths} = Cmd, _From, State) ->
+    {reply, call_gosecrets(Cmd, State), State};
+handle_call(get_key_id_in_use, _From, State) ->
+    {reply, convert_empty_data(call_gosecrets(get_key_id_in_use, State)), State};
 handle_call(stop, _From, State) ->
     {stop, normal, call_gosecrets(stop, State), State};
 handle_call(Call, _From, State) ->
@@ -493,7 +511,14 @@ encode({decrypt_with_key, Data, AD, KeyKind, Name}) ->
           (encode_param(KeyKind))/binary,
           (encode_param(Name))/binary>>;
 encode({read_key, Kind, Name}) ->
-    <<15, (encode_param(Kind))/binary, (encode_param(Name))/binary>>.
+    <<15, (encode_param(Kind))/binary, (encode_param(Name))/binary>>;
+encode({rotate_integrity_tokens, KeyName}) ->
+    <<17, (encode_param(KeyName))/binary>>;
+encode({remove_old_integrity_tokens, Paths}) ->
+    PathsBin = list_to_binary([encode_param(P) || P <- Paths]),
+    <<18, PathsBin/binary>>;
+encode(get_key_id_in_use) ->
+    <<19>>.
 
 encode_param(B) when is_atom(B) ->
     encode_param(atom_to_binary(B));
@@ -853,6 +878,86 @@ config_reload_test() ->
           ?assertEqual({ok, <<"default">>}, get_state(Pid))
       end).
 
+-define(key1, <<"key10000-0000-0000-0000-000000000000">>).
+-define(key2, <<"key20000-0000-0000-0000-000000000000">>).
+-define(key3, <<"key30000-0000-0000-0000-000000000000">>).
+-define(key4, <<"key40000-0000-0000-0000-000000000000">>).
+-define(key5, <<"key50000-0000-0000-0000-000000000000">>).
+-define(key6, <<"key60000-0000-0000-0000-000000000000">>).
+-define(key7, <<"key70000-0000-0000-0000-000000000000">>).
+
+integrity_check_for_stored_keys_test() ->
+    DKFile = path_config:tempfile("bucket_dek_test", ".tmp"),
+    ok = filelib:ensure_dir(DKFile),
+    Cfg = [{bucket_dek_path, iolist_to_binary(DKFile)}],
+    BucketPath = fun (Bucket) ->
+                     iolist_to_binary(filename:join([DKFile, Bucket, "deks"]))
+                 end,
+    KeyDirs = [key_path(configDek, Cfg),
+               key_path(logDek, Cfg),
+               key_path(auditDek, Cfg),
+               key_path(kek, Cfg),
+               BucketPath("bucket1"),
+               BucketPath("bucket2")],
+    with_all_stored_keys_cleaned(
+      Cfg,
+      fun () ->
+          with_gosecrets(
+            Cfg,
+            fun (CfgPath, Pid) ->
+                TokensPath = filename:join(filename:dirname(CfgPath),
+                                           "stored_keys_tokens"),
+                StoreKey = fun (Kind, KeyId, EncryptWithKey) ->
+                               store_key(Pid, Kind, KeyId, 'raw-aes-gcm',
+                                         rand:bytes(32),
+                                         EncryptWithKey,
+                                         <<"2024-07-26T19:32:19Z">>, false)
+                           end,
+
+                ok = StoreKey(configDek, ?key1, <<"encryptionService">>),
+
+                {ok, [undefined]} = cb_crypto:get_file_dek_ids(TokensPath),
+
+                %% First rotation with key1
+                ok = rotate_integrity_tokens(Pid, ?key1),
+                {ok, [?key1]} = cb_crypto:get_file_dek_ids(TokensPath),
+                ok = remove_old_integrity_tokens(Pid, KeyDirs),
+                {ok, [?key1]} = cb_crypto:get_file_dek_ids(TokensPath),
+
+                %% Store more keys of different kinds
+                ok = StoreKey(kek, ?key2, <<"encryptionService">>),
+                ok = StoreKey(configDek, ?key3, ?key2),
+                ok = StoreKey(logDek, ?key4, ?key2),
+                ok = StoreKey(auditDek, ?key5, ?key2),
+                Bucket1Key = <<"bucket1/deks/", ?key6/binary>>,
+                ok = StoreKey(bucketDek, Bucket1Key, ?key2),
+                Bucket2Key = <<"bucket2/deks/", ?key7/binary>>,
+                ok = StoreKey(bucketDek, Bucket2Key, ?key2),
+
+                %% Second rotation with key1
+                ok = rotate_integrity_tokens(Pid, ?key3),
+                {ok, [?key3]} = cb_crypto:get_file_dek_ids(TokensPath),
+
+                ok = remove_old_integrity_tokens(Pid, KeyDirs),
+                {ok, [?key3]} = cb_crypto:get_file_dek_ids(TokensPath),
+
+                %% Final rotation with empty key
+                ok = rotate_integrity_tokens(Pid, <<>>),
+                {ok, [undefined]} = cb_crypto:get_file_dek_ids(TokensPath),
+                ok = remove_old_integrity_tokens(Pid, KeyDirs),
+                {ok, [undefined]} = cb_crypto:get_file_dek_ids(TokensPath),
+
+                %% Verify all keys are still readable
+                {ok, _} = read_key(Pid, configDek, ?key1),
+                {ok, _} = read_key(Pid, kek, ?key2),
+                {ok, _} = read_key(Pid, configDek, ?key3),
+                {ok, _} = read_key(Pid, logDek, ?key4),
+                {ok, _} = read_key(Pid, auditDek, ?key5),
+                {ok, _} = read_key(Pid, bucketDek, Bucket1Key),
+                {ok, _} = read_key(Pid, bucketDek, Bucket2Key)
+            end)
+      end).
+
 store_and_read_key_test() ->
     Cfg = [],
     DecodeKey = fun (EncodedData) ->
@@ -1216,15 +1321,15 @@ with_tmp_datakey_cfg(Fun) ->
     end.
 
 with_all_stored_keys_cleaned(Cfg, Fun) ->
-    KeksPath = binary_to_list(key_path(kek, Cfg)),
-    DeksPath = binary_to_list(key_path(configDek, Cfg)),
-    ok = misc:rm_rf(KeksPath), %% In case if there are leftovers
-    ok = misc:rm_rf(DeksPath),
+    Keys = [configDek, logDek, auditDek, kek, bucketDek],
+    Paths = [binary_to_list(P) || Key <- Keys,
+                                  P <- [key_path(Key, Cfg)],
+                                  P =/= undefined],
+    [ok = misc:rm_rf(Path) || Path <- Paths],
     try
         Fun()
     after
-        ok = misc:rm_rf(KeksPath),
-        ok = misc:rm_rf(DeksPath)
+        [ok = misc:rm_rf(Path) || Path <- Paths]
     end.
 
 with_password_sent(WrongPassword, CorrectPassword, Fun) ->
@@ -1313,6 +1418,8 @@ with_tmp_cfg(Cfg, Fun) ->
         Fun(CfgPath)
     after
         delete_all_default_files(),
+        file:delete(filename:join(filename:dirname(CfgPath),
+                                  "stored_keys_tokens")),
         file:delete(CfgPath)
     end.
 
diff --git a/apps/ns_server/src/cb_cluster_secrets.erl b/apps/ns_server/src/cb_cluster_secrets.erl
@@ -1263,6 +1263,7 @@ maybe_update_deks(Kind, #state{deks = CurDeks} = OldState) ->
                 %% On disk it is enabled but in config it is disabled:
                 true when EncrMethod == disabled ->
                     NewState = set_active(Kind, ActiveId, false, State),
+                    ok = maybe_rotate_integrity_tokens(Kind, undefined),
                     call_set_active_cb(Kind, NewState);
 
                 %% It is enabled on disk and in config:
@@ -1283,6 +1284,7 @@ maybe_update_deks(Kind, #state{deks = CurDeks} = OldState) ->
                 %% and we already have a dek
                 false when is_binary(ActiveId) and not ShouldRotate ->
                     NewState = set_active(Kind, ActiveId, true, State),
+                    ok = maybe_rotate_integrity_tokens(Kind, ActiveId),
                     call_set_active_cb(Kind, NewState);
 
                 %% On disk it is disabled but in config it is enabled
@@ -1293,6 +1295,7 @@ maybe_update_deks(Kind, #state{deks = CurDeks} = OldState) ->
                     case generate_new_dek(Kind, Deks, EncrMethod, Snapshot) of
                         {ok, DekId} ->
                             NewState = set_active(Kind, DekId, true, State),
+                            ok = maybe_rotate_integrity_tokens(Kind, DekId),
                             call_set_active_cb(Kind, NewState);
                         %% Too many DEKs and encryption is being enabled
                         %% We could not create new DEK, but should still
@@ -1496,6 +1499,13 @@ call_set_active_cb(Kind, #state{deks = AllDeks} = State) ->
             {error, State, Reason}
     end.
 
+-spec maybe_rotate_integrity_tokens(cb_deks:dek_kind(),
+                                    cb_deks:dek_id() | undefined) -> ok.
+maybe_rotate_integrity_tokens(configDek, DekId) ->
+    ok = encryption_service:maybe_rotate_integrity_tokens(DekId);
+maybe_rotate_integrity_tokens(_Kind, _DekId) ->
+    ok.
+
 call_dek_callback(CallbackName, Kind, Args) ->
     #{CallbackName := CB} = cb_deks:dek_config(Kind),
     try erlang:apply(CB, Args) of
@@ -1597,6 +1607,19 @@ maybe_read_deks(#state{proc_type = ?NODE_PROC, deks = undefined} = State) ->
                       NewAcc
                   end, NewState2, Kinds),
     ok = garbage_collect_keks(),
+
+    %% We should rotate here (when process is starting) because we can
+    %% hypothetically crash after calling set_active() but before calling
+    %% maybe_rotate_integrity_tokens() below
+    case NewState3 of
+        #state{deks = #{configDek := #{is_enabled := true,
+                                        active_id := ActiveId}}} ->
+            ok = maybe_rotate_integrity_tokens(configDek, ActiveId);
+        #state{deks = #{configDek := #{is_enabled := false}}} ->
+            ok = maybe_rotate_integrity_tokens(configDek, undefined);
+        #state{} ->
+            ok
+    end,
     add_jobs([{maybe_update_deks, K} || K <- Kinds], NewState3);
 maybe_read_deks(#state{} = State) ->
     State.
diff --git a/apps/ns_server/src/cb_deks.erl b/apps/ns_server/src/cb_deks.erl
@@ -425,6 +425,8 @@ force_config_encryption_keys() ->
         ok ?= simple_store:resave(?XDCR_CHECKPOINT_STORE),
         ok ?= chronicle_local:maybe_apply_new_keys(),
         ok ?= ns_ssl_services_setup:resave_encrypted_files(),
+        ok ?= encryption_service:remove_old_integrity_tokens(
+                [kek | dek_kinds_list()]),
         ok
     end.
 
@@ -439,10 +441,11 @@ get_config_dek_ids_in_use() ->
         {ok, Ids7} ?= simple_store:get_key_ids_in_use(?XDCR_CHECKPOINT_STORE),
         {ok, Ids8} ?= chronicle_local:get_encryption_dek_ids(),
         {ok, Ids9} ?= ns_ssl_services_setup:get_key_ids_in_use(),
+        {ok, Ids10} ?= encryption_service:get_key_ids_in_use(),
         {ok, lists:map(fun (undefined) -> ?NULL_DEK;
                            (Id) -> Id
                        end, lists:uniq(Ids1 ++ Ids2 ++ Ids3 ++ Ids4 ++ Ids5 ++
-                                       Ids6 ++ Ids7 ++ Ids8 ++ Ids9))}
+                                       Ids6 ++ Ids7 ++ Ids8 ++ Ids9 ++ Ids10))}
     end.
 
 get_dek_ids_in_use(logDek) ->
diff --git a/apps/ns_server/src/encryption_service.erl b/apps/ns_server/src/encryption_service.erl
@@ -38,7 +38,10 @@
          key_path/1,
          decode_key_info/1,
          garbage_collect_keks/1,
-         garbage_collect_keys/2]).
+         garbage_collect_keys/2,
+         maybe_rotate_integrity_tokens/1,
+         remove_old_integrity_tokens/1,
+         get_key_ids_in_use/0]).
 
 
 -export_type([stored_key_error/0]).
@@ -173,6 +176,32 @@ decrypt_key(Data, AD, KekId) when is_binary(Data), is_binary(AD),
       cb_gosecrets_runner:decrypt_with_key(?RUNNER, Data, FinalAD, kek, KekId),
       decrypt_key_error).
 
+maybe_rotate_integrity_tokens(undefined) ->
+    maybe_rotate_integrity_tokens(<<>>);
+maybe_rotate_integrity_tokens(KeyName) ->
+    wrap_error_msg(
+      cb_gosecrets_runner:rotate_integrity_tokens(?RUNNER, KeyName),
+      rotate_integrity_tokens_error).
+
+remove_old_integrity_tokens(Kinds) ->
+    Paths = lists:filtermap(
+              fun(Kind) ->
+                  case key_path(Kind) of
+                      undefined -> false;
+                      Path -> {true, Path}
+                  end
+              end, Kinds),
+    wrap_error_msg(
+      cb_gosecrets_runner:remove_old_integrity_tokens(?RUNNER, Paths),
+      remove_old_integrity_tokens_error).
+
+get_key_ids_in_use() ->
+    case cb_gosecrets_runner:get_key_id_in_use(?RUNNER) of
+        {ok, <<>>} -> {ok, [undefined]};
+        {ok, KeyId} -> {ok, [KeyId]};
+        {error, Error} -> {error, Error}
+    end.
+
 %%%===================================================================
 %%% callbacks
 %%%===================================================================
diff --git a/deps/gocode/go.mod b/deps/gocode/go.mod
@@ -6,6 +6,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/config v1.27.18
 	github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.5
 	github.com/evanw/esbuild v0.13.13
+	github.com/google/uuid v1.6.0
 	gocloud.dev v0.37.0
 	golang.org/x/crypto v0.24.0
 )
diff --git a/deps/gocode/go.sum b/deps/gocode/go.sum
@@ -91,6 +91,8 @@ github.com/google/s2a-go v0.1.7 h1:60BLSyTrOV4/haCDW4zb1guZItoSq8foHCXrAnjBo/o=
 github.com/google/s2a-go v0.1.7/go.mod h1:50CgR4k1jNlWBu4UfS4AcfhVe1r6pdZPygJ3R8F0Qdw=
 github.com/google/subcommands v1.2.0/go.mod h1:ZjhPrFU+Olkh9WazFPsl27BQ4UPiG37m3yTrtFlrHVk=
 github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
+github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/google/wire v0.6.0 h1:HBkoIh4BdSxoyo9PveV8giw7ZsaBOvzWKfcg/6MrVwI=
 github.com/google/wire v0.6.0/go.mod h1:F4QhpQ9EDIdJ1Mbop/NZBRB+5yrR6qg3BnctaoUk6NA=
 github.com/googleapis/enterprise-certificate-proxy v0.3.2 h1:Vie5ybvEvT75RniqhfFxPRy3Bf7vr3h0cechB90XaQs=
diff --git a/deps/gocode/gosecrets/gosecrets.go b/deps/gocode/gosecrets/gosecrets.go
diff --git a/deps/gocode/gosecrets/misc.go b/deps/gocode/gosecrets/misc.go
diff --git a/deps/gocode/gosecrets/stored_keys.go b/deps/gocode/gosecrets/stored_keys.go

Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,7 @@ require (`
`6`	`6`	`github.com/aws/aws-sdk-go-v2/config v1.27.18`
`7`	`7`	`github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.5`
`8`	`8`	`github.com/evanw/esbuild v0.13.13`
	`9`	`+ github.com/google/uuid v1.6.0`
`9`	`10`	`gocloud.dev v0.37.0`
`10`	`11`	`golang.org/x/crypto v0.24.0`
`11`	`12`	`)`