Skip to content

Commit

Permalink
MB-61292: Fix for PUT /../secrets/<ID>
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Well-Formed: Build Bot <[email protected]>
Reviewed-by: Navdeep S Boparai <[email protected]>
  • Loading branch information
@timofey-barmin
timofey-barmin committed Sep 27, 2024
1 parent 96db79e commit c296733
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 47 deletions.
62 changes: 32 additions & 30 deletions apps/ns_server/src/cb_cluster_secrets.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.

Expand Down
51 changes: 34 additions & 17 deletions apps/ns_server/src/menelaus_web_secrets.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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, _),
Expand Down Expand Up @@ -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),
Expand All @@ -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, _),
Expand Down Expand Up @@ -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, _),
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit c296733

Please sign in to comment.