From 88ca94b4171c4880013f15312fed2d33541d4f8e Mon Sep 17 00:00:00 2001 From: Ilya Averyanov Date: Mon, 17 Apr 2023 20:00:57 +0300 Subject: [PATCH] fix(auth): fix uri path handling Fix uri path handling `emqx_connector_http`, HTTP authentication and authorization backends. --- apps/emqx_authn/src/emqx_authn_utils.erl | 13 ++++ .../src/simple_authn/emqx_authn_http.erl | 14 ++-- .../emqx_authn/test/emqx_authn_http_SUITE.erl | 49 +++++++++++++- apps/emqx_authz/src/emqx_authz_http.erl | 14 ++-- apps/emqx_authz/src/emqx_authz_utils.erl | 14 +++- .../emqx_authz/test/emqx_authz_http_SUITE.erl | 10 +-- .../src/emqx_connector_http.erl | 65 +++++++++++++++---- changes/ce/fix-10420.en.md | 3 + changes/ce/fix-10420.zh.md | 0 9 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 changes/ce/fix-10420.en.md create mode 100644 changes/ce/fix-10420.zh.md diff --git a/apps/emqx_authn/src/emqx_authn_utils.erl b/apps/emqx_authn/src/emqx_authn_utils.erl index 1352e3daf..12520251e 100644 --- a/apps/emqx_authn/src/emqx_authn_utils.erl +++ b/apps/emqx_authn/src/emqx_authn_utils.erl @@ -28,6 +28,7 @@ parse_sql/2, render_deep/2, render_str/2, + render_urlencoded_str/2, render_sql_params/2, is_superuser/1, bin/1, @@ -129,6 +130,13 @@ render_str(Template, Credential) -> #{return => full_binary, var_trans => fun handle_var/2} ). +render_urlencoded_str(Template, Credential) -> + emqx_placeholder:proc_tmpl( + Template, + mapping_credential(Credential), + #{return => full_binary, var_trans => fun urlencode_var/2} + ). + render_sql_params(ParamList, Credential) -> emqx_placeholder:proc_tmpl( ParamList, @@ -217,6 +225,11 @@ without_password(Credential, [Name | Rest]) -> without_password(Credential, Rest) end. +urlencode_var({var, _} = Var, Value) -> + emqx_http_lib:uri_encode(handle_var(Var, Value)); +urlencode_var(Var, Value) -> + handle_var(Var, Value). + handle_var({var, _Name}, undefined) -> <<>>; handle_var({var, <<"peerhost">>}, PeerHost) -> diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl index 3c34d878e..502562e2c 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -285,9 +285,9 @@ parse_url(Url) -> BaseUrl = iolist_to_binary([Scheme, "//", HostPort]), case string:split(Remaining, "?", leading) of [Path, QueryString] -> - {BaseUrl, Path, QueryString}; + {BaseUrl, <<"/", Path/binary>>, QueryString}; [Path] -> - {BaseUrl, Path, <<>>} + {BaseUrl, <<"/", Path/binary>>, <<>>} end; [HostPort] -> {iolist_to_binary([Scheme, "//", HostPort]), <<>>, <<>>} @@ -328,7 +328,7 @@ generate_request(Credential, #{ body_template := BodyTemplate }) -> Headers = maps:to_list(Headers0), - Path = emqx_authn_utils:render_str(BasePathTemplate, Credential), + Path = emqx_authn_utils:render_urlencoded_str(BasePathTemplate, Credential), Query = emqx_authn_utils:render_deep(BaseQueryTemplate, Credential), Body = emqx_authn_utils:render_deep(BodyTemplate, Credential), case Method of @@ -343,9 +343,9 @@ generate_request(Credential, #{ end. append_query(Path, []) -> - encode_path(Path); + Path; append_query(Path, Query) -> - encode_path(Path) ++ "?" ++ binary_to_list(qs(Query)). + Path ++ "?" ++ binary_to_list(qs(Query)). qs(KVs) -> qs(KVs, []). @@ -407,10 +407,6 @@ parse_body(ContentType, _) -> uri_encode(T) -> emqx_http_lib:uri_encode(to_list(T)). -encode_path(Path) -> - Parts = string:split(Path, "/", all), - lists:flatten(["/" ++ Part || Part <- lists:map(fun uri_encode/1, Parts)]). - request_for_log(Credential, #{url := Url} = State) -> SafeCredential = emqx_authn_utils:without_password(Credential), case generate_request(SafeCredential, State) of diff --git a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl index 851e80f6d..b08167a5b 100644 --- a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl @@ -47,7 +47,6 @@ }) ). --define(SERVER_RESPONSE_URLENCODE(Result), ?SERVER_RESPONSE_URLENCODE(Result, false)). -define(SERVER_RESPONSE_URLENCODE(Result, IsSuperuser), list_to_binary( "result=" ++ @@ -166,6 +165,54 @@ test_user_auth(#{ ?GLOBAL ). +t_authenticate_path_placeholders(_Config) -> + ok = emqx_authn_http_test_server:stop(), + {ok, _} = emqx_authn_http_test_server:start_link(?HTTP_PORT, <<"/[...]">>), + ok = emqx_authn_http_test_server:set_handler( + fun(Req0, State) -> + Req = + case cowboy_req:path(Req0) of + <<"/my/p%20ath//us%20er/auth//">> -> + cowboy_req:reply( + 200, + #{<<"content-type">> => <<"application/json">>}, + emqx_utils_json:encode(#{result => allow, is_superuser => false}), + Req0 + ); + Path -> + ct:pal("Unexpected path: ~p", [Path]), + cowboy_req:reply(403, Req0) + end, + {ok, Req, State} + end + ), + + Credentials = ?CREDENTIALS#{ + username => <<"us er">> + }, + + AuthConfig = maps:merge( + raw_http_auth_config(), + #{ + <<"url">> => <<"http://127.0.0.1:32333/my/p%20ath//${username}/auth//">>, + <<"body">> => #{} + } + ), + {ok, _} = emqx:update_config( + ?PATH, + {create_authenticator, ?GLOBAL, AuthConfig} + ), + + ?assertMatch( + {ok, #{is_superuser := false}}, + emqx_access_control:authenticate(Credentials) + ), + + _ = emqx_authn_test_lib:delete_authenticators( + [authentication], + ?GLOBAL + ). + t_no_value_for_placeholder(_Config) -> Handler = fun(Req0, State) -> {ok, RawBody, Req1} = cowboy_req:read_body(Req0), diff --git a/apps/emqx_authz/src/emqx_authz_http.erl b/apps/emqx_authz/src/emqx_authz_http.erl index 53378d9c2..5747e6eeb 100644 --- a/apps/emqx_authz/src/emqx_authz_http.erl +++ b/apps/emqx_authz/src/emqx_authz_http.erl @@ -161,9 +161,9 @@ parse_url(Url) -> BaseUrl = iolist_to_binary([Scheme, "//", HostPort]), case string:split(Remaining, "?", leading) of [Path, QueryString] -> - {BaseUrl, Path, QueryString}; + {BaseUrl, <<"/", Path/binary>>, QueryString}; [Path] -> - {BaseUrl, Path, <<>>} + {BaseUrl, <<"/", Path/binary>>, <<>>} end; [HostPort] -> {iolist_to_binary([Scheme, "//", HostPort]), <<>>, <<>>} @@ -185,7 +185,7 @@ generate_request( } ) -> Values = client_vars(Client, PubSub, Topic), - Path = emqx_authz_utils:render_str(BasePathTemplate, Values), + Path = emqx_authz_utils:render_urlencoded_str(BasePathTemplate, Values), Query = emqx_authz_utils:render_deep(BaseQueryTemplate, Values), Body = emqx_authz_utils:render_deep(BodyTemplate, Values), case Method of @@ -202,9 +202,9 @@ generate_request( end. append_query(Path, []) -> - encode_path(Path); + to_list(Path); append_query(Path, Query) -> - encode_path(Path) ++ "?" ++ to_list(query_string(Query)). + to_list(Path) ++ "?" ++ to_list(query_string(Query)). query_string(Body) -> query_string(Body, []). @@ -222,10 +222,6 @@ query_string([{K, V} | More], Acc) -> uri_encode(T) -> emqx_http_lib:uri_encode(to_list(T)). -encode_path(Path) -> - Parts = string:split(Path, "/", all), - lists:flatten(["/" ++ Part || Part <- lists:map(fun uri_encode/1, Parts)]). - serialize_body(<<"application/json">>, Body) -> emqx_utils_json:encode(Body); serialize_body(<<"application/x-www-form-urlencoded">>, Body) -> diff --git a/apps/emqx_authz/src/emqx_authz_utils.erl b/apps/emqx_authz/src/emqx_authz_utils.erl index 560141d0a..c01505680 100644 --- a/apps/emqx_authz/src/emqx_authz_utils.erl +++ b/apps/emqx_authz/src/emqx_authz_utils.erl @@ -16,7 +16,6 @@ -module(emqx_authz_utils). --include_lib("emqx/include/emqx_placeholder.hrl"). -include_lib("emqx_authz.hrl"). -export([ @@ -28,6 +27,7 @@ update_config/2, parse_deep/2, parse_str/2, + render_urlencoded_str/2, parse_sql/3, render_deep/2, render_str/2, @@ -128,6 +128,13 @@ render_str(Template, Values) -> #{return => full_binary, var_trans => fun handle_var/2} ). +render_urlencoded_str(Template, Values) -> + emqx_placeholder:proc_tmpl( + Template, + client_vars(Values), + #{return => full_binary, var_trans => fun urlencode_var/2} + ). + render_sql_params(ParamList, Values) -> emqx_placeholder:proc_tmpl( ParamList, @@ -181,6 +188,11 @@ convert_client_var({dn, DN}) -> {cert_subject, DN}; convert_client_var({protocol, Proto}) -> {proto_name, Proto}; convert_client_var(Other) -> Other. +urlencode_var({var, _} = Var, Value) -> + emqx_http_lib:uri_encode(handle_var(Var, Value)); +urlencode_var(Var, Value) -> + handle_var(Var, Value). + handle_var({var, _Name}, undefined) -> <<>>; handle_var({var, <<"peerhost">>}, IpAddr) -> diff --git a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl index 9ff84b805..702bf2756 100644 --- a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl @@ -199,7 +199,7 @@ t_query_params(_Config) -> peerhost := <<"127.0.0.1">>, proto_name := <<"MQTT">>, mountpoint := <<"MOUNTPOINT">>, - topic := <<"t">>, + topic := <<"t/1">>, action := <<"publish">> } = cowboy_req:match_qs( [ @@ -241,7 +241,7 @@ t_query_params(_Config) -> ?assertEqual( allow, - emqx_access_control:authorize(ClientInfo, publish, <<"t">>) + emqx_access_control:authorize(ClientInfo, publish, <<"t/1">>) ). t_path(_Config) -> @@ -249,13 +249,13 @@ t_path(_Config) -> fun(Req0, State) -> ?assertEqual( << - "/authz/users/" + "/authz/use%20rs/" "user%20name/" "client%20id/" "127.0.0.1/" "MQTT/" "MOUNTPOINT/" - "t/1/" + "t%2F1/" "publish" >>, cowboy_req:path(Req0) @@ -264,7 +264,7 @@ t_path(_Config) -> end, #{ <<"url">> => << - "http://127.0.0.1:33333/authz/users/" + "http://127.0.0.1:33333/authz/use%20rs/" "${username}/" "${clientid}/" "${peerhost}/" diff --git a/apps/emqx_connector/src/emqx_connector_http.erl b/apps/emqx_connector/src/emqx_connector_http.erl index ef2e11eb7..610632e36 100644 --- a/apps/emqx_connector/src/emqx_connector_http.erl +++ b/apps/emqx_connector/src/emqx_connector_http.erl @@ -47,7 +47,7 @@ namespace/0 ]). --export([check_ssl_opts/2, validate_method/1]). +-export([check_ssl_opts/2, validate_method/1, join_paths/2]). -type connect_timeout() :: emqx_schema:duration() | infinity. -type pool_type() :: random | hash. @@ -458,7 +458,7 @@ preprocess_request( } = Req ) -> #{ - method => emqx_plugin_libs_rule:preproc_tmpl(bin(Method)), + method => emqx_plugin_libs_rule:preproc_tmpl(to_bin(Method)), path => emqx_plugin_libs_rule:preproc_tmpl(Path), body => maybe_preproc_tmpl(body, Req), headers => preproc_headers(Headers), @@ -471,8 +471,8 @@ preproc_headers(Headers) when is_map(Headers) -> fun(K, V, Acc) -> [ { - emqx_plugin_libs_rule:preproc_tmpl(bin(K)), - emqx_plugin_libs_rule:preproc_tmpl(bin(V)) + emqx_plugin_libs_rule:preproc_tmpl(to_bin(K)), + emqx_plugin_libs_rule:preproc_tmpl(to_bin(V)) } | Acc ] @@ -484,8 +484,8 @@ preproc_headers(Headers) when is_list(Headers) -> lists:map( fun({K, V}) -> { - emqx_plugin_libs_rule:preproc_tmpl(bin(K)), - emqx_plugin_libs_rule:preproc_tmpl(bin(V)) + emqx_plugin_libs_rule:preproc_tmpl(to_bin(K)), + emqx_plugin_libs_rule:preproc_tmpl(to_bin(V)) } end, Headers @@ -553,15 +553,41 @@ formalize_request(Method, BasePath, {Path, Headers, _Body}) when -> formalize_request(Method, BasePath, {Path, Headers}); formalize_request(_Method, BasePath, {Path, Headers, Body}) -> - {filename:join(BasePath, Path), Headers, Body}; + {join_paths(BasePath, Path), Headers, Body}; formalize_request(_Method, BasePath, {Path, Headers}) -> - {filename:join(BasePath, Path), Headers}. + {join_paths(BasePath, Path), Headers}. -bin(Bin) when is_binary(Bin) -> +%% By default, we cannot treat HTTP paths as "file" or "resource" paths, +%% because an HTTP server may handle paths like +%% "/a/b/c/", "/a/b/c" and "/a//b/c" differently. +%% +%% So we try to avoid unneccessary path normalization. +%% +%% See also: `join_paths_test_/0` +join_paths(Path1, Path2) -> + do_join_paths(lists:reverse(to_list(Path1)), to_list(Path2)). + +%% "abc/" + "/cde" +do_join_paths([$/ | Path1], [$/ | Path2]) -> + lists:reverse(Path1) ++ [$/ | Path2]; +%% "abc/" + "cde" +do_join_paths([$/ | Path1], Path2) -> + lists:reverse(Path1) ++ [$/ | Path2]; +%% "abc" + "/cde" +do_join_paths(Path1, [$/ | Path2]) -> + lists:reverse(Path1) ++ [$/ | Path2]; +%% "abc" + "cde" +do_join_paths(Path1, Path2) -> + lists:reverse(Path1) ++ [$/ | Path2]. + +to_list(List) when is_list(List) -> List; +to_list(Bin) when is_binary(Bin) -> binary_to_list(Bin). + +to_bin(Bin) when is_binary(Bin) -> Bin; -bin(Str) when is_list(Str) -> +to_bin(Str) when is_list(Str) -> list_to_binary(Str); -bin(Atom) when is_atom(Atom) -> +to_bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom, utf8). reply_delegator(ReplyFunAndArgs, Result) -> @@ -642,4 +668,21 @@ redact_test_() -> ?_assertNotEqual(TestData2, redact(TestData2)) ]. +join_paths_test_() -> + [ + ?_assertEqual("abc/cde", join_paths("abc", "cde")), + ?_assertEqual("abc/cde", join_paths("abc", "/cde")), + ?_assertEqual("abc/cde", join_paths("abc/", "cde")), + ?_assertEqual("abc/cde", join_paths("abc/", "/cde")), + + ?_assertEqual("/", join_paths("", "")), + ?_assertEqual("/cde", join_paths("", "cde")), + ?_assertEqual("/cde", join_paths("", "/cde")), + ?_assertEqual("/cde", join_paths("/", "cde")), + ?_assertEqual("/cde", join_paths("/", "/cde")), + + ?_assertEqual("//cde/", join_paths("/", "//cde/")), + ?_assertEqual("abc///cde/", join_paths("abc//", "//cde/")) + ]. + -endif. diff --git a/changes/ce/fix-10420.en.md b/changes/ce/fix-10420.en.md new file mode 100644 index 000000000..70afd8b45 --- /dev/null +++ b/changes/ce/fix-10420.en.md @@ -0,0 +1,3 @@ +Fix HTTP path handling when composing the URL for the HTTP requests in authentication and authorization modules. +* Avoid unnecessary URL normalization since we cannot assume that external servers treat original and normalized URLs equally. This led to bugs like [#10411](https://github.com/emqx/emqx/issues/10411). +* Fix the issue that path segments could be HTTP encoded twice. diff --git a/changes/ce/fix-10420.zh.md b/changes/ce/fix-10420.zh.md new file mode 100644 index 000000000..e69de29bb