From 2ffcaad41e47c61b24cdd49b9df4f9b519ff5e3b Mon Sep 17 00:00:00 2001 From: Ilya Averyanov Date: Thu, 30 Dec 2021 23:57:52 +0300 Subject: [PATCH] chore(authz): increase coverage --- apps/emqx/src/emqx_tls_lib.erl | 10 ++- .../emqx_authz/src/emqx_authz_api_sources.erl | 3 +- apps/emqx_authz/src/emqx_authz_http.erl | 17 +--- apps/emqx_authz/src/emqx_authz_mongodb.erl | 15 +++- apps/emqx_authz/src/emqx_authz_schema.erl | 34 -------- apps/emqx_authz/test/emqx_authz_SUITE.erl | 10 +++ .../test/emqx_authz_api_sources_SUITE.erl | 87 ++++++++++++++----- .../emqx_authz/test/emqx_authz_http_SUITE.erl | 8 +- .../test/emqx_authz_mongodb_SUITE.erl | 18 ++++ 9 files changed, 118 insertions(+), 84 deletions(-) diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index ec339a968..fa9c996e9 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -34,6 +34,9 @@ -include("logger.hrl"). +-define(IS_TRUE(Val), ((Val =:= true) or (Val =:= <<"true">>))). +-define(IS_FALSE(Val), ((Val =:= false) or (Val =:= <<"false">>))). + -define(SSL_FILE_OPT_NAMES, [<<"keyfile">>, <<"certfile">>, <<"cacertfile">>]). %% non-empty string @@ -235,7 +238,8 @@ ensure_ssl_files(Dir, Opts) -> ensure_ssl_files(Dir, Opts, _DryRun = false). ensure_ssl_files(_Dir, undefined, _DryRun) -> {ok, undefined}; -ensure_ssl_files(_Dir, #{<<"enable">> := false} = Opts, _DryRun) -> {ok, Opts}; +ensure_ssl_files(_Dir, #{<<"enable">> := False} = Opts, _DryRun) when ?IS_FALSE(False) -> + {ok, Opts}; ensure_ssl_files(Dir, Opts, DryRun) -> ensure_ssl_files(Dir, Opts, ?SSL_FILE_OPT_NAMES, DryRun). @@ -352,9 +356,9 @@ is_valid_pem_file(Path) -> %% @doc This is to return SSL file content in management APIs. file_content_as_options(undefined) -> undefined; -file_content_as_options(#{<<"enable">> := false} = SSL) -> +file_content_as_options(#{<<"enable">> := False} = SSL) when ?IS_FALSE(False) -> {ok, maps:without(?SSL_FILE_OPT_NAMES, SSL)}; -file_content_as_options(#{<<"enable">> := true} = SSL) -> +file_content_as_options(#{<<"enable">> := True} = SSL) when ?IS_TRUE(True) -> file_content_as_options(?SSL_FILE_OPT_NAMES, SSL). file_content_as_options([], SSL) -> diff --git a/apps/emqx_authz/src/emqx_authz_api_sources.erl b/apps/emqx_authz/src/emqx_authz_api_sources.erl index df5a6c819..578e05e24 100644 --- a/apps/emqx_authz/src/emqx_authz_api_sources.erl +++ b/apps/emqx_authz/src/emqx_authz_api_sources.erl @@ -445,7 +445,8 @@ read_certs(#{<<"ssl">> := SSL} = Source) -> end; read_certs(Source) -> Source. -maybe_write_certs(#{<<"ssl">> := #{<<"enable">> := true} = SSL} = Source) -> +maybe_write_certs(#{<<"ssl">> := #{<<"enable">> := True} = SSL} = Source) + when (True =:= true) or (True =:= <<"true">>) -> Type = maps:get(<<"type">>, Source), {ok, Return} = emqx_tls_lib:ensure_ssl_files(filename:join(["authz", Type]), SSL), maps:put(<<"ssl">>, Return, Source); diff --git a/apps/emqx_authz/src/emqx_authz_http.erl b/apps/emqx_authz/src/emqx_authz_http.erl index d677704c4..222595f8b 100644 --- a/apps/emqx_authz/src/emqx_authz_http.erl +++ b/apps/emqx_authz/src/emqx_authz_http.erl @@ -29,7 +29,6 @@ , destroy/1 , dry_run/1 , authorize/4 - , parse_url/1 ]). -ifdef(TEST). @@ -113,18 +112,6 @@ authorize(Client, PubSub, Topic, ignore end. -parse_url(URL) - when URL =:= undefined -> - #{}; -parse_url(URL) -> - {ok, URIMap} = emqx_http_lib:uri_parse(URL), - case maps:get(query, URIMap, undefined) of - undefined -> - URIMap#{query => ""}; - _ -> - URIMap - end. - query_string(Body) -> query_string(maps:to_list(Body), []). @@ -147,13 +134,13 @@ replvar_deep(Map, PubSub, Topic, Vars, VarEncode) when is_map(Map) -> lists:map( fun({Key, Value}) -> {replvar(Key, PubSub, Topic, Vars, VarEncode), - replvar(Value, PubSub, Topic, Vars, VarEncode)} + replvar_deep(Value, PubSub, Topic, Vars, VarEncode)} end, maps:to_list(Map))); replvar_deep(List, PubSub, Topic, Vars, VarEncode) when is_list(List) -> lists:map( fun(Value) -> - replvar(Value, PubSub, Topic, Vars, VarEncode) + replvar_deep(Value, PubSub, Topic, Vars, VarEncode) end, List); replvar_deep(Number, _PubSub, _Topic, _Vars, _VarEncode) when is_number(Number) -> diff --git a/apps/emqx_authz/src/emqx_authz_mongodb.erl b/apps/emqx_authz/src/emqx_authz_mongodb.erl index 97fac9627..62bd54314 100644 --- a/apps/emqx_authz/src/emqx_authz_mongodb.erl +++ b/apps/emqx_authz/src/emqx_authz_mongodb.erl @@ -56,7 +56,14 @@ authorize(Client, PubSub, Topic, selector := Selector, annotations := #{id := ResourceID} }) -> - case emqx_resource:query(ResourceID, {find, Collection, replvar(Selector, Client), #{}}) of + RenderedSelector = replvar(Selector, Client), + Result = try + emqx_resource:query(ResourceID, {find, Collection, RenderedSelector, #{}}) + catch + error:Error -> {error, Error} + end, + + case Result of {error, Reason} -> ?SLOG(error, #{msg => "query_mongo_error", reason => Reason, @@ -65,9 +72,9 @@ authorize(Client, PubSub, Topic, [] -> nomatch; Rows -> Rules = [ emqx_authz_rule:compile({Permission, all, Action, Topics}) - || #{<<"topics">> := Topics, - <<"permission">> := Permission, - <<"action">> := Action} <- Rows], + || #{<<"topics">> := Topics, + <<"permission">> := Permission, + <<"action">> := Action} <- Rows], do_authorize(Client, PubSub, Topic, Rules) end. diff --git a/apps/emqx_authz/src/emqx_authz_schema.erl b/apps/emqx_authz/src/emqx_authz_schema.erl index 8415e3710..64b82c39d 100644 --- a/apps/emqx_authz/src/emqx_authz_schema.erl +++ b/apps/emqx_authz/src/emqx_authz_schema.erl @@ -28,7 +28,6 @@ -export([ namespace/0 , roots/0 , fields/1 - , validations/0 ]). -import(emqx_schema, [mk_duration/2]). @@ -153,11 +152,6 @@ mongo_common_fields() -> default => true}} ]. -validations() -> - [ {check_ssl_opts, fun check_ssl_opts/1} - , {check_headers, fun check_headers/1} - ]. - headers(type) -> map(); headers(converter) -> fun(Headers) -> @@ -196,28 +190,6 @@ transform_header_name(Headers) -> maps:put(K, V, Acc) end, #{}, Headers). -check_ssl_opts(Conf) - when Conf =:= #{} -> - true; -check_ssl_opts(Conf) -> - case emqx_authz_http:parse_url(hocon_schema:get_value("config.base_url", Conf)) of - #{scheme := https} -> - case hocon_schema:get_value("config.ssl.enable", Conf) of - true -> ok; - false -> false - end; - #{scheme := http} -> - ok - end. - -check_headers(Conf) - when Conf =:= #{} -> - true; -check_headers(Conf) -> - Method = to_bin(hocon_schema:get_value("config.method", Conf)), - Headers = hocon_schema:get_value("config.headers", Conf), - Method =:= <<"post">> orelse (not maps:is_key(<<"content-type">>, Headers)). - union_array(Item) when is_list(Item) -> hoconsc:array(hoconsc:union(Item)). @@ -253,9 +225,3 @@ to_list(A) when is_atom(A) -> to_list(B) when is_binary(B) -> binary_to_list(B). -to_bin(A) when is_atom(A) -> - atom_to_binary(A); -to_bin(B) when is_binary(B) -> - B; -to_bin(L) when is_list(L) -> - list_to_binary(L). diff --git a/apps/emqx_authz/test/emqx_authz_SUITE.erl b/apps/emqx_authz/test/emqx_authz_SUITE.erl index 942f74abc..70222cfe3 100644 --- a/apps/emqx_authz/test/emqx_authz_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_SUITE.erl @@ -164,6 +164,16 @@ t_update_source(_) -> {ok, _} = emqx_authz:update(?CMD_REPLACE, []). +t_delete_source(_) -> + {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE1]), + + ?assertMatch([ #{type := http, enable := true} + ], emqx_conf:get([authorization, sources], [])), + + {ok, _} = emqx_authz:update({?CMD_DELETE, http}, #{}), + + ?assertMatch([], emqx_conf:get([authorization, sources], [])). + t_move_source(_) -> {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE1, ?SOURCE2, ?SOURCE3, diff --git a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl index 67e1b05da..3a7284c48 100644 --- a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl @@ -29,7 +29,9 @@ -define(SOURCE1, #{<<"type">> => <<"http">>, <<"enable">> => true, - <<"url">> => <<"https://fake.com:443/">>, + <<"base_url">> => <<"https://fake.com:443/">>, + <<"path">> => <<"foo">>, + <<"query">> => <<"a=b">>, <<"headers">> => #{}, <<"method">> => <<"get">>, <<"request_timeout">> => <<"5s">> @@ -37,9 +39,7 @@ -define(SOURCE2, #{<<"type">> => <<"mongodb">>, <<"enable">> => true, <<"mongo_type">> => <<"sharded">>, - <<"servers">> => [<<"127.0.0.1:27017">>, - <<"192.168.0.1:27017">> - ], + <<"servers">> => <<"127.0.0.1:27017,192.168.0.1:27017">>, <<"pool_size">> => 1, <<"database">> => <<"mqtt">>, <<"ssl">> => #{<<"enable">> => false}, @@ -87,9 +87,11 @@ "\n{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}.">> }). +-define(MATCH_RSA_KEY, <<"-----BEGIN RSA PRIVATE KEY", _/binary>>). +-define(MATCH_CERT, <<"-----BEGIN CERTIFICATE", _/binary>>). + all() -> - []. %% Todo: Waiting for @terry-xiaoyu to fix the config_not_found error - % emqx_common_test_helpers:all(?MODULE). + emqx_common_test_helpers:all(?MODULE). groups() -> []. @@ -98,7 +100,7 @@ init_per_suite(Config) -> meck:new(emqx_resource, [non_strict, passthrough, no_history, no_link]), meck:expect(emqx_resource, create, fun(_, _, _) -> {ok, meck_data} end), meck:expect(emqx_resource, create_dry_run, - fun(emqx_connector_mysql, _) -> {ok, meck_data}; + fun(emqx_connector_mysql, _) -> ok; (T, C) -> meck:passthrough([T, C]) end), meck:expect(emqx_resource, update, fun(_, _, _, _) -> {ok, meck_data} end), @@ -186,26 +188,65 @@ t_api(_) -> {ok, 200, Result3} = request(get, uri(["authorization", "sources", "http"]), []), ?assertMatch(#{<<"type">> := <<"http">>, <<"enable">> := false}, jsx:decode(Result3)), + Keyfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "key.pem"])), + Certfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cert.pem"])), + Cacertfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cacert.pem"])), + {ok, 204, _} = request(put, uri(["authorization", "sources", "mongodb"]), - ?SOURCE2#{<<"ssl">> := #{ - <<"enable">> => true, - <<"cacertfile">> => <<"fake cacert file">>, - <<"certfile">> => <<"fake cert file">>, - <<"keyfile">> => <<"fake key file">>, - <<"verify">> => false + ?SOURCE2#{<<"ssl">> => #{ + <<"enable">> => <<"true">>, + <<"cacertfile">> => Cacertfile, + <<"certfile">> => Certfile, + <<"keyfile">> => Keyfile, + <<"verify">> => <<"verify_none">> }}), {ok, 200, Result4} = request(get, uri(["authorization", "sources", "mongodb"]), []), ?assertMatch(#{<<"type">> := <<"mongodb">>, - <<"ssl">> := #{<<"enable">> := true, - <<"cacertfile">> := <<"fake cacert file">>, - <<"certfile">> := <<"fake cert file">>, - <<"keyfile">> := <<"fake key file">>, - <<"verify">> := false + <<"ssl">> := #{<<"enable">> := <<"true">>, + <<"cacertfile">> := ?MATCH_CERT, + <<"certfile">> := ?MATCH_CERT, + <<"keyfile">> := ?MATCH_RSA_KEY, + <<"verify">> := <<"verify_none">> } }, jsx:decode(Result4)), - ?assert(filelib:is_file(filename:join([data_dir(), "certs", "cacert-fake.pem"]))), - ?assert(filelib:is_file(filename:join([data_dir(), "certs", "cert-fake.pem"]))), - ?assert(filelib:is_file(filename:join([data_dir(), "certs", "key-fake.pem"]))), + + {ok, Cacert} = file:read_file(Cacertfile), + {ok, Cert} = file:read_file(Certfile), + {ok, Key} = file:read_file(Keyfile), + + {ok, 204, _} = request(put, uri(["authorization", "sources", "mongodb"]), + ?SOURCE2#{<<"ssl">> => #{ + <<"enable">> => <<"true">>, + <<"cacertfile">> => Cacert, + <<"certfile">> => Cert, + <<"keyfile">> => Key, + <<"verify">> => <<"verify_none">> + }}), + {ok, 200, Result5} = request(get, uri(["authorization", "sources", "mongodb"]), []), + ?assertMatch(#{<<"type">> := <<"mongodb">>, + <<"ssl">> := #{<<"enable">> := <<"true">>, + <<"cacertfile">> := ?MATCH_CERT, + <<"certfile">> := ?MATCH_CERT, + <<"keyfile">> := ?MATCH_RSA_KEY, + <<"verify">> := <<"verify_none">> + } + }, jsx:decode(Result5)), + + + #{ssl := #{cacertfile := SavedCacertfile, + certfile := SavedCertfile, + keyfile := SavedKeyfile + }} = emqx_authz:lookup(mongodb), + + ?assert(filelib:is_file(SavedCacertfile)), + ?assert(filelib:is_file(SavedCertfile)), + ?assert(filelib:is_file(SavedKeyfile)), {ok, 204, _} = request( put, @@ -229,8 +270,8 @@ t_api(_) -> uri(["authorization", "sources", binary_to_list(Type)]), []) end, Sources), - {ok, 200, Result5} = request(get, uri(["authorization", "sources"]), []), - ?assertEqual([], get_sources(Result5)), + {ok, 200, Result6} = request(get, uri(["authorization", "sources"]), []), + ?assertEqual([], get_sources(Result6)), ?assertEqual([], emqx:get_config([authorization, sources])), ok. diff --git a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl index 4e303bed7..2187fba73 100644 --- a/apps/emqx_authz/test/emqx_authz_http_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_http_SUITE.erl @@ -233,8 +233,8 @@ t_json_body(_Config) -> ?assertMatch( #{<<"username">> := <<"user name">>, <<"CLIENT_client id">> := <<"client id">>, - <<"peerhost">> := <<"127.0.0.1">>, - <<"proto_name">> := <<"MQTT">>, + <<"peerhost">> := [<<"127.0.0.1">>, 1], + <<"proto_name">> := #{<<"proto">> := <<"MQTT">>}, <<"mountpoint">> := <<"MOUNTPOINT">>, <<"topic">> := <<"t">>, <<"action">> := <<"publish">>}, @@ -253,8 +253,8 @@ t_json_body(_Config) -> "action/${action}">>, <<"body">> => #{<<"username">> => <<"${username}">>, <<"CLIENT_${clientid}">> => <<"${clientid}">>, - <<"peerhost">> => <<"${peerhost}">>, - <<"proto_name">> => <<"${proto_name}">>, + <<"peerhost">> => [<<"${peerhost}">>, 1], + <<"proto_name">> => #{<<"proto">> => <<"${proto_name}">>}, <<"mountpoint">> => <<"${mountpoint}">>, <<"topic">> => <<"${topic}">>, <<"action">> => <<"${action}">>} diff --git a/apps/emqx_authz/test/emqx_authz_mongodb_SUITE.erl b/apps/emqx_authz/test/emqx_authz_mongodb_SUITE.erl index 679b58c8e..f010fbd81 100644 --- a/apps/emqx_authz/test/emqx_authz_mongodb_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_mongodb_SUITE.erl @@ -167,6 +167,24 @@ t_lookups(_Config) -> [{allow, subscribe, <<"a">>}, {deny, subscribe, <<"b">>}]). +t_bad_selector(_Config) -> + ClientInfo = #{clientid => <<"clientid">>, + cn => <<"cn">>, + dn => <<"dn">>, + username => <<"username">>, + peerhost => {127,0,0,1}, + zone => default, + listener => {tcp, default} + }, + + ok = setup_config( + #{<<"selector">> => #{<<"$in">> => #{<<"a">> => 1}}}), + + ok = emqx_authz_test_lib:test_samples( + ClientInfo, + [{deny, subscribe, <<"a">>}, + {deny, subscribe, <<"b">>}]). + %%------------------------------------------------------------------------------ %% Helpers %%------------------------------------------------------------------------------