From 64aa30ec631e37970db0568d1aea83e44987a9f3 Mon Sep 17 00:00:00 2001 From: Ilya Averyanov Date: Tue, 2 Aug 2022 15:20:56 +0300 Subject: [PATCH] chore(authn/authz): better handling of placeholder interpolation errors --- .gitignore | 1 + CHANGES-5.0.md | 1 + apps/emqx_authn/include/emqx_authn.hrl | 4 ++ apps/emqx_authn/src/emqx_authn.app.src | 2 +- apps/emqx_authn/src/emqx_authn_utils.erl | 15 ++++- .../src/simple_authn/emqx_authn_http.erl | 42 +++++++------- .../src/simple_authn/emqx_authn_mongodb.erl | 50 +++++++++-------- .../src/simple_authn/emqx_authn_mysql.erl | 54 +++++++++--------- .../src/simple_authn/emqx_authn_pgsql.erl | 52 +++++++++-------- .../src/simple_authn/emqx_authn_redis.erl | 56 ++++++++++--------- .../emqx_authn/test/emqx_authn_http_SUITE.erl | 41 ++++++++++++++ .../test/emqx_authn_mongo_SUITE.erl | 14 +++++ .../test/emqx_authn_mysql_SUITE.erl | 14 +++++ .../test/emqx_authn_pgsql_SUITE.erl | 14 +++++ .../test/emqx_authn_redis_SUITE.erl | 14 +++++ apps/emqx_authz/src/emqx_authz.app.src | 2 +- apps/emqx_authz/src/emqx_authz.erl | 8 +++ apps/emqx_authz/src/emqx_authz_utils.erl | 8 +-- 18 files changed, 268 insertions(+), 124 deletions(-) diff --git a/.gitignore b/.gitignore index d8b3806e3..26b146cef 100644 --- a/.gitignore +++ b/.gitignore @@ -68,3 +68,4 @@ apps/emqx/test/emqx_static_checks_data/master.bpapi # rendered configurations *.conf.rendered lux_logs/ +.ci/docker-compose-file/redis/*.log diff --git a/CHANGES-5.0.md b/CHANGES-5.0.md index 40a949d13..734b88515 100644 --- a/CHANGES-5.0.md +++ b/CHANGES-5.0.md @@ -11,6 +11,7 @@ * The license is now copied to all nodes in the cluster when it's reloaded. [#8598](https://github.com/emqx/emqx/pull/8598) * Added a HTTP API to manage licenses. [#8610](https://github.com/emqx/emqx/pull/8610) * Updated `/nodes` API node_status from `Running/Stopped` to `running/stopped`. [#8642](https://github.com/emqx/emqx/pull/8642) +* Improve handling of placeholder interpolation errors [#8635](https://github.com/emqx/emqx/pull/8635) * Better logging on unknown object IDs. [#8670](https://github.com/emqx/emqx/pull/8670) # 5.0.4 diff --git a/apps/emqx_authn/include/emqx_authn.hrl b/apps/emqx_authn/include/emqx_authn.hrl index d59eea1af..ba5f80a74 100644 --- a/apps/emqx_authn/include/emqx_authn.hrl +++ b/apps/emqx_authn/include/emqx_authn.hrl @@ -38,4 +38,8 @@ -define(RESOURCE_GROUP, <<"emqx_authn">>). +-define(WITH_SUCCESSFUL_RENDER(Code), + emqx_authn_utils:with_successful_render(?MODULE, fun() -> Code end) +). + -endif. diff --git a/apps/emqx_authn/src/emqx_authn.app.src b/apps/emqx_authn/src/emqx_authn.app.src index 8087e822f..ef67b9a14 100644 --- a/apps/emqx_authn/src/emqx_authn.app.src +++ b/apps/emqx_authn/src/emqx_authn.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_authn, [ {description, "EMQX Authentication"}, - {vsn, "0.1.3"}, + {vsn, "0.1.4"}, {modules, []}, {registered, [emqx_authn_sup, emqx_authn_registry]}, {applications, [kernel, stdlib, emqx_resource, ehttpc, epgsql, mysql, jose]}, diff --git a/apps/emqx_authn/src/emqx_authn_utils.erl b/apps/emqx_authn/src/emqx_authn_utils.erl index 994f2f275..b989da3b4 100644 --- a/apps/emqx_authn/src/emqx_authn_utils.erl +++ b/apps/emqx_authn/src/emqx_authn_utils.erl @@ -34,7 +34,8 @@ ensure_apps_started/1, cleanup_resources/0, make_resource_id/1, - without_password/1 + without_password/1, + with_successful_render/2 ]). -define(AUTHN_PLACEHOLDERS, [ @@ -136,6 +137,18 @@ render_sql_params(ParamList, Credential) -> #{return => rawlist, var_trans => fun handle_sql_var/2} ). +with_successful_render(Provider, Fun) when is_function(Fun, 0) -> + try + Fun() + catch + error:{cannot_get_variable, Name} -> + ?TRACE_AUTHN(error, "placeholder_interpolation_failed", #{ + provider => Provider, + placeholder => Name + }), + ignore + end. + %% true is_superuser(#{<<"is_superuser">> := <<"true">>}) -> #{is_superuser => true}; 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 db3eb3c2f..2304cf1e4 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -187,25 +187,29 @@ authenticate( request_timeout := RequestTimeout } = State ) -> - Request = generate_request(Credential, State), - Response = emqx_resource:query(ResourceId, {Method, Request, RequestTimeout}), - ?TRACE_AUTHN_PROVIDER("http_response", #{ - request => request_for_log(Credential, State), - response => response_for_log(Response), - resource => ResourceId - }), - case Response of - {ok, 204, _Headers} -> - {ok, #{is_superuser => false}}; - {ok, 200, Headers, Body} -> - handle_response(Headers, Body); - {ok, _StatusCode, _Headers} = Response -> - ignore; - {ok, _StatusCode, _Headers, _Body} = Response -> - ignore; - {error, _Reason} -> - ignore - end. + ?WITH_SUCCESSFUL_RENDER( + begin + Request = generate_request(Credential, State), + Response = emqx_resource:query(ResourceId, {Method, Request, RequestTimeout}), + ?TRACE_AUTHN_PROVIDER("http_response", #{ + request => request_for_log(Credential, State), + response => response_for_log(Response), + resource => ResourceId + }), + case Response of + {ok, 204, _Headers} -> + {ok, #{is_superuser => false}}; + {ok, 200, Headers, Body} -> + handle_response(Headers, Body); + {ok, _StatusCode, _Headers} = Response -> + ignore; + {ok, _StatusCode, _Headers, _Body} = Response -> + ignore; + {error, _Reason} -> + ignore + end + end + ). destroy(#{resource_id := ResourceId}) -> _ = emqx_resource:remove_local(ResourceId), diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index f7249ae57..43a1ebd3b 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -162,35 +162,39 @@ authenticate( resource_id := ResourceId } = State ) -> - Filter = emqx_authn_utils:render_deep(FilterTemplate, Credential), - case emqx_resource:query(ResourceId, {find_one, Collection, Filter, #{}}) of - undefined -> - ignore; - {error, Reason} -> - ?TRACE_AUTHN_PROVIDER(error, "mongodb_query_failed", #{ - resource => ResourceId, - collection => Collection, - filter => Filter, - reason => Reason - }), - ignore; - Doc -> - case check_password(Password, Doc, State) of - ok -> - {ok, is_superuser(Doc, State)}; - {error, {cannot_find_password_hash_field, PasswordHashField}} -> - ?TRACE_AUTHN_PROVIDER(error, "cannot_find_password_hash_field", #{ + ?WITH_SUCCESSFUL_RENDER( + begin + Filter = emqx_authn_utils:render_deep(FilterTemplate, Credential), + case emqx_resource:query(ResourceId, {find_one, Collection, Filter, #{}}) of + undefined -> + ignore; + {error, Reason} -> + ?TRACE_AUTHN_PROVIDER(error, "mongodb_query_failed", #{ resource => ResourceId, collection => Collection, filter => Filter, - document => Doc, - password_hash_field => PasswordHashField + reason => Reason }), ignore; - {error, Reason} -> - {error, Reason} + Doc -> + case check_password(Password, Doc, State) of + ok -> + {ok, is_superuser(Doc, State)}; + {error, {cannot_find_password_hash_field, PasswordHashField}} -> + ?TRACE_AUTHN_PROVIDER(error, "cannot_find_password_hash_field", #{ + resource => ResourceId, + collection => Collection, + filter => Filter, + document => Doc, + password_hash_field => PasswordHashField + }), + ignore; + {error, Reason} -> + {error, Reason} + end end - end. + end + ). %%------------------------------------------------------------------------------ %% Internal functions diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl index e95302ad4..4efa62670 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -113,32 +113,36 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - Params = emqx_authn_utils:render_sql_params(TmplToken, Credential), - case emqx_resource:query(ResourceId, {prepared_query, ?PREPARE_KEY, Params, Timeout}) of - {ok, _Columns, []} -> - ignore; - {ok, Columns, [Row | _]} -> - Selected = maps:from_list(lists:zip(Columns, Row)), - case - emqx_authn_utils:check_password_from_selected_map( - Algorithm, Selected, Password - ) - of - ok -> - {ok, emqx_authn_utils:is_superuser(Selected)}; + ?WITH_SUCCESSFUL_RENDER( + begin + Params = emqx_authn_utils:render_sql_params(TmplToken, Credential), + case emqx_resource:query(ResourceId, {prepared_query, ?PREPARE_KEY, Params, Timeout}) of + {ok, _Columns, []} -> + ignore; + {ok, Columns, [Row | _]} -> + Selected = maps:from_list(lists:zip(Columns, Row)), + case + emqx_authn_utils:check_password_from_selected_map( + Algorithm, Selected, Password + ) + of + ok -> + {ok, emqx_authn_utils:is_superuser(Selected)}; + {error, Reason} -> + {error, Reason} + end; {error, Reason} -> - {error, Reason} - end; - {error, Reason} -> - ?TRACE_AUTHN_PROVIDER(error, "mysql_query_failed", #{ - resource => ResourceId, - tmpl_token => TmplToken, - params => Params, - timeout => Timeout, - reason => Reason - }), - ignore - end. + ?TRACE_AUTHN_PROVIDER(error, "mysql_query_failed", #{ + resource => ResourceId, + tmpl_token => TmplToken, + params => Params, + timeout => Timeout, + reason => Reason + }), + ignore + end + end + ). parse_config( #{ diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl index 2962308ab..f8b47959a 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -115,31 +115,35 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - Params = emqx_authn_utils:render_sql_params(PlaceHolders, Credential), - case emqx_resource:query(ResourceId, {prepared_query, ResourceId, Params}) of - {ok, _Columns, []} -> - ignore; - {ok, Columns, [Row | _]} -> - NColumns = [Name || #column{name = Name} <- Columns], - Selected = maps:from_list(lists:zip(NColumns, erlang:tuple_to_list(Row))), - case - emqx_authn_utils:check_password_from_selected_map( - Algorithm, Selected, Password - ) - of - ok -> - {ok, emqx_authn_utils:is_superuser(Selected)}; + ?WITH_SUCCESSFUL_RENDER( + begin + Params = emqx_authn_utils:render_sql_params(PlaceHolders, Credential), + case emqx_resource:query(ResourceId, {prepared_query, ResourceId, Params}) of + {ok, _Columns, []} -> + ignore; + {ok, Columns, [Row | _]} -> + NColumns = [Name || #column{name = Name} <- Columns], + Selected = maps:from_list(lists:zip(NColumns, erlang:tuple_to_list(Row))), + case + emqx_authn_utils:check_password_from_selected_map( + Algorithm, Selected, Password + ) + of + ok -> + {ok, emqx_authn_utils:is_superuser(Selected)}; + {error, Reason} -> + {error, Reason} + end; {error, Reason} -> - {error, Reason} - end; - {error, Reason} -> - ?TRACE_AUTHN_PROVIDER(error, "postgresql_query_failed", #{ - resource => ResourceId, - params => Params, - reason => Reason - }), - ignore - end. + ?TRACE_AUTHN_PROVIDER(error, "postgresql_query_failed", #{ + resource => ResourceId, + params => Params, + reason => Reason + }), + ignore + end + end + ). parse_config( #{ diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl index 71cd292e6..684d60e49 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -133,33 +133,37 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - NKey = emqx_authn_utils:render_str(KeyTemplate, Credential), - Command = [CommandName, NKey | Fields], - case emqx_resource:query(ResourceId, {cmd, Command}) of - {ok, []} -> - ignore; - {ok, Values} -> - Selected = merge(Fields, Values), - case - emqx_authn_utils:check_password_from_selected_map( - Algorithm, Selected, Password - ) - of - ok -> - {ok, emqx_authn_utils:is_superuser(Selected)}; + ?WITH_SUCCESSFUL_RENDER( + begin + NKey = emqx_authn_utils:render_str(KeyTemplate, Credential), + Command = [CommandName, NKey | Fields], + case emqx_resource:query(ResourceId, {cmd, Command}) of + {ok, []} -> + ignore; + {ok, Values} -> + Selected = merge(Fields, Values), + case + emqx_authn_utils:check_password_from_selected_map( + Algorithm, Selected, Password + ) + of + ok -> + {ok, emqx_authn_utils:is_superuser(Selected)}; + {error, Reason} -> + {error, Reason} + end; {error, Reason} -> - {error, Reason} - end; - {error, Reason} -> - ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ - resource => ResourceId, - cmd => Command, - keys => NKey, - fields => Fields, - reason => Reason - }), - ignore - end. + ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ + resource => ResourceId, + cmd => Command, + keys => NKey, + fields => Fields, + reason => Reason + }), + ignore + end + end + ). %%------------------------------------------------------------------------------ %% Internal functions diff --git a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl index 44ef43903..db22a6cfe 100644 --- a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl @@ -247,6 +247,27 @@ t_update(_Config) -> emqx_access_control:authenticate(?CREDENTIALS) ). +t_interpolation_error(_Config) -> + {ok, _} = emqx:update_config( + ?PATH, + {create_authenticator, ?GLOBAL, raw_http_auth_config()} + ), + + Headers = #{<<"content-type">> => <<"application/json">>}, + Response = ?SERVER_RESPONSE_JSON(allow), + + ok = emqx_authn_http_test_server:set_handler( + fun(Req0, State) -> + Req = cowboy_req:reply(200, Headers, Response, Req0), + {ok, Req, State} + end + ), + + ?assertMatch( + ?EXCEPTION_DENY, + emqx_access_control:authenticate(maps:without([username], ?CREDENTIALS)) + ). + t_is_superuser(_Config) -> Config = raw_http_auth_config(), {ok, _} = emqx:update_config( @@ -410,6 +431,26 @@ samples() -> result => {ok, #{is_superuser => false, user_property => #{}}} }, + %% simple get request, no username + #{ + handler => fun(Req0, State) -> + #{ + username := <<"plain">>, + password := <<"plain">> + } = cowboy_req:match_qs([username, password], Req0), + + Req = cowboy_req:reply( + 200, + #{<<"content-type">> => <<"application/json">>}, + jiffy:encode(#{result => allow, is_superuser => false}), + Req0 + ), + {ok, Req, State} + end, + config_params => #{}, + result => {ok, #{is_superuser => false, user_property => #{}}} + }, + %% get request with json body response #{ handler => fun(Req0, State) -> diff --git a/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl index 2f7dd2391..ddde18c49 100644 --- a/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl @@ -288,6 +288,20 @@ raw_mongo_auth_config() -> user_seeds() -> [ + #{ + data => #{ + username => <<"plain">>, + password_hash => <<"plainsalt">>, + salt => <<"salt">>, + is_superuser => <<"1">> + }, + credentials => #{ + password => <<"plain">> + }, + config_params => #{}, + result => {error, not_authorized} + }, + #{ data => #{ username => <<"plain">>, diff --git a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl index 0fdba0b31..175aa7f1d 100644 --- a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl @@ -258,6 +258,20 @@ raw_mysql_auth_config() -> user_seeds() -> [ + #{ + data => #{ + username => "plain", + password_hash => "plainsalt", + salt => "salt", + is_superuser_str => "1" + }, + credentials => #{ + password => <<"plain">> + }, + config_params => #{}, + result => {error, not_authorized} + }, + #{ data => #{ username => "plain", diff --git a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl index ff017a79e..02095c07d 100644 --- a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl @@ -320,6 +320,20 @@ raw_pgsql_auth_config() -> user_seeds() -> [ + #{ + data => #{ + username => "plain", + password_hash => "plainsalt", + salt => "salt", + is_superuser_str => "1" + }, + credentials => #{ + password => <<"plain">> + }, + config_params => #{}, + result => {error, not_authorized} + }, + #{ data => #{ username => "plain", diff --git a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl index dde5f8188..889404c5e 100644 --- a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl @@ -280,6 +280,20 @@ raw_redis_auth_config() -> user_seeds() -> [ + #{ + data => #{ + password_hash => <<"plainsalt">>, + salt => <<"salt">>, + is_superuser => <<"1">> + }, + credentials => #{ + password => <<"plain">> + }, + key => <<"mqtt_user:plain">>, + config_params => #{}, + result => {error, not_authorized} + }, + #{ data => #{ password_hash => <<"plainsalt">>, diff --git a/apps/emqx_authz/src/emqx_authz.app.src b/apps/emqx_authz/src/emqx_authz.app.src index ed19b15a8..e40b5e64c 100644 --- a/apps/emqx_authz/src/emqx_authz.app.src +++ b/apps/emqx_authz/src/emqx_authz.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_authz, [ {description, "An OTP application"}, - {vsn, "0.1.3"}, + {vsn, "0.1.4"}, {registered, []}, {mod, {emqx_authz_app, []}}, {applications, [ diff --git a/apps/emqx_authz/src/emqx_authz.erl b/apps/emqx_authz/src/emqx_authz.erl index f7ebcece0..8eb3c6eae 100644 --- a/apps/emqx_authz/src/emqx_authz.erl +++ b/apps/emqx_authz/src/emqx_authz.erl @@ -402,6 +402,14 @@ do_authorize( Matched -> {Matched, Type} catch + error:{cannot_get_variable, Name} -> + emqx_metrics_worker:inc(authz_metrics, Type, nomatch), + ?SLOG(warning, #{ + msg => "placeholder_interpolation_failed", + placeholder => Name, + authorize_type => Type + }), + do_authorize(Client, PubSub, Topic, Tail); Class:Reason:Stacktrace -> emqx_metrics_worker:inc(authz_metrics, Type, nomatch), ?SLOG(warning, #{ diff --git a/apps/emqx_authz/src/emqx_authz_utils.erl b/apps/emqx_authz/src/emqx_authz_utils.erl index dd6f66c7f..d364bc5fa 100644 --- a/apps/emqx_authz/src/emqx_authz_utils.erl +++ b/apps/emqx_authz/src/emqx_authz_utils.erl @@ -181,15 +181,15 @@ convert_client_var({dn, DN}) -> {cert_subject, DN}; convert_client_var({protocol, Proto}) -> {proto_name, Proto}; convert_client_var(Other) -> Other. -handle_var({var, _Name}, undefined) -> - "undefined"; +handle_var({var, Name}, undefined) -> + error({cannot_get_variable, Name}); handle_var({var, <<"peerhost">>}, IpAddr) -> inet_parse:ntoa(IpAddr); handle_var(_Name, Value) -> emqx_placeholder:bin(Value). -handle_sql_var({var, _Name}, undefined) -> - "undefined"; +handle_sql_var({var, Name}, undefined) -> + error({cannot_get_variable, Name}); handle_sql_var({var, <<"peerhost">>}, IpAddr) -> inet_parse:ntoa(IpAddr); handle_sql_var(_Name, Value) ->