From 2e035dea3905a123143f6f0f79d173d55dbae6bb Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 9 Nov 2024 16:16:35 -0500 Subject: [PATCH 1/4] HTTP API: limit default body size for binding creation It does not need to use the "worst case scenario" default HTTP request body size limit that is primarily necessary because definition imports can be large (MiBs in size, for example). Since exchange, queue names and routing key have limits of 255 bytes and optional arguments can practically be expected to be short, we can lower the limit to < 10 KiB. (cherry picked from commit fb300d2a4b67b1cfeb433e7986e2f87d240f3198) --- .../src/rabbit_mgmt_util.erl | 33 +++++++- .../src/rabbit_mgmt_wm_bindings.erl | 75 +++++++++++-------- .../src/rabbit_mgmt_wm_definitions.erl | 4 +- 3 files changed, 74 insertions(+), 38 deletions(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl index 99a8436e16ea..def9bdaf0a67 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl @@ -30,7 +30,7 @@ list_login_vhosts_names/2]). -export([filter_tracked_conn_list/3]). -export([with_decode/5, decode/1, decode/2, set_resp_header/3, - args/1, read_complete_body/1]). + args/1, read_complete_body/1, read_complete_body_with_limit/2]). -export([reply_list/3, reply_list/5, reply_list/4, sort_list/2, destination_type/1, reply_list_or_paginate/3 ]). @@ -703,15 +703,18 @@ halt_response(Code, Type, Reason, ReqData, Context) -> id(Key, ReqData) -> rabbit_web_dispatch_access_control:id(Key, ReqData). +%% IMPORTANT: +%% Prefer read_complete_body_with_limit/2 with an explicit limit to make it easier +%% to reason about what limit will be used. read_complete_body(Req) -> read_complete_body(Req, <<"">>). read_complete_body(Req, Acc) -> BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE), read_complete_body(Req, Acc, BodySizeLimit). read_complete_body(Req0, Acc, BodySizeLimit) -> - case bit_size(Acc) > BodySizeLimit of + case byte_size(Acc) > BodySizeLimit of true -> - {error, "Exceeded HTTP request body size limit"}; + {error, http_body_limit_exceeded}; false -> case cowboy_req:read_body(Req0) of {ok, Data, Req} -> {ok, <>, Req}; @@ -719,6 +722,30 @@ read_complete_body(Req0, Acc, BodySizeLimit) -> end end. +read_complete_body_with_limit(Req, BodySizeLimit) when is_integer(BodySizeLimit) -> + case cowboy_req:body_length(Req) of + N when is_integer(N) -> + case N > BodySizeLimit of + true -> + {error, http_body_limit_exceeded, BodySizeLimit, N}; + false -> + do_read_complete_body_with_limit(Req, <<"">>, BodySizeLimit) + end; + undefined -> + do_read_complete_body_with_limit(Req, <<"">>, BodySizeLimit) + end. + +do_read_complete_body_with_limit(Req0, Acc, BodySizeLimit) -> + case byte_size(Acc) > BodySizeLimit of + true -> + {error, http_body_limit_exceeded, BodySizeLimit, byte_size(Acc)}; + false -> + case cowboy_req:read_body(Req0, #{length => BodySizeLimit, period => 30000}) of + {ok, Data, Req} -> {ok, <>, Req}; + {more, Data, Req} -> do_read_complete_body_with_limit(Req, <>, BodySizeLimit) + end + end. + with_decode(Keys, ReqData, Context, Fun) -> case read_complete_body(ReqData) of {error, Reason} -> diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl index 299eebb90a0c..234db6eb5d4b 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_bindings.erl @@ -16,6 +16,10 @@ -include_lib("rabbitmq_management_agent/include/rabbit_mgmt_records.hrl"). -include_lib("amqp_client/include/amqp_client.hrl"). +%% Use a much lower limit for creating bindings over the HTTP API. +%% The payload is not meant to be even 50 KiB in size. +-define(HTTP_BODY_SIZE_LIMIT, 5000). + %%-------------------------------------------------------------------- init(Req, [Mode]) -> @@ -64,39 +68,44 @@ to_json(ReqData, {Mode, Context}) -> ReqData, {Mode, Context}). accept_content(ReqData0, {_Mode, Context}) -> - {ok, Body, ReqData} = rabbit_mgmt_util:read_complete_body(ReqData0), - Source = rabbit_mgmt_util:id(source, ReqData), - Dest = rabbit_mgmt_util:id(destination, ReqData), - DestType = rabbit_mgmt_util:id(dtype, ReqData), - VHost = rabbit_mgmt_util:vhost(ReqData), - {ok, Props} = rabbit_mgmt_util:decode(Body), - MethodName = case rabbit_mgmt_util:destination_type(ReqData) of - exchange -> 'exchange.bind'; - queue -> 'queue.bind' - end, - {Key, Args} = key_args(DestType, Props), - case rabbit_mgmt_util:direct_request( - MethodName, - fun rabbit_mgmt_format:format_accept_content/1, - [{queue, Dest}, - {exchange, Source}, - {destination, Dest}, - {source, Source}, - {routing_key, Key}, - {arguments, Args}], - "Binding error: ~ts", ReqData, Context) of - {stop, _, _} = Res -> - Res; - {true, ReqData, Context2} -> - From = binary_to_list(cowboy_req:path(ReqData)), - Prefix = rabbit_mgmt_util:get_path_prefix(), - BindingProps = rabbit_mgmt_format:pack_binding_props(Key, Args), - UrlWithBindings = rabbit_mgmt_format:url("/api/bindings/~ts/e/~ts/~ts/~ts/~ts", - [VHost, Source, DestType, - Dest, BindingProps]), - To = Prefix ++ binary_to_list(UrlWithBindings), - Loc = rabbit_web_dispatch_util:relativise(From, To), - {{true, Loc}, ReqData, Context2} + case rabbit_mgmt_util:read_complete_body_with_limit(ReqData0, ?HTTP_BODY_SIZE_LIMIT) of + {ok, Body, ReqData} -> + Source = rabbit_mgmt_util:id(source, ReqData), + Dest = rabbit_mgmt_util:id(destination, ReqData), + DestType = rabbit_mgmt_util:id(dtype, ReqData), + VHost = rabbit_mgmt_util:vhost(ReqData), + {ok, Props} = rabbit_mgmt_util:decode(Body), + MethodName = case rabbit_mgmt_util:destination_type(ReqData) of + exchange -> 'exchange.bind'; + queue -> 'queue.bind' + end, + {Key, Args} = key_args(DestType, Props), + case rabbit_mgmt_util:direct_request( + MethodName, + fun rabbit_mgmt_format:format_accept_content/1, + [{queue, Dest}, + {exchange, Source}, + {destination, Dest}, + {source, Source}, + {routing_key, Key}, + {arguments, Args}], + "Binding error: ~ts", ReqData, Context) of + {stop, _, _} = Res -> + Res; + {true, ReqData, Context2} -> + From = binary_to_list(cowboy_req:path(ReqData)), + Prefix = rabbit_mgmt_util:get_path_prefix(), + BindingProps = rabbit_mgmt_format:pack_binding_props(Key, Args), + UrlWithBindings = rabbit_mgmt_format:url("/api/bindings/~ts/e/~ts/~ts/~ts/~ts", + [VHost, Source, DestType, + Dest, BindingProps]), + To = Prefix ++ binary_to_list(UrlWithBindings), + Loc = rabbit_web_dispatch_util:relativise(From, To), + {{true, Loc}, ReqData, Context2} + end; + {error, http_body_limit_exceeded, LimitApplied, BytesRead} -> + rabbit_log:warning("HTTP API: binding creation request exceeded maximum allowed payload size (limit: ~tp bytes, payload size: ~tp bytes)", [LimitApplied, BytesRead]), + rabbit_mgmt_util:bad_request("Payload size limit exceeded", ReqData0, Context) end. is_authorized(ReqData, {Mode, Context}) -> diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index 335081c7ad55..c0ad53826194 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -84,9 +84,9 @@ all_definitions(ReqData, Context) -> Context). accept_json(ReqData0, Context) -> - case rabbit_mgmt_util:read_complete_body(ReqData0) of + BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE), + case rabbit_mgmt_util:read_complete_body_with_limit(ReqData0, BodySizeLimit) of {error, Reason} -> - BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE), _ = rabbit_log:warning("HTTP API: uploaded definition file exceeded the maximum request body limit of ~p bytes. " "Use the 'management.http.max_body_size' key in rabbitmq.conf to increase the limit if necessary", [BodySizeLimit]), rabbit_mgmt_util:bad_request(Reason, ReqData0, Context); From afd4209c50d76668bd7557920283af33df9b31e4 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 9 Nov 2024 16:38:48 -0500 Subject: [PATCH 2/4] rabbit_mgmt_util: minor refactoring (cherry picked from commit b0abf88aa810c82915abd42a0ddd86ea549dfa0e) --- deps/rabbitmq_management/src/rabbit_mgmt_util.erl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl index def9bdaf0a67..62b3aa4be508 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl @@ -712,9 +712,10 @@ read_complete_body(Req, Acc) -> BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE), read_complete_body(Req, Acc, BodySizeLimit). read_complete_body(Req0, Acc, BodySizeLimit) -> - case byte_size(Acc) > BodySizeLimit of + N = byte_size(Acc), + case N > BodySizeLimit of true -> - {error, http_body_limit_exceeded}; + {error, http_body_limit_exceeded, BodySizeLimit, N}; false -> case cowboy_req:read_body(Req0) of {ok, Data, Req} -> {ok, <>, Req}; @@ -736,9 +737,10 @@ read_complete_body_with_limit(Req, BodySizeLimit) when is_integer(BodySizeLimit) end. do_read_complete_body_with_limit(Req0, Acc, BodySizeLimit) -> - case byte_size(Acc) > BodySizeLimit of + N = byte_size(Acc), + case N > BodySizeLimit of true -> - {error, http_body_limit_exceeded, BodySizeLimit, byte_size(Acc)}; + {error, http_body_limit_exceeded, BodySizeLimit, N}; false -> case cowboy_req:read_body(Req0, #{length => BodySizeLimit, period => 30000}) of {ok, Data, Req} -> {ok, <>, Req}; From 40c0a741bd98bf0d829c7c01bf6c2ddff0b9fc81 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 9 Nov 2024 16:53:45 -0500 Subject: [PATCH 3/4] Pass Dialyzer (cherry picked from commit 3dc5c463a4d58c22ff9730dda0e7e7f2bd7ee6f0) --- deps/rabbitmq_amqp1_0/BUILD.bazel | 1 + deps/rabbitmq_ct_client_helpers/BUILD.bazel | 1 + deps/rabbitmq_ct_helpers/BUILD.bazel | 1 + deps/rabbitmq_management/src/rabbit_mgmt_util.erl | 5 +++-- .../src/rabbit_mgmt_wm_definitions.erl | 8 ++++---- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/deps/rabbitmq_amqp1_0/BUILD.bazel b/deps/rabbitmq_amqp1_0/BUILD.bazel index 3c5a1d767c07..8880ca385337 100644 --- a/deps/rabbitmq_amqp1_0/BUILD.bazel +++ b/deps/rabbitmq_amqp1_0/BUILD.bazel @@ -30,6 +30,7 @@ rabbitmq_app( app_description = APP_DESCRIPTION, app_name = APP_NAME, beam_files = [":beam_files"], + extra_apps = ["rabbit"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_ct_client_helpers/BUILD.bazel b/deps/rabbitmq_ct_client_helpers/BUILD.bazel index 8fa9dfa34f41..1141dd990501 100644 --- a/deps/rabbitmq_ct_client_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_client_helpers/BUILD.bazel @@ -33,6 +33,7 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_client_helpers", beam_files = [":beam_files"], + extra_apps = ["rabbit_common"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_ct_helpers/BUILD.bazel b/deps/rabbitmq_ct_helpers/BUILD.bazel index 1002b4289a8a..c4319137c279 100644 --- a/deps/rabbitmq_ct_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_helpers/BUILD.bazel @@ -39,6 +39,7 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_helpers", beam_files = [":beam_files"], + extra_apps = ["inet_tcp_proxy"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl index 62b3aa4be508..68ef793c1cba 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_util.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_util.erl @@ -750,8 +750,9 @@ do_read_complete_body_with_limit(Req0, Acc, BodySizeLimit) -> with_decode(Keys, ReqData, Context, Fun) -> case read_complete_body(ReqData) of - {error, Reason} -> - bad_request(Reason, ReqData, Context); + {error, http_body_limit_exceeded, LimitApplied, BytesRead} -> + rabbit_log:warning("HTTP API: request exceeded maximum allowed payload size (limit: ~tp bytes, payload size: ~tp bytes)", [LimitApplied, BytesRead]), + bad_request("Exceeded HTTP request body size limit", ReqData, Context); {ok, Body, ReqData1} -> with_decode(Keys, Body, ReqData1, Context, Fun) end. diff --git a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl index c0ad53826194..3790ca97b90c 100644 --- a/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl +++ b/deps/rabbitmq_management/src/rabbit_mgmt_wm_definitions.erl @@ -86,10 +86,10 @@ all_definitions(ReqData, Context) -> accept_json(ReqData0, Context) -> BodySizeLimit = application:get_env(rabbitmq_management, max_http_body_size, ?MANAGEMENT_DEFAULT_HTTP_MAX_BODY_SIZE), case rabbit_mgmt_util:read_complete_body_with_limit(ReqData0, BodySizeLimit) of - {error, Reason} -> - _ = rabbit_log:warning("HTTP API: uploaded definition file exceeded the maximum request body limit of ~p bytes. " - "Use the 'management.http.max_body_size' key in rabbitmq.conf to increase the limit if necessary", [BodySizeLimit]), - rabbit_mgmt_util:bad_request(Reason, ReqData0, Context); + {error, http_body_limit_exceeded, LimitApplied, BytesRead} -> + _ = rabbit_log:warning("HTTP API: uploaded definition file size (~tp) exceeded the maximum request body limit of ~tp bytes. " + "Use the 'management.http.max_body_size' key in rabbitmq.conf to increase the limit if necessary", [BytesRead, LimitApplied]), + rabbit_mgmt_util:bad_request("Exceeded HTTP request body size limit", ReqData0, Context); {ok, Body, ReqData} -> accept(Body, ReqData, Context) end. From a66c9269859d8243ceb7f3d6455eb9069aa42808 Mon Sep 17 00:00:00 2001 From: Michael Klishin Date: Sat, 9 Nov 2024 17:46:41 -0500 Subject: [PATCH 4/4] Undo the Bazel-related change from #12696 --- deps/rabbitmq_amqp1_0/BUILD.bazel | 1 - deps/rabbitmq_ct_client_helpers/BUILD.bazel | 1 - deps/rabbitmq_ct_helpers/BUILD.bazel | 1 - 3 files changed, 3 deletions(-) diff --git a/deps/rabbitmq_amqp1_0/BUILD.bazel b/deps/rabbitmq_amqp1_0/BUILD.bazel index 8880ca385337..3c5a1d767c07 100644 --- a/deps/rabbitmq_amqp1_0/BUILD.bazel +++ b/deps/rabbitmq_amqp1_0/BUILD.bazel @@ -30,7 +30,6 @@ rabbitmq_app( app_description = APP_DESCRIPTION, app_name = APP_NAME, beam_files = [":beam_files"], - extra_apps = ["rabbit"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_ct_client_helpers/BUILD.bazel b/deps/rabbitmq_ct_client_helpers/BUILD.bazel index 1141dd990501..8fa9dfa34f41 100644 --- a/deps/rabbitmq_ct_client_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_client_helpers/BUILD.bazel @@ -33,7 +33,6 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_client_helpers", beam_files = [":beam_files"], - extra_apps = ["rabbit_common"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_ct_helpers/BUILD.bazel b/deps/rabbitmq_ct_helpers/BUILD.bazel index c4319137c279..1002b4289a8a 100644 --- a/deps/rabbitmq_ct_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_helpers/BUILD.bazel @@ -39,7 +39,6 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_helpers", beam_files = [":beam_files"], - extra_apps = ["inet_tcp_proxy"], license_files = [":license_files"], priv = [":priv"], deps = [