diff --git a/.gitignore b/.gitignore index 26b146cef..d8b3806e3 100644 --- a/.gitignore +++ b/.gitignore @@ -68,4 +68,3 @@ 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 cfd36d751..fe586f1fb 100644 --- a/CHANGES-5.0.md +++ b/CHANGES-5.0.md @@ -8,6 +8,7 @@ * Check ACLs for last will testament topic before publishing the message. [#8930](https://github.com/emqx/emqx/pull/8930) * Fix GET /listeners API crash When some nodes still in initial configuration. [#9002](https://github.com/emqx/emqx/pull/9002) +* Fix empty variable interpolation in authentication and authorization. Placeholders for undefined variables are rendered now as empty strings and do not cause errors anymore. [#8963](https://github.com/emqx/emqx/pull/8963) # 5.0.8 diff --git a/apps/emqx_authn/include/emqx_authn.hrl b/apps/emqx_authn/include/emqx_authn.hrl index ba5f80a74..d59eea1af 100644 --- a/apps/emqx_authn/include/emqx_authn.hrl +++ b/apps/emqx_authn/include/emqx_authn.hrl @@ -38,8 +38,4 @@ -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_utils.erl b/apps/emqx_authn/src/emqx_authn_utils.erl index b989da3b4..3b0d96905 100644 --- a/apps/emqx_authn/src/emqx_authn_utils.erl +++ b/apps/emqx_authn/src/emqx_authn_utils.erl @@ -34,8 +34,7 @@ ensure_apps_started/1, cleanup_resources/0, make_resource_id/1, - without_password/1, - with_successful_render/2 + without_password/1 ]). -define(AUTHN_PLACEHOLDERS, [ @@ -137,18 +136,6 @@ 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}; @@ -230,15 +217,15 @@ without_password(Credential, [Name | Rest]) -> without_password(Credential, Rest) end. -handle_var({var, Name}, undefined) -> - error({cannot_get_variable, Name}); +handle_var({var, _Name}, undefined) -> + <<>>; handle_var({var, <<"peerhost">>}, PeerHost) -> emqx_placeholder:bin(inet:ntoa(PeerHost)); handle_var(_, Value) -> emqx_placeholder:bin(Value). -handle_sql_var({var, Name}, undefined) -> - error({cannot_get_variable, Name}); +handle_sql_var({var, _Name}, undefined) -> + <<>>; handle_sql_var({var, <<"peerhost">>}, PeerHost) -> emqx_placeholder:bin(inet:ntoa(PeerHost)); handle_sql_var(_, Value) -> 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 8489debcd..7527edcb3 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -187,29 +187,25 @@ authenticate( request_timeout := RequestTimeout } = State ) -> - ?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 - ). + 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. 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 43a1ebd3b..f7249ae57 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -162,39 +162,35 @@ authenticate( resource_id := ResourceId } = State ) -> - ?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", #{ + 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", #{ resource => ResourceId, collection => Collection, filter => Filter, - reason => Reason + document => Doc, + password_hash_field => PasswordHashField }), 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", #{ - resource => ResourceId, - collection => Collection, - filter => Filter, - document => Doc, - password_hash_field => PasswordHashField - }), - ignore; - {error, Reason} -> - {error, Reason} - end + {error, Reason} -> + {error, Reason} 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 4efa62670..e95302ad4 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -113,36 +113,32 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - ?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; + 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} -> - ?TRACE_AUTHN_PROVIDER(error, "mysql_query_failed", #{ - resource => ResourceId, - tmpl_token => TmplToken, - params => Params, - timeout => Timeout, - reason => Reason - }), - ignore - end - end - ). + {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. 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 f8b47959a..2962308ab 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -115,35 +115,31 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - ?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; + 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} -> - ?TRACE_AUTHN_PROVIDER(error, "postgresql_query_failed", #{ - resource => ResourceId, - params => Params, - reason => Reason - }), - ignore - end - end - ). + {error, Reason} + end; + {error, Reason} -> + ?TRACE_AUTHN_PROVIDER(error, "postgresql_query_failed", #{ + resource => ResourceId, + params => Params, + reason => Reason + }), + ignore + 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 4cc00322f..fd2065f5a 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -133,37 +133,33 @@ authenticate( password_hash_algorithm := Algorithm } ) -> - ?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} -> - ignore - end; - {error, Reason} -> - ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ - resource => ResourceId, - cmd => Command, - keys => NKey, - fields => Fields, - reason => Reason - }), + 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} -> ignore - end - end - ). + end; + {error, Reason} -> + ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ + resource => ResourceId, + cmd => Command, + keys => NKey, + fields => Fields, + reason => Reason + }), + ignore + 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 db22a6cfe..b063bd833 100644 --- a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl @@ -166,6 +166,49 @@ test_user_auth(#{ ?GLOBAL ). +t_no_value_for_placeholder(_Config) -> + Handler = fun(Req0, State) -> + {ok, RawBody, Req1} = cowboy_req:read_body(Req0), + #{ + <<"cert_subject">> := <<"">>, + <<"cert_common_name">> := <<"">> + } = jiffy:decode(RawBody, [return_maps]), + Req = cowboy_req:reply( + 200, + #{<<"content-type">> => <<"application/json">>}, + jiffy:encode(#{result => allow, is_superuser => false}), + Req1 + ), + {ok, Req, State} + end, + + SpecificConfgParams = #{ + <<"method">> => <<"post">>, + <<"headers">> => #{<<"content-type">> => <<"application/json">>}, + <<"body">> => #{ + <<"cert_subject">> => ?PH_CERT_SUBJECT, + <<"cert_common_name">> => ?PH_CERT_CN_NAME + } + }, + + AuthConfig = maps:merge(raw_http_auth_config(), SpecificConfgParams), + + {ok, _} = emqx:update_config( + ?PATH, + {create_authenticator, ?GLOBAL, AuthConfig} + ), + + ok = emqx_authn_http_test_server:set_handler(Handler), + + Credentials = maps:without([cert_subject, cert_common_name], ?CREDENTIALS), + + ?assertMatch({ok, _}, emqx_access_control:authenticate(Credentials)), + + emqx_authn_test_lib:delete_authenticators( + [authentication], + ?GLOBAL + ). + t_destroy(_Config) -> AuthConfig = raw_http_auth_config(), @@ -247,27 +290,6 @@ 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( @@ -431,26 +453,6 @@ 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 ddde18c49..2f7dd2391 100644 --- a/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl @@ -288,20 +288,6 @@ 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 175aa7f1d..0fdba0b31 100644 --- a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl @@ -258,20 +258,6 @@ 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 02095c07d..ff017a79e 100644 --- a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl @@ -320,20 +320,6 @@ 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 f9ed8bcb1..410f529b3 100644 --- a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl @@ -292,20 +292,6 @@ 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.erl b/apps/emqx_authz/src/emqx_authz.erl index 3b175fc31..52f03fa90 100644 --- a/apps/emqx_authz/src/emqx_authz.erl +++ b/apps/emqx_authz/src/emqx_authz.erl @@ -391,14 +391,6 @@ 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 d364bc5fa..130b38d21 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) -> - error({cannot_get_variable, Name}); +handle_var({var, _Name}, undefined) -> + <<>>; handle_var({var, <<"peerhost">>}, IpAddr) -> inet_parse:ntoa(IpAddr); handle_var(_Name, Value) -> emqx_placeholder:bin(Value). -handle_sql_var({var, Name}, undefined) -> - error({cannot_get_variable, Name}); +handle_sql_var({var, _Name}, undefined) -> + <<>>; handle_sql_var({var, <<"peerhost">>}, IpAddr) -> inet_parse:ntoa(IpAddr); handle_sql_var(_Name, Value) -> diff --git a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl index 672fd6ddd..628e8dbfe 100644 --- a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl @@ -364,6 +364,46 @@ t_placeholder_and_body(_Config) -> emqx_access_control:authorize(ClientInfo, publish, <<"t">>) ). +t_no_value_for_placeholder(_Config) -> + ok = setup_handler_and_config( + fun(Req0, State) -> + ?assertEqual( + <<"/authz/users/">>, + cowboy_req:path(Req0) + ), + + {ok, RawBody, Req1} = cowboy_req:read_body(Req0), + + ?assertMatch( + #{ + <<"mountpoint">> := <<"[]">> + }, + jiffy:decode(RawBody, [return_maps]) + ), + {ok, ?AUTHZ_HTTP_RESP(allow, Req1), State} + end, + #{ + <<"method">> => <<"post">>, + <<"body">> => #{ + <<"mountpoint">> => <<"[${mountpoint}]">> + } + } + ), + + ClientInfo = #{ + clientid => <<"client id">>, + username => <<"user name">>, + peerhost => {127, 0, 0, 1}, + protocol => <<"MQTT">>, + zone => default, + listener => {tcp, default} + }, + + ?assertEqual( + allow, + emqx_access_control:authorize(ClientInfo, publish, <<"t">>) + ). + t_create_replace(_Config) -> ClientInfo = #{ clientid => <<"clientid">>,