fix(tpl): ensure backward compat with authz / authn templates

This commit leans heavy into discouraging the former approach where
only part of placeholders were interpolated, depending on `placeholders`
option.
This commit is contained in:
Andrew Mayorov 2023-04-27 13:53:17 +03:00
parent 49f5325c67
commit 49fba40ee7
No known key found for this signature in database
GPG Key ID: 2837C62ACFBFED5D
10 changed files with 260 additions and 87 deletions

View File

@ -18,6 +18,7 @@
-include_lib("emqx/include/emqx_placeholder.hrl").
-include_lib("emqx_authn.hrl").
-include_lib("snabbkaffe/include/trace.hrl").
-export([
create_resource/3,
@ -44,13 +45,13 @@
default_headers_no_content_type/0
]).
-define(AUTHN_PLACEHOLDERS, [
<<?VAR_USERNAME>>,
<<?VAR_CLIENTID>>,
<<?VAR_PASSWORD>>,
<<?VAR_PEERHOST>>,
<<?VAR_CERT_SUBJECT>>,
<<?VAR_CERT_CN_NAME>>
-define(ALLOWED_VARS, [
?VAR_USERNAME,
?VAR_CLIENTID,
?VAR_PASSWORD,
?VAR_PEERHOST,
?VAR_CERT_SUBJECT,
?VAR_CERT_CN_NAME
]).
-define(DEFAULT_RESOURCE_OPTS, #{
@ -108,21 +109,55 @@ check_password_from_selected_map(Algorithm, Selected, Password) ->
parse_deep(Template) ->
Result = emqx_connector_template:parse_deep(Template),
ok = emqx_connector_template:validate(?AUTHN_PLACEHOLDERS, Result),
Result.
handle_disallowed_placeholders(Result, {deep, Template}).
parse_str(Template) ->
Result = emqx_connector_template:parse(Template),
ok = emqx_connector_template:validate(?AUTHN_PLACEHOLDERS, Result),
Result.
handle_disallowed_placeholders(Result, {string, Template}).
parse_sql(Template, ReplaceWith) ->
{Statement, Result} = emqx_connector_template_sql:parse_prepstmt(
Template,
#{parameters => ReplaceWith, strip_double_quote => true}
),
ok = emqx_connector_template:validate(?AUTHN_PLACEHOLDERS, Result),
{Statement, Result}.
{Statement, handle_disallowed_placeholders(Result, {string, Template})}.
handle_disallowed_placeholders(Template, Source) ->
case emqx_connector_template:validate(?ALLOWED_VARS, Template) of
ok ->
Template;
{error, Disallowed} ->
?tp(warning, "authn_template_invalid", #{
template => Source,
reason => Disallowed,
allowed => #{placeholders => ?ALLOWED_VARS},
notice =>
"Disallowed placeholders will be rendered as is."
" However, consider using `$${...}` escaping for literal `${...}` where"
" needed to avoid unexpected results."
}),
Result = prerender_disallowed_placeholders(Template),
case Source of
{string, _} ->
emqx_connector_template:parse(Result);
{deep, _} ->
emqx_connector_template:parse_deep(Result)
end
end.
prerender_disallowed_placeholders(Template) ->
{Result, _} = emqx_connector_template:render(Template, #{}, #{
var_trans => fun(Name, _) ->
% NOTE
% Rendering disallowed placeholders in escaped form, which will then
% parse as a literal string.
case lists:member(Name, ?ALLOWED_VARS) of
true -> "${" ++ Name ++ "}";
false -> "$${" ++ Name ++ "}"
end
end
}),
Result.
render_deep(Template, Credential) ->
% NOTE
@ -130,7 +165,7 @@ render_deep(Template, Credential) ->
{Term, _Errors} = emqx_connector_template:render(
Template,
mapping_credential(Credential),
#{var_trans => fun handle_var/2}
#{var_trans => fun to_string/2}
),
Term.
@ -140,7 +175,7 @@ render_str(Template, Credential) ->
{String, _Errors} = emqx_connector_template:render(
Template,
mapping_credential(Credential),
#{var_trans => fun handle_var/2}
#{var_trans => fun to_string/2}
),
unicode:characters_to_binary(String).
@ -150,7 +185,7 @@ render_urlencoded_str(Template, Credential) ->
{String, _Errors} = emqx_connector_template:render(
Template,
mapping_credential(Credential),
#{var_trans => fun urlencode_var/2}
#{var_trans => fun to_urlencoded_string/2}
),
unicode:characters_to_binary(String).
@ -160,7 +195,7 @@ render_sql_params(ParamList, Credential) ->
{Row, _Errors} = emqx_connector_template:render(
ParamList,
mapping_credential(Credential),
#{var_trans => fun handle_sql_var/2}
#{var_trans => fun to_sql_valaue/2}
),
Row.
@ -283,22 +318,24 @@ without_password(Credential, [Name | Rest]) ->
without_password(Credential, Rest)
end.
urlencode_var(Var, Value) ->
emqx_http_lib:uri_encode(handle_var(Var, Value)).
to_urlencoded_string(Name, Value) ->
emqx_http_lib:uri_encode(to_string(Name, Value)).
handle_var(_, undefined) ->
<<>>;
handle_var([<<"peerhost">>], PeerHost) ->
emqx_connector_template:to_string(inet:ntoa(PeerHost));
handle_var(_, Value) ->
emqx_connector_template:to_string(Value).
to_string(Name, Value) ->
emqx_connector_template:to_string(render_var(Name, Value)).
handle_sql_var(_, undefined) ->
to_sql_valaue(Name, Value) ->
emqx_connector_sql:to_sql_value(render_var(Name, Value)).
render_var(_, undefined) ->
% NOTE
% Any allowed but undefined binding will be replaced with empty string, even when
% rendering SQL values.
<<>>;
handle_sql_var([<<"peerhost">>], PeerHost) ->
emqx_connector_sql:to_sql_value(inet:ntoa(PeerHost));
handle_sql_var(_, Value) ->
emqx_connector_sql:to_sql_value(Value).
render_var(?VAR_PEERHOST, Value) ->
inet:ntoa(Value);
render_var(_Name, Value) ->
Value.
mapping_credential(C = #{cn := CN, dn := DN}) ->
C#{cert_common_name => CN, cert_subject => DN};

View File

@ -183,8 +183,7 @@ compile_topic(<<"eq ", Topic/binary>>) ->
compile_topic({eq, Topic}) ->
{eq, emqx_topic:words(bin(Topic))};
compile_topic(Topic) ->
Template = emqx_connector_template:parse(Topic),
ok = emqx_connector_template:validate([?VAR_USERNAME, ?VAR_CLIENTID], Template),
Template = emqx_authz_utils:parse_str(Topic, [?VAR_USERNAME, ?VAR_CLIENTID]),
case emqx_connector_template:trivial(Template) of
true -> emqx_topic:words(bin(Topic));
false -> {pattern, Template}

View File

@ -16,7 +16,9 @@
-module(emqx_authz_utils).
-include_lib("emqx/include/emqx_placeholder.hrl").
-include_lib("emqx_authz.hrl").
-include_lib("snabbkaffe/include/trace.hrl").
-export([
cleanup_resources/0,
@ -109,21 +111,56 @@ update_config(Path, ConfigRequest) ->
parse_deep(Template, PlaceHolders) ->
Result = emqx_connector_template:parse_deep(Template),
ok = emqx_connector_template:validate(PlaceHolders, Result),
Result.
handle_disallowed_placeholders(Result, {deep, Template}, PlaceHolders).
parse_str(Template, PlaceHolders) ->
Result = emqx_connector_template:parse(Template),
ok = emqx_connector_template:validate(PlaceHolders, Result),
Result.
handle_disallowed_placeholders(Result, {string, Template}, PlaceHolders).
parse_sql(Template, ReplaceWith, PlaceHolders) ->
{Statement, Result} = emqx_connector_template_sql:parse_prepstmt(
Template,
#{parameters => ReplaceWith, strip_double_quote => true}
),
ok = emqx_connector_template:validate(PlaceHolders, Result),
{Statement, Result}.
FResult = handle_disallowed_placeholders(Result, {string, Template}, PlaceHolders),
{Statement, FResult}.
handle_disallowed_placeholders(Template, Source, Allowed) ->
case emqx_connector_template:validate(Allowed, Template) of
ok ->
Template;
{error, Disallowed} ->
?tp(warning, "authz_template_invalid", #{
template => Source,
reason => Disallowed,
allowed => #{placeholders => Allowed},
notice =>
"Disallowed placeholders will be rendered as is."
" However, consider using `$${...}` escaping for literal `${...}` where"
" needed to avoid unexpected results."
}),
Result = prerender_disallowed_placeholders(Template, Allowed),
case Source of
{string, _} ->
emqx_connector_template:parse(Result);
{deep, _} ->
emqx_connector_template:parse_deep(Result)
end
end.
prerender_disallowed_placeholders(Template, Allowed) ->
{Result, _} = emqx_connector_template:render(Template, #{}, #{
var_trans => fun(Name, _) ->
% NOTE
% Rendering disallowed placeholders in escaped form, which will then
% parse as a literal string.
case lists:member(Name, Allowed) of
true -> "${" ++ Name ++ "}";
false -> "$${" ++ Name ++ "}"
end
end
}),
Result.
render_deep(Template, Values) ->
% NOTE
@ -131,7 +168,7 @@ render_deep(Template, Values) ->
{Term, _Errors} = emqx_connector_template:render(
Template,
client_vars(Values),
#{var_trans => fun handle_var/2}
#{var_trans => fun to_string/2}
),
Term.
@ -141,7 +178,7 @@ render_str(Template, Values) ->
{String, _Errors} = emqx_connector_template:render(
Template,
client_vars(Values),
#{var_trans => fun handle_var/2}
#{var_trans => fun to_string/2}
),
unicode:characters_to_binary(String).
@ -151,7 +188,7 @@ render_urlencoded_str(Template, Values) ->
{String, _Errors} = emqx_connector_template:render(
Template,
client_vars(Values),
#{var_trans => fun urlencode_var/2}
#{var_trans => fun to_urlencoded_string/2}
),
unicode:characters_to_binary(String).
@ -161,7 +198,7 @@ render_sql_params(ParamList, Values) ->
{Row, _Errors} = emqx_connector_template:render(
ParamList,
client_vars(Values),
#{var_trans => fun handle_sql_var/2}
#{var_trans => fun to_sql_value/2}
),
Row.
@ -229,22 +266,24 @@ convert_client_var({dn, DN}) -> {cert_subject, DN};
convert_client_var({protocol, Proto}) -> {proto_name, Proto};
convert_client_var(Other) -> Other.
urlencode_var(Var, Value) ->
emqx_http_lib:uri_encode(handle_var(Var, Value)).
to_urlencoded_string(Name, Value) ->
emqx_http_lib:uri_encode(to_string(Name, Value)).
handle_var(_, undefined) ->
<<>>;
handle_var([<<"peerhost">>], IpAddr) ->
inet_parse:ntoa(IpAddr);
handle_var(_Name, Value) ->
emqx_connector_template:to_string(Value).
to_string(Name, Value) ->
emqx_connector_template:to_string(render_var(Name, Value)).
handle_sql_var(_, undefined) ->
to_sql_value(Name, Value) ->
emqx_connector_sql:to_sql_value(render_var(Name, Value)).
render_var(_, undefined) ->
% NOTE
% Any allowed but undefined binding will be replaced with empty string, even when
% rendering SQL values.
<<>>;
handle_sql_var([<<"peerhost">>], IpAddr) ->
inet_parse:ntoa(IpAddr);
handle_sql_var(_Name, Value) ->
emqx_connector_sql:to_sql_value(Value).
render_var(?VAR_PEERHOST, Value) ->
inet:ntoa(Value);
render_var(_Name, Value) ->
Value.
bin(A) when is_atom(A) -> atom_to_binary(A, utf8);
bin(L) when is_list(L) -> list_to_binary(L);

View File

@ -38,7 +38,7 @@
-compile(nowarn_export_all).
-endif.
-define(PLACEHOLDERS, [
-define(ALLOWED_VARS, [
?VAR_USERNAME,
?VAR_CLIENTID,
?VAR_PEERHOST,
@ -50,9 +50,9 @@
?VAR_CERT_CN_NAME
]).
-define(PLACEHOLDERS_FOR_RICH_ACTIONS, [
<<?VAR_QOS>>,
<<?VAR_RETAIN>>
-define(ALLOWED_VARS_RICH_ACTIONS, [
?VAR_QOS,
?VAR_RETAIN
]).
description() ->
@ -157,14 +157,14 @@ parse_config(
method => Method,
base_url => BaseUrl,
headers => Headers,
base_path_templete => emqx_authz_utils:parse_str(Path, placeholders()),
base_path_templete => emqx_authz_utils:parse_str(Path, allowed_vars()),
base_query_template => emqx_authz_utils:parse_deep(
cow_qs:parse_qs(to_bin(Query)),
placeholders()
allowed_vars()
),
body_template => emqx_authz_utils:parse_deep(
maps:to_list(maps:get(body, Conf, #{})),
placeholders()
allowed_vars()
),
request_timeout => ReqTimeout,
%% pool_type default value `random`
@ -260,10 +260,10 @@ to_bin(B) when is_binary(B) -> B;
to_bin(L) when is_list(L) -> list_to_binary(L);
to_bin(X) -> X.
placeholders() ->
placeholders(emqx_authz:feature_available(rich_actions)).
allowed_vars() ->
allowed_vars(emqx_authz:feature_available(rich_actions)).
placeholders(true) ->
?PLACEHOLDERS ++ ?PLACEHOLDERS_FOR_RICH_ACTIONS;
placeholders(false) ->
?PLACEHOLDERS.
allowed_vars(true) ->
?ALLOWED_VARS ++ ?ALLOWED_VARS_RICH_ACTIONS;
allowed_vars(false) ->
?ALLOWED_VARS.

View File

@ -27,7 +27,7 @@
-define(PATH, [?CONF_NS_ATOM]).
-define(HTTP_PORT, 32333).
-define(HTTP_PATH, "/auth").
-define(HTTP_PATH, "/auth/[...]").
-define(CREDENTIALS, #{
clientid => <<"clienta">>,
username => <<"plain">>,
@ -146,8 +146,12 @@ t_authenticate(_Config) ->
test_user_auth(#{
handler := Handler,
config_params := SpecificConfgParams,
result := Result
result := Expect
}) ->
Result = perform_user_auth(SpecificConfgParams, Handler, ?CREDENTIALS),
?assertEqual(Expect, Result).
perform_user_auth(SpecificConfgParams, Handler, Credentials) ->
AuthConfig = maps:merge(raw_http_auth_config(), SpecificConfgParams),
{ok, _} = emqx:update_config(
@ -157,21 +161,21 @@ test_user_auth(#{
ok = emqx_authn_http_test_server:set_handler(Handler),
?assertEqual(Result, emqx_access_control:authenticate(?CREDENTIALS)),
Result = emqx_access_control:authenticate(Credentials),
emqx_authn_test_lib:delete_authenticators(
[authentication],
?GLOBAL
).
),
Result.
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//">> ->
<<"/auth/p%20ath//us%20er/auth//">> ->
cowboy_req:reply(
200,
#{<<"content-type">> => <<"application/json">>},
@ -193,7 +197,7 @@ t_authenticate_path_placeholders(_Config) ->
AuthConfig = maps:merge(
raw_http_auth_config(),
#{
<<"url">> => <<"http://127.0.0.1:32333/my/p%20ath//${username}/auth//">>,
<<"url">> => <<"http://127.0.0.1:32333/auth/p%20ath//${username}/auth//">>,
<<"body">> => #{}
}
),
@ -255,6 +259,39 @@ t_no_value_for_placeholder(_Config) ->
?GLOBAL
).
t_disallowed_placeholders_preserved(_Config) ->
Config = #{
<<"method">> => <<"post">>,
<<"headers">> => #{<<"content-type">> => <<"application/json">>},
<<"body">> => #{
<<"username">> => ?PH_USERNAME,
<<"password">> => ?PH_PASSWORD,
<<"this">> => <<"${whatisthis}">>
}
},
Handler = fun(Req0, State) ->
{ok, Body, Req1} = cowboy_req:read_body(Req0),
#{
<<"username">> := <<"plain">>,
<<"password">> := <<"plain">>,
<<"this">> := <<"${whatisthis}">>
} = emqx_utils_json:decode(Body),
Req = cowboy_req:reply(
200,
#{<<"content-type">> => <<"application/json">>},
emqx_utils_json:encode(#{result => allow, is_superuser => false}),
Req1
),
{ok, Req, State}
end,
?assertMatch({ok, _}, perform_user_auth(Config, Handler, ?CREDENTIALS)),
% NOTE: disallowed placeholder left intact, which makes the URL invalid
ConfigUrl = Config#{
<<"url">> => <<"http://127.0.0.1:32333/auth/${whatisthis}">>
},
?assertMatch({error, _}, perform_user_auth(ConfigUrl, Handler, ?CREDENTIALS)).
t_destroy(_Config) ->
AuthConfig = raw_http_auth_config(),

View File

@ -494,6 +494,67 @@ t_no_value_for_placeholder(_Config) ->
emqx_access_control:authorize(ClientInfo, ?AUTHZ_PUBLISH, <<"t">>)
).
t_disallowed_placeholders_preserved(_Config) ->
ok = setup_handler_and_config(
fun(Req0, State) ->
{ok, Body, Req1} = cowboy_req:read_body(Req0),
?assertMatch(
#{
<<"cname">> := <<>>,
<<"usertypo">> := <<"${usertypo}">>
},
emqx_utils_json:decode(Body)
),
{ok, ?AUTHZ_HTTP_RESP(allow, Req1), State}
end,
#{
<<"method">> => <<"post">>,
<<"body">> => #{
<<"cname">> => ?PH_CERT_CN_NAME,
<<"usertypo">> => <<"${usertypo}">>
}
}
),
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, ?AUTHZ_PUBLISH, <<"t">>)
).
t_disallowed_placeholders_path(_Config) ->
ok = setup_handler_and_config(
fun(Req, State) ->
{ok, ?AUTHZ_HTTP_RESP(allow, Req), State}
end,
#{
<<"url">> => <<"http://127.0.0.1:33333/authz/use%20rs/${typo}">>
}
),
ClientInfo = #{
clientid => <<"client id">>,
username => <<"user name">>,
peerhost => {127, 0, 0, 1},
protocol => <<"MQTT">>,
zone => default,
listener => {tcp, default}
},
% % NOTE: disallowed placeholder left intact, which makes the URL invalid
?assertEqual(
deny,
emqx_access_control:authorize(ClientInfo, ?AUTHZ_PUBLISH, <<"t">>)
).
t_create_replace(_Config) ->
ClientInfo = #{
clientid => <<"clientid">>,

View File

@ -35,7 +35,7 @@
-compile(nowarn_export_all).
-endif.
-define(PLACEHOLDERS, [
-define(ALLOWED_VARS, [
?VAR_USERNAME,
?VAR_CLIENTID,
?VAR_PEERHOST,
@ -49,11 +49,11 @@ description() ->
create(#{filter := Filter} = Source) ->
ResourceId = emqx_authz_utils:make_resource_id(?MODULE),
{ok, _Data} = emqx_authz_utils:create_resource(ResourceId, emqx_mongodb, Source),
FilterTemp = emqx_authz_utils:parse_deep(Filter, ?PLACEHOLDERS),
FilterTemp = emqx_authz_utils:parse_deep(Filter, ?ALLOWED_VARS),
Source#{annotations => #{id => ResourceId}, filter_template => FilterTemp}.
update(#{filter := Filter} = Source) ->
FilterTemp = emqx_authz_utils:parse_deep(Filter, ?PLACEHOLDERS),
FilterTemp = emqx_authz_utils:parse_deep(Filter, ?ALLOWED_VARS),
case emqx_authz_utils:update_resource(emqx_mongodb, Source) of
{error, Reason} ->
error({load_config_error, Reason});

View File

@ -37,7 +37,7 @@
-compile(nowarn_export_all).
-endif.
-define(PLACEHOLDERS, [
-define(ALLOWED_VARS, [
?VAR_USERNAME,
?VAR_CLIENTID,
?VAR_PEERHOST,
@ -49,14 +49,14 @@ description() ->
"AuthZ with Mysql".
create(#{query := SQL} = Source0) ->
{PrepareSQL, TmplToken} = emqx_authz_utils:parse_sql(SQL, '?', ?PLACEHOLDERS),
{PrepareSQL, TmplToken} = emqx_authz_utils:parse_sql(SQL, '?', ?ALLOWED_VARS),
ResourceId = emqx_authz_utils:make_resource_id(?MODULE),
Source = Source0#{prepare_statement => #{?PREPARE_KEY => PrepareSQL}},
{ok, _Data} = emqx_authz_utils:create_resource(ResourceId, emqx_mysql, Source),
Source#{annotations => #{id => ResourceId, tmpl_token => TmplToken}}.
update(#{query := SQL} = Source0) ->
{PrepareSQL, TmplToken} = emqx_authz_utils:parse_sql(SQL, '?', ?PLACEHOLDERS),
{PrepareSQL, TmplToken} = emqx_authz_utils:parse_sql(SQL, '?', ?ALLOWED_VARS),
Source = Source0#{prepare_statement => #{?PREPARE_KEY => PrepareSQL}},
case emqx_authz_utils:update_resource(emqx_mysql, Source) of
{error, Reason} ->

View File

@ -37,7 +37,7 @@
-compile(nowarn_export_all).
-endif.
-define(PLACEHOLDERS, [
-define(ALLOWED_VARS, [
?VAR_USERNAME,
?VAR_CLIENTID,
?VAR_PEERHOST,
@ -49,7 +49,7 @@ description() ->
"AuthZ with PostgreSQL".
create(#{query := SQL0} = Source) ->
{SQL, PlaceHolders} = emqx_authz_utils:parse_sql(SQL0, '$n', ?PLACEHOLDERS),
{SQL, PlaceHolders} = emqx_authz_utils:parse_sql(SQL0, '$n', ?ALLOWED_VARS),
ResourceID = emqx_authz_utils:make_resource_id(emqx_postgresql),
{ok, _Data} = emqx_authz_utils:create_resource(
ResourceID,
@ -59,7 +59,7 @@ create(#{query := SQL0} = Source) ->
Source#{annotations => #{id => ResourceID, placeholders => PlaceHolders}}.
update(#{query := SQL0, annotations := #{id := ResourceID}} = Source) ->
{SQL, PlaceHolders} = emqx_authz_utils:parse_sql(SQL0, '$n', ?PLACEHOLDERS),
{SQL, PlaceHolders} = emqx_authz_utils:parse_sql(SQL0, '$n', ?ALLOWED_VARS),
case
emqx_authz_utils:update_resource(
emqx_postgresql,

View File

@ -35,7 +35,7 @@
-compile(nowarn_export_all).
-endif.
-define(PLACEHOLDERS, [
-define(ALLOWED_VARS, [
?VAR_CERT_CN_NAME,
?VAR_CERT_SUBJECT,
?VAR_PEERHOST,
@ -133,7 +133,7 @@ parse_cmd(Query) ->
case emqx_redis_command:split(Query) of
{ok, Cmd} ->
ok = validate_cmd(Cmd),
emqx_authz_utils:parse_deep(Cmd, ?PLACEHOLDERS);
emqx_authz_utils:parse_deep(Cmd, ?ALLOWED_VARS);
{error, Reason} ->
error({invalid_redis_cmd, Reason, Query})
end.