MB-61292: Fix for PUT /../secrets/<ID>

timofey-barmin · timofey-barmin · commit c296733925be · 2024-09-27T01:29:13.000Z
Fix permissions check: should check not only usages that are being set but also usages that are being replaced. Without this check, for example, bucket admin can overwrite a secret created by full admin that was supposed to be used for things like config encryption. Also this change fixes a race scenario when two parallel changes can hypothetically overwrite some settings of the secret being modified: 1. PUT takes current secret properties and prepares new properties based on that value; 2. Another process modifies some secret properties (auto-rotation); 3. PUT finishes and sets the properties prepared at step #1 4. Change made by step #2 is lost This obvious race was considered imposible in the very first implementation, but then after several changes it became possible:( Change-Id: I3c508e9eb8d8b367bc63bb8aaadfc050c4204160 Reviewed-on: https://review.couchbase.org/c/ns_server/+/216863 Tested-by: Timofey Barmin <timofey.barmin@couchbase.com> Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Navdeep S Boparai <navdeep.boparai@couchbase.com>
diff --git a/apps/ns_server/src/cb_cluster_secrets.erl b/apps/ns_server/src/cb_cluster_secrets.erl
@@ -43,7 +43,7 @@
 -export([start_link_node_monitor/0,
          start_link_master_monitor/0,
          add_new_secret/1,
-         replace_secret/2,
+         replace_secret/3,
          delete_secret/1,
          get_all/0,
          get_secret/1,
@@ -67,7 +67,7 @@
 
 %% Can be called by other nodes:
 -export([add_new_secret_internal/1,
-         replace_secret_internal/2,
+         replace_secret_internal/3,
          rotate_internal/1,
          sync_with_node_monitor/0,
          delete_secret_internal/1,
@@ -218,44 +218,46 @@ add_new_secret_internal(Props) ->
         {error, _} = Error -> Error
     end.
 
--spec replace_secret(secret_props(), map()) ->
-                                    {ok, secret_props()} |
-                                    {error, not_found | bad_encrypt_id() |
-                                            bad_usage_change()}.
-replace_secret(OldProps, NewProps) ->
-    execute_on_master({?MODULE, replace_secret_internal, [OldProps, NewProps]}).
-
--spec replace_secret_internal(secret_props(), map()) ->
-                                    {ok, secret_props()} |
-                                    {error, not_found | bad_encrypt_id() |
-                                            bad_usage_change()}.
-replace_secret_internal(OldProps, NewProps) ->
+-spec replace_secret(secret_id(), map(), fun((secret_props()) -> boolean())) ->
+          {ok, secret_props()} |
+          {error, not_found | bad_encrypt_id() | bad_usage_change() |
+                  forbidden}.
+replace_secret(Id, NewProps, IsSecretWritableFun) ->
+    execute_on_master({?MODULE, replace_secret_internal,
+                       [Id, NewProps, IsSecretWritableFun]}).
+
+-spec replace_secret_internal(secret_id(), map(),
+                              fun((secret_props()) -> boolean())) ->
+          {ok, secret_props()} |
+          {error, not_found | bad_encrypt_id() | bad_usage_change() |
+                  forbidden}.
+replace_secret_internal(Id, NewProps, IsSecretWritableFun) ->
     %% Make sure we have most recent information about which secrets are in use
     %% This is needed for verification of 'usage' modification
     maybe_reset_deks_counters(),
-    Props = copy_static_props(OldProps, NewProps),
     Res =
         chronicle_compat:txn(
           fun (Txn) ->
-              Snapshot = fetch_snapshot_in_txn(Txn),
-              CurList = get_all(Snapshot),
-              case replace_secret_in_list(Props, CurList) of
-                  false -> %% already removed, we should not create new
-                      {abort, {error, not_found}};
-                  NewList ->
-                      case validate_secret_in_txn(Props, OldProps, Snapshot) of
-                          ok ->
-                              {commit,
-                               [{set, ?CHRONICLE_SECRETS_KEY, NewList}]};
-                          {error, _} = Error ->
-                              {abort, Error}
-                      end
+              maybe
+                  Snapshot = fetch_snapshot_in_txn(Txn),
+                  {ok, OldProps} ?= get_secret(Id, Snapshot),
+                  true ?= IsSecretWritableFun(OldProps),
+                  Props = copy_static_props(OldProps, NewProps),
+                  CurList = get_all(Snapshot),
+                  NewList = replace_secret_in_list(Props, CurList),
+                  ok ?= validate_secret_in_txn(Props, OldProps, Snapshot),
+                  {commit, [{set, ?CHRONICLE_SECRETS_KEY, NewList}], Props}
+              else
+                  false ->
+                      {abort, {error, forbidden}};
+                  {error, _} = Err ->
+                      {abort, Err}
               end
            end),
     case Res of
-        {ok, _} ->
+        {ok, _, ResProps} ->
             sync_with_all_node_monitors(),
-            {ok, Props};
+            {ok, ResProps};
         {error, _} = Error -> Error
     end.
 
diff --git a/apps/ns_server/src/menelaus_web_secrets.erl b/apps/ns_server/src/menelaus_web_secrets.erl
@@ -53,12 +53,12 @@ handle_post_secret(Req) ->
       fun (RawProps) ->
           maybe
               ToAdd = import_secret(RawProps),
-              [_] ?= write_filter_secrets_by_permission([ToAdd], Req),
+              true ?= is_writable(ToAdd, Req),
               {ok, Res} ?= cb_cluster_secrets:add_new_secret(ToAdd),
               Formatted = export_secret(Res),
               menelaus_util:reply_json(Req, {Formatted})
           else
-              [] ->
+              false ->
                   menelaus_util:web_exception(403, "Forbidden");
               {error, {encrypt_id, not_found}} ->
                   menelaus_util:reply_global_error(
@@ -73,19 +73,27 @@ handle_post_secret(Req) ->
       end, Req, json, secret_validators(#{})).
 
 handle_put_secret(IdStr, Req) ->
-    case cb_cluster_secrets:get_secret(parse_id(IdStr)) of
+    Id = parse_id(IdStr),
+    case cb_cluster_secrets:get_secret(Id) of
         {ok, CurProps} ->
             validator:handle(
               fun (RawProps) ->
                   maybe
                       Props = import_secret(RawProps),
-                      [_] ?= write_filter_secrets_by_permission([Props], Req),
-                      {ok, Res} ?= cb_cluster_secrets:replace_secret(CurProps,
-                                                                     Props),
+                      %% Note: All "usages" should be writable by current user.
+                      %% This includes "new usages" (usages that are being set)
+                      %% and "old usages" (usages that are being replaced)
+                      %% Checking "new usages" here:
+                      true ?= is_writable(Props, Req),
+                      %% replace_secret will check "old usages" inside txn
+                      {ok, Res} ?= cb_cluster_secrets:replace_secret(
+                                     Id, Props, is_writable(_, Req)),
                       Formatted = export_secret(Res),
                       menelaus_util:reply_json(Req, {Formatted})
                   else
-                      [] ->
+                      false ->
+                          menelaus_util:web_exception(403, "Forbidden");
+                      {error, forbidden} ->
                           menelaus_util:web_exception(403, "Forbidden");
                       {error, {usage, in_use}} ->
                           menelaus_util:reply_global_error(
@@ -109,6 +117,9 @@ handle_put_secret(IdStr, Req) ->
             menelaus_util:reply_not_found(Req)
     end.
 
+%% Note: CurProps can only be used for static fields validation here.
+%% Any field that can be modified and needs to use CurProps should be
+%% checked in transaction in cb_cluster_secret:replace_secret_internal.
 secret_validators(CurProps) ->
     [validator:string(name, _),
      validator:required(name, _),
@@ -272,6 +283,9 @@ validate_key_usage(Name, State) ->
           end
       end, false, State).
 
+%% Note: CurSecretProps can only be used for static fields validation here.
+%% Any field that can be modified and needs to use CurProps should be
+%% checked in transaction in cb_cluster_secret:replace_secret_internal.
 validate_secrets_data(Name, CurSecretProps, State) ->
     Type = validator:get_value(type, State),
     CurType = maps:get(type, CurSecretProps, Type),
@@ -295,6 +309,9 @@ validate_secrets_data(Name, CurSecretProps, State) ->
             enforce_static_field_validator(type, CurType, State)
     end.
 
+%% Note: CurSecretProps can only be used for static fields validation here.
+%% Any field that can be modified and needs to use CurProps should be
+%% checked in transaction in cb_cluster_secret:replace_secret_internal.
 generated_key_validators(CurSecretProps) ->
     [validator:boolean(autoRotation, _),
      validator:default(autoRotation, true, _),
@@ -339,6 +356,9 @@ validate_encrypt_secret_id(Name, CurSecretProps, State) ->
               ok
       end, Name, encryptBy, State).
 
+%% Note: CurSecretProps can only be used for static fields validation here.
+%% Any field that can be modified and needs to use CurProps should be
+%% checked in transaction in cb_cluster_secret:replace_secret_internal.
 awskms_key_validators(CurSecretProps) ->
     [validator:string(keyARN, _),
      validator:required(keyARN, _),
@@ -423,16 +443,13 @@ is_usage_allowed(config_encryption, read, Req) ->
     menelaus_auth:has_permission({[admin, security], read}, Req).
 
 read_filter_secrets_by_permission(Secrets, Req) ->
-    lists:filter(
-      fun (#{usage := List}) when List /= [] ->
-          lists:any(is_usage_allowed(_, read, Req), List)
-      end, Secrets).
-
-write_filter_secrets_by_permission(Secrets, Req) ->
-    lists:filter(
-      fun (#{usage := List}) when List /= [] ->
-          lists:all(is_usage_allowed(_, write, Req), List)
-      end, Secrets).
+    lists:filter(is_readable(_, Req), Secrets).
+
+is_readable(#{usage := Usages}, Req) when Usages /= [] ->
+    lists:any(is_usage_allowed(_, read, Req), Usages).
+
+is_writable(#{usage := Usages}, Req) when Usages /= [] ->
+    lists:all(is_usage_allowed(_, write, Req), Usages).
 
 parse_id(Str) when is_list(Str) ->
     try list_to_integer(Str) of
