From 64c15d35e6ac65b7443bf89d50e145552a0ee607 Mon Sep 17 00:00:00 2001 From: Luke Bakken Date: Mon, 10 Nov 2025 17:00:06 -0800 Subject: [PATCH] When vhost is known, limit auth to that vhost This reduces the number of extraneous auth queries to vhosts that aren't relevant. These changes also provide friendlier formatting in `log_access_control_result` Fixes #14923 (cherry picked from commit e8d9218325ff73429a754539f6946bbfeed1768b) --- .../src/rabbit_mgmt_util.erl | 28 ++++++++-- .../test/rabbit_mgmt_http_SUITE.erl | 28 +++++++--- .../rabbit_web_dispatch_access_control.erl | 56 ++++++------------- 3 files changed, 61 insertions(+), 51 deletions(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl index 54fef24144d5..fbda5edc48a3 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl @@ -313,7 +313,6 @@ get_sort_reverse(ReqData) -> V -> list_to_atom(V) end. - -spec is_pagination_requested(#pagination{} | undefined) -> boolean(). is_pagination_requested(undefined) -> false; @@ -981,9 +980,28 @@ all_or_one_vhost(ReqData, Fun) -> VHost -> Fun(VHost) end. -filter_vhost(List, ReqData, Context) -> - User = #user{tags = Tags} = Context#context.user, - Fn = case rabbit_web_dispatch_access_control:is_admin(Tags) of +filter_vhost(List, ReqData, Context = #context{user = #user{tags = Tags}}) -> + IsAdmin = rabbit_web_dispatch_access_control:is_admin(Tags), + filter_vhost({vhost, vhost(ReqData)}, IsAdmin, List, ReqData, Context). + +filter_vhost({vhost, not_found}, IsAdmin, List, ReqData, Context) -> + filter_none_or_not_found_vhost(IsAdmin, List, ReqData, Context); +filter_vhost({vhost, none}, IsAdmin, List, ReqData, Context) -> + filter_none_or_not_found_vhost(IsAdmin, List, ReqData, Context); +filter_vhost({vhost, _VHost}, _IsAdmin=true, List, _ReqData, _Context) -> + [I || I <- List, lists:member(pget(vhost, I), rabbit_vhost:list())]; +filter_vhost({vhost, VHost}, _IsAdmin=false, List, ReqData, #context{user = User}) -> + AuthzData = get_authz_data(ReqData), + case catch rabbit_access_control:check_vhost_access(User, VHost, AuthzData, #{}) of + ok -> + [I || I <- List, pget(vhost, I) =:= VHost]; + NotOK -> + log_access_control_result(NotOK), + [] + end. + +filter_none_or_not_found_vhost(IsAdmin, List, ReqData, #context{user = User}) -> + Fn = case IsAdmin of true -> fun list_visible_vhosts_names/2; false -> fun list_login_vhosts_names/2 end, @@ -1090,6 +1108,8 @@ list_login_vhosts(User, AuthzData) -> end]. % rabbitmq/rabbitmq-auth-backend-http#100 +log_access_control_result({'EXIT', #amqp_error{name = Name, explanation = Msg}}) -> + ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: '~tp', ~ts", [Name, Msg]); log_access_control_result(NotOK) -> ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: ~tp", [NotOK]). diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl index b253e38044b8..af720ee4f783 100644 --- a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl +++ b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl @@ -1709,9 +1709,12 @@ permissions_vhost_test(Config) -> {tags, <<"administrator">>}], {group, '2xx'}), http_put(Config, "/users/myuser", [{password, <<"myuser">>}, {tags, <<"management">>}], {group, '2xx'}), + http_put(Config, "/users/mymonitor", [{password, <<"mymonitor">>}, + {tags, [<<"monitor">>, <<"management">>]}], {group, '2xx'}), http_put(Config, "/vhosts/myvhost1", none, {group, '2xx'}), http_put(Config, "/vhosts/myvhost2", none, {group, '2xx'}), http_put(Config, "/permissions/myvhost1/myuser", PermArgs, {group, '2xx'}), + http_put(Config, "/permissions/myvhost1/mymonitor", PermArgs, {group, '2xx'}), http_put(Config, "/permissions/myvhost1/guest", PermArgs, {group, '2xx'}), http_put(Config, "/permissions/myvhost2/guest", PermArgs, {group, '2xx'}), assert_list([#{name => <<"/">>}, @@ -1719,23 +1722,34 @@ permissions_vhost_test(Config) -> #{name => <<"myvhost2">>}], http_get(Config, "/vhosts", ?OK)), assert_list([#{name => <<"myvhost1">>}], http_get(Config, "/vhosts", "myuser", "myuser", ?OK)), + assert_list([#{name => <<"myvhost1">>}], + http_get(Config, "/vhosts", "mymonitor", "mymonitor", ?OK)), http_put(Config, "/queues/myvhost1/myqueue", QArgs, {group, '2xx'}), http_put(Config, "/queues/myvhost2/myqueue", QArgs, {group, '2xx'}), + CheckResult = fun (Path, Result) -> + case maps:get(vhost, Result) of + <<"myvhost2">> -> + throw({got_result_from_vhost2_in, Path, Result}); + _ -> + ok + end + end, Test1 = fun(Path) -> - Results = http_get(Config, Path, "myuser", "myuser", ?OK), - [case maps:get(vhost, Result) of - <<"myvhost2">> -> - throw({got_result_from_vhost2_in, Path, Result}); - _ -> - ok - end || Result <- Results] + Results0 = http_get(Config, Path, "myuser", "myuser", ?OK), + [CheckResult(Path, Result0) || Result0 <- Results0], + Results1 = http_get(Config, Path, "mymonitor", "mymonitor", ?OK), + [CheckResult(Path, Result1) || Result1 <- Results1] end, Test2 = fun(Path1, Path2) -> http_get(Config, Path1 ++ "/myvhost1/" ++ Path2, "myuser", "myuser", ?OK), http_get(Config, Path1 ++ "/myvhost2/" ++ Path2, "myuser", "myuser", + ?NOT_AUTHORISED), + http_get(Config, Path1 ++ "/myvhost1/" ++ Path2, "mymonitor", "mymonitor", + ?OK), + http_get(Config, Path1 ++ "/myvhost2/" ++ Path2, "mymonitor", "mymonitor", ?NOT_AUTHORISED) end, Test3 = 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 82ea479856ca..b79247abfd1d 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 @@ -20,7 +20,6 @@ is_authorized_vhost_visible_for_monitoring/3, is_authorized_global_parameters/3]). --export([list_visible_vhosts/1, list_visible_vhosts_names/1, list_login_vhosts/2]). -export([id/2]). -export([not_authorised/3, halt_response/5]). @@ -255,23 +254,34 @@ user_matches_vhost(ReqData, User) -> not_found -> true; none -> true; V -> - AuthzData = get_authz_data(ReqData), - lists:member(V, list_login_vhosts_names(User, AuthzData)) + check_vhost_access(ReqData, User, V) end. -user_matches_vhost_visible(ReqData, User) -> +user_matches_vhost_visible(ReqData, User = #user{tags = Tags}) -> case vhost(ReqData) of not_found -> true; none -> true; V -> - AuthzData = get_authz_data(ReqData), - lists:member(V, list_visible_vhosts_names(User, AuthzData)) + case is_monitor(Tags) of + true -> + rabbit_vhost:exists(V); + false -> + check_vhost_access(ReqData, User, V) + end end. get_authz_data(ReqData) -> {PeerAddress, _PeerPort} = cowboy_req:peer(ReqData), {ip, PeerAddress}. +check_vhost_access(ReqData, User, VHost) -> + AuthzData = get_authz_data(ReqData), + case catch rabbit_access_control:check_vhost_access(User, VHost, AuthzData, #{}) of + ok -> true; + NotOK -> + log_access_control_result(NotOK), + false + end. not_authorised(Reason, ReqData, Context) -> %% TODO: consider changing to 403 in 4.0 @@ -328,40 +338,6 @@ id0(Key, ReqData) -> Id -> Id end. - -list_visible_vhosts_names(User) -> - list_visible_vhosts(User, undefined). - -list_visible_vhosts_names(User, AuthzData) -> - list_visible_vhosts(User, AuthzData). - -list_visible_vhosts(User) -> - list_visible_vhosts(User, undefined). - -list_visible_vhosts(User = #user{tags = Tags}, AuthzData) -> - case is_monitor(Tags) of - true -> rabbit_vhost:list_names(); - false -> list_login_vhosts_names(User, AuthzData) - end. - -list_login_vhosts_names(User, AuthzData) -> - [V || V <- rabbit_vhost:list_names(), - case catch rabbit_access_control:check_vhost_access(User, V, AuthzData, #{}) of - ok -> true; - NotOK -> - log_access_control_result(NotOK), - false - end]. - -list_login_vhosts(User, AuthzData) -> - [V || V <- rabbit_vhost:all(), - case catch rabbit_access_control:check_vhost_access(User, vhost:get_name(V), AuthzData, #{}) of - ok -> true; - NotOK -> - log_access_control_result(NotOK), - false - end]. - % rabbitmq/rabbitmq-auth-backend-http#100 log_access_control_result(NotOK) -> ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: ~tp", [NotOK]).