Skip to content

Commit

Permalink
Correctly use AMQP URI query parameter password
Browse files Browse the repository at this point in the history
Fixes #8129

The query parameter `password` in an AMQP URI should only be used to set
a certificate password, *not* the login password. The login password is
set via the `amqp_authority` section as defined here -

https://www.rabbitmq.com/uri-spec.html

* Add test that demonstrates issue in #8129
* Modify code to fix test

Modify amqp_uri so that test passes
  • Loading branch information
lukebakken authored and dcorbacho committed May 11, 2023
1 parent fef099f commit ccb232c
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 11 deletions.
20 changes: 13 additions & 7 deletions deps/amqp_client/src/amqp_uri.erl
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,15 @@ broker_add_query(Params, ParsedUri, Fields) ->
return({ParamsN, Pos1});
Value ->
try
ValueParsed = parse_amqp_param(Field, Value),
return(
{setelement(Pos, ParamsN, ValueParsed), Pos1})
case parse_amqp_param(Field, Value) of
ignore ->
return({ParamsN, Pos1});
ValueParsed ->
return({setelement(Pos, ParamsN, ValueParsed), Pos1})
end
catch throw:Reason ->
fail({invalid_amqp_params_parameter,
Field, Value, Query, Reason})
fail({invalid_amqp_params_parameter,
Field, Value, Query, Reason})
end
end
end || Field <- Fields], {Params, 2}),
Expand All @@ -221,8 +224,11 @@ parse_amqp_param(Field, String) when Field =:= channel_max orelse
Field =:= connection_timeout orelse
Field =:= depth ->
find_integer_parameter(String);
parse_amqp_param(Field, String) when Field =:= password ->
find_identity_parameter(String);
parse_amqp_param(Field, _String) when Field =:= password ->
%% https://github.com/rabbitmq/rabbitmq-server/issues/8129
%% Ignore `password` here since the parameter is used for setting a
%% certificate password, NOT an AMQP login password
return(ignore);
parse_amqp_param(Field, String) ->
fail({parameter_unconfigurable_in_query, Field, String}).

Expand Down
10 changes: 6 additions & 4 deletions deps/amqp_client/test/unit_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,9 @@ amqp_uri_parsing(_Config) ->
{server_name_indication,"host3"}],
?assertEqual(lists:usort(Exp3), lists:usort(TLSOpts3)),

{ok, #amqp_params_network{host = "host4", ssl_options = TLSOpts4}} =
amqp_uri:parse("amqps://host4/%2f?cacertfile=/path/to/cacertfile.pem"
{ok, #amqp_params_network{username = <<"user">>, password = <<"pass">>,
host = "host4", ssl_options = TLSOpts4}} =
amqp_uri:parse("amqps://user:pass@host4/%2f?cacertfile=/path/to/cacertfile.pem"
"&certfile=/path/to/certfile.pem"
"&password=topsecret"
"&depth=5"),
Expand All @@ -171,8 +172,9 @@ amqp_uri_parsing(_Config) ->
{verify, verify_none}]),
lists:usort(TLSOpts8)),

{ok, #amqp_params_network{host = "127.0.0.1", ssl_options = TLSOpts9}} =
amqp_uri:parse("amqps://127.0.0.1/%2f?cacertfile=/path/to/cacertfile.pem"
{ok, #amqp_params_network{username = <<"user">>, password = <<"pass">>,
host = "127.0.0.1", ssl_options = TLSOpts9}} =
amqp_uri:parse("amqps://user:pass@127.0.0.1/%2f?cacertfile=/path/to/cacertfile.pem"
"&certfile=/path/to/certfile.pem"
"&password=topsecret"
"&depth=5"),
Expand Down

0 comments on commit ccb232c

Please sign in to comment.