From ef7e772013b36ff05ab1fc39f0254ce47029a1fc Mon Sep 17 00:00:00 2001 From: Sunny Katkuri Date: Mon, 28 Jul 2025 06:13:01 +0000 Subject: [PATCH 1/2] Check for protected tag during user update and deletion via management API --- .../src/rabbit_mgmt_util.erl | 10 +++++++ .../src/rabbit_mgmt_wm_user.erl | 30 ++++++++++++++----- .../test/rabbit_mgmt_http_SUITE.erl | 16 ++++++++++ .../rabbit_web_dispatch_access_control.erl | 3 +- 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl index 3cead5b415..54fef24144 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl @@ -53,6 +53,8 @@ -export([set_resp_not_found/2]). +-export([is_protected_user/1]). + -import(rabbit_misc, [pget/2]). -include("rabbit_mgmt.hrl"). @@ -208,6 +210,14 @@ is_authorized_user(ReqData, Context, Username, Password, ReplyWhenFailed) -> ReplyWhenFailed, auth_config()). +is_protected_user(Username) -> + case rabbit_auth_backend_internal:lookup_user(Username) of + {ok, User} -> + Tags = internal_user:get_tags(User), + rabbit_web_dispatch_access_control:is_protected_user(Tags); + {error, _} -> false + end. + vhost_from_headers(ReqData) -> rabbit_web_dispatch_access_control:vhost_from_headers(ReqData). diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_user.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_user.erl index d3691c8de3..e66f20513f 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_user.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_user.erl @@ -46,17 +46,31 @@ to_json(ReqData, Context) -> accept_content(ReqData0, Context = #context{user = #user{username = ActingUser}}) -> Username = rabbit_mgmt_util:id(user, ReqData0), - rabbit_mgmt_util:with_decode( - [], ReqData0, Context, - fun(_, User, ReqData) -> - _ = put_user(User#{name => Username}, ActingUser), - {true, ReqData, Context} - end). + case rabbit_mgmt_util:is_protected_user(Username) of + true -> + rabbit_mgmt_util:bad_request( + <<"User updates via API are disabled for this user">>, + ReqData0, Context); + false -> + rabbit_mgmt_util:with_decode( + [], ReqData0, Context, + fun(_, User, ReqData) -> + _ = put_user(User#{name => Username}, ActingUser), + {true, ReqData, Context} + end) + end. delete_resource(ReqData, Context = #context{user = #user{username = ActingUser}}) -> User = rabbit_mgmt_util:id(user, ReqData), - rabbit_auth_backend_internal:delete_user(User, ActingUser), - {true, ReqData, Context}. + case rabbit_mgmt_util:is_protected_user(User) of + true -> + rabbit_mgmt_util:bad_request( + <<"User deletion via API is disabled for this user">>, + ReqData, Context); + false -> + rabbit_auth_backend_internal:delete_user(User, ActingUser), + {true, ReqData, Context} + end. is_authorized(ReqData, Context) -> rabbit_mgmt_util:is_authorized_admin(ReqData, Context). diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index 7ad2d476ef..6f12cac9d2 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -77,6 +77,7 @@ groups() -> some_tests() -> [ users_test, + users_protected_test, exchanges_test, queues_test, bindings_test, @@ -657,6 +658,21 @@ users_test(Config) -> http_get(Config, "/users/users_test", ?NOT_FOUND), passed. +users_protected_test(Config) -> + ProtectedUser = <<"protected_user">>, + rabbit_ct_broker_helpers:add_user(Config, ProtectedUser), + rabbit_ct_broker_helpers:set_user_tags(Config, 0, ProtectedUser, [management, protected]), + + %% Verify protected user cannot be updated via API + http_put(Config, "/users/protected_user", [{password, <<"new_password">>}, + {tags, <<"management,protected">>}], ?BAD_REQUEST), + + %% Verify protected user cannot be deleted via API + http_delete(Config, "/users/protected_user", ?BAD_REQUEST), + + rabbit_ct_broker_helpers:delete_user(Config, ProtectedUser), + passed. + without_permissions_users_test(Config) -> assert_item(#{name => <<"guest">>, tags => [<<"administrator">>]}, http_get(Config, "/whoami")), diff --git a/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl b/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl index 3069a91604..9209241c61 100644 --- a/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl +++ b/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl @@ -24,7 +24,7 @@ -export([id/2]). -export([not_authorised/3, halt_response/5]). --export([is_admin/1, is_policymaker/1, is_monitor/1, is_mgmt_user/1]). +-export([is_admin/1, is_policymaker/1, is_monitor/1, is_mgmt_user/1, is_protected_user/1]). -import(rabbit_misc, [pget/2]). @@ -243,6 +243,7 @@ is_policymaker(T) -> intersects(T, [administrator, policymaker]). is_monitor(T) -> intersects(T, [administrator, monitoring]). is_mgmt_user(T) -> intersects(T, [administrator, monitoring, policymaker, management]). +is_protected_user(T) -> intersects(T, [protected]). intersects(A, B) -> lists:any(fun(I) -> lists:member(I, B) end, A). From 34f00e036283d67d7b07b7fca94974ee5623efd3 Mon Sep 17 00:00:00 2001 From: Luke Bakken Date: Mon, 15 Sep 2025 11:30:16 -0700 Subject: [PATCH 2/2] Add function head to match single-element list containing for which to search. --- .../src/rabbit_web_dispatch_access_control.erl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl b/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl index 9209241c61..82ea479856 100644 --- a/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl +++ b/deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl @@ -245,7 +245,10 @@ is_mgmt_user(T) -> intersects(T, [administrator, monitoring, policymaker, management]). is_protected_user(T) -> intersects(T, [protected]). -intersects(A, B) -> lists:any(fun(I) -> lists:member(I, B) end, A). +intersects(A, [B]) -> + lists:member(B, A); +intersects(A, B) -> + lists:any(fun(I) -> lists:member(I, B) end, A). user_matches_vhost(ReqData, User) -> case vhost(ReqData) of