You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by da...@apache.org on 2019/12/05 19:35:43 UTC
[couchdb] 10/11: Check security properties in the main transaction
This is an automated email from the ASF dual-hosted git repository.
davisp pushed a commit to branch opentracing-davisp
in repository https://gitbox.apache.org/repos/asf/couchdb.git
commit eea36ae715d0b6e654ecfba8655038d964394df5
Author: Nick Vatamaniuc <va...@apache.org>
AuthorDate: Tue Nov 5 10:52:34 2019 -0500
Check security properties in the main transaction
Previously we checked security properties in a separate transaction, after
opening the db or fetching it from the cache. To avoid running an extra
transaction move the check inside the main transaction right after the metadata
check runs. That ensure it will be consistent and it won't be accidentally
missed as all operations run the `ensure_current` metadata check.
Also remove the special `get_config/2` function in `fabric2_fdb` for getting
revs limit and security properties and just read them directly from the db map.
---
src/chttpd/src/chttpd_auth_request.erl | 5 +-
src/fabric/src/fabric2_db.erl | 67 ++++++++++++++++-----------
src/fabric/src/fabric2_fdb.erl | 39 +++++++---------
src/fabric/src/fabric2_server.erl | 3 +-
src/fabric/test/fabric2_db_security_tests.erl | 21 +++++----
5 files changed, 73 insertions(+), 62 deletions(-)
diff --git a/src/chttpd/src/chttpd_auth_request.erl b/src/chttpd/src/chttpd_auth_request.erl
index 7210905..26fee17 100644
--- a/src/chttpd/src/chttpd_auth_request.erl
+++ b/src/chttpd/src/chttpd_auth_request.erl
@@ -102,9 +102,8 @@ server_authorization_check(#httpd{method=Method, path_parts=[<<"_utils">>|_]}=Re
server_authorization_check(#httpd{path_parts=[<<"_", _/binary>>|_]}=Req) ->
require_admin(Req).
-db_authorization_check(#httpd{path_parts=[DbName|_],user_ctx=Ctx}=Req) ->
- {ok, Db} = fabric2_db:open(DbName, [{user_ctx, Ctx}]),
- fabric2_db:check_is_member(Db),
+db_authorization_check(#httpd{path_parts=[_DbName|_]}=Req) ->
+ % Db authorization checks are performed in fabric before every FDB operation
Req.
require_admin(Req) ->
diff --git a/src/fabric/src/fabric2_db.erl b/src/fabric/src/fabric2_db.erl
index 668d26d..ab122ed 100644
--- a/src/fabric/src/fabric2_db.erl
+++ b/src/fabric/src/fabric2_db.erl
@@ -176,19 +176,18 @@ open(DbName, Options) ->
case fabric2_server:fetch(DbName) of
#{} = Db ->
Db1 = maybe_set_user_ctx(Db, Options),
- ok = check_is_member(Db1),
- {ok, Db1};
+ {ok, require_member_check(Db1)};
undefined ->
Result = fabric2_fdb:transactional(DbName, Options, fun(TxDb) ->
fabric2_fdb:open(TxDb, Options)
end),
% Cache outside the transaction retry loop
case Result of
- #{security_doc := SecDoc} = Db0 ->
- ok = check_is_member(Db0, SecDoc),
+ #{} = Db0 ->
Db1 = maybe_add_sys_db_callbacks(Db0),
ok = fabric2_server:store(Db1),
- {ok, Db1#{tx := undefined}};
+ Db2 = Db1#{tx := undefined},
+ {ok, require_member_check(Db2)};
Error ->
Error
end
@@ -256,7 +255,11 @@ is_admin(Db, {SecProps}) when is_list(SecProps) ->
check_is_admin(Db) ->
- case is_admin(Db) of
+ check_is_admin(Db, get_security(Db)).
+
+
+check_is_admin(Db, SecDoc) ->
+ case is_admin(Db, SecDoc) of
true ->
ok;
false ->
@@ -280,6 +283,18 @@ check_is_member(Db, SecDoc) ->
end.
+require_admin_check(#{} = Db) ->
+ Db#{security_fun := fun check_is_admin/2}.
+
+
+require_member_check(#{} = Db) ->
+ Db#{security_fun := fun check_is_member/2}.
+
+
+no_security_check(#{} = Db) ->
+ Db#{security_fun := undefined}.
+
+
name(#{name := DbName}) ->
DbName.
@@ -359,21 +374,17 @@ get_pid(#{}) ->
get_revs_limit(#{} = Db) ->
- fabric2_fdb:transactional(Db, fun(TxDb) ->
- #{
- revs_limit := RevsLimit
- } = fabric2_fdb:ensure_current(TxDb),
- RevsLimit
- end).
+ #{revs_limit := RevsLimit} = fabric2_fdb:transactional(Db, fun(TxDb) ->
+ fabric2_fdb:ensure_current(TxDb)
+ end),
+ RevsLimit.
get_security(#{} = Db) ->
- fabric2_fdb:transactional(Db, fun(TxDb) ->
- #{
- security_doc := Security
- } = fabric2_fdb:ensure_current(TxDb),
- Security
- end).
+ #{security_doc := SecDoc} = fabric2_fdb:transactional(Db, fun(TxDb) ->
+ fabric2_fdb:ensure_current(no_security_check(TxDb))
+ end),
+ SecDoc.
get_update_seq(#{} = Db) ->
@@ -444,26 +455,26 @@ is_users_db(DbName) when is_binary(DbName) ->
IsAuthCache orelse IsCfgUsersDb orelse IsGlobalUsersDb.
-set_revs_limit(#{} = Db, RevsLimit) ->
- check_is_admin(Db),
+set_revs_limit(#{} = Db0, RevsLimit) ->
+ Db1 = require_admin_check(Db0),
RevsLimBin = ?uint2bin(max(1, RevsLimit)),
- Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
- fabric2_fdb:set_config(TxDb, <<"revs_limit">>, RevsLimBin)
+ {Resp, Db2} = fabric2_fdb:transactional(Db1, fun(TxDb) ->
+ {fabric2_fdb:set_config(TxDb, <<"revs_limit">>, RevsLimBin), TxDb}
end),
if Resp /= ok -> Resp; true ->
- fabric2_server:store(Db#{revs_limit := RevsLimit})
+ fabric2_server:store(Db2#{revs_limit := RevsLimit})
end.
-set_security(#{} = Db, Security) ->
- check_is_admin(Db),
+set_security(#{} = Db0, Security) ->
+ Db1 = require_admin_check(Db0),
ok = fabric2_util:validate_security_object(Security),
SecBin = ?JSON_ENCODE(Security),
- Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
- fabric2_fdb:set_config(TxDb, <<"security_doc">>, SecBin)
+ {Resp, Db2} = fabric2_fdb:transactional(Db1, fun(TxDb) ->
+ {fabric2_fdb:set_config(TxDb, <<"security_doc">>, SecBin), TxDb}
end),
if Resp /= ok -> Resp; true ->
- fabric2_server:store(Db#{security_doc := Security})
+ fabric2_server:store(Db2#{security_doc := Security})
end.
diff --git a/src/fabric/src/fabric2_fdb.erl b/src/fabric/src/fabric2_fdb.erl
index 2945ee9..3db0cde 100644
--- a/src/fabric/src/fabric2_fdb.erl
+++ b/src/fabric/src/fabric2_fdb.erl
@@ -22,7 +22,7 @@
create/2,
open/2,
- reopen/1,
+ ensure_current/1,
delete/1,
exists/1,
@@ -34,7 +34,6 @@
get_info/1,
get_config/1,
- get_config/2,
set_config/3,
get_stat/2,
@@ -323,10 +322,12 @@ reopen(#{} = OldDb) ->
tx := Tx,
name := DbName,
db_options := Options,
- user_ctx := UserCtx
+ user_ctx := UserCtx,
+ security_fun := SecurityFun
} = OldDb,
Options1 = lists:keystore(user_ctx, 1, Options, {user_ctx, UserCtx}),
- open(init_db(Tx, DbName, Options1), Options1).
+ NewDb = open(init_db(Tx, DbName, Options1), Options1),
+ NewDb#{security_fun := SecurityFun}.
delete(#{} = Db) ->
@@ -437,19 +438,6 @@ get_config(#{} = Db) ->
end, erlfdb:wait(Future)).
-get_config(#{} = Db, ConfigKey) ->
- #{
- tx := Tx,
- db_prefix := DbPrefix
- } = ensure_current(Db),
-
- Key = erlfdb_tuple:pack({?DB_CONFIG, ConfigKey}, DbPrefix),
- case erlfdb:wait(erlfdb:get(Tx, Key)) of
- % config values are expected to be set so we blow if not_found
- Val when Val =/= not_found -> Val
- end.
-
-
set_config(#{} = Db, ConfigKey, ConfigVal) ->
#{
tx := Tx,
@@ -912,6 +900,7 @@ init_db(Tx, DbName, Options) ->
layer_prefix => Prefix,
md_version => Version,
+ security_fun => undefined,
db_options => Options
}.
@@ -1353,12 +1342,20 @@ ensure_current(Db) ->
ensure_current(Db, true).
-ensure_current(#{} = Db, CheckDbVersion) ->
- require_transaction(Db),
- case check_metadata_version(Db) of
+ensure_current(#{} = Db0, CheckDbVersion) ->
+ require_transaction(Db0),
+ Db2 = case check_metadata_version(Db0) of
{current, Db1} -> Db1;
{stale, Db1} -> check_db_version(Db1, CheckDbVersion)
- end.
+ end,
+ case maps:get(security_fun, Db2) of
+ SecurityFun when is_function(SecurityFun, 2) ->
+ #{security_doc := SecDoc} = Db2,
+ ok = SecurityFun(Db2, SecDoc);
+ undefined ->
+ ok
+ end,
+ Db2.
is_transaction_applied(Tx) ->
diff --git a/src/fabric/src/fabric2_server.erl b/src/fabric/src/fabric2_server.erl
index b5e2ee9..866e338 100644
--- a/src/fabric/src/fabric2_server.erl
+++ b/src/fabric/src/fabric2_server.erl
@@ -59,7 +59,8 @@ fetch(DbName) when is_binary(DbName) ->
store(#{name := DbName} = Db0) when is_binary(DbName) ->
Db1 = Db0#{
tx := undefined,
- user_ctx := #user_ctx{}
+ user_ctx := #user_ctx{},
+ security_fun := undefined
},
true = ets:insert(?MODULE, {DbName, Db1}),
ok.
diff --git a/src/fabric/test/fabric2_db_security_tests.erl b/src/fabric/test/fabric2_db_security_tests.erl
index 4a54083..e5f3ad2 100644
--- a/src/fabric/test/fabric2_db_security_tests.erl
+++ b/src/fabric/test/fabric2_db_security_tests.erl
@@ -39,9 +39,9 @@ security_test_() ->
fun check_admin_is_member/1,
fun check_is_member_of_public_db/1,
fun check_set_user_ctx/1,
- fun check_open_forbidden/1,
- fun check_fail_open_no_opts/1,
- fun check_fail_open_name_null/1
+ fun check_forbidden/1,
+ fun check_fail_no_opts/1,
+ fun check_fail_name_null/1
]}
}
}.
@@ -165,18 +165,21 @@ check_set_user_ctx({Db0, _, _}) ->
?assertEqual(UserCtx, fabric2_db:get_user_ctx(Db1)).
-check_open_forbidden({Db0, _, _}) ->
+check_forbidden({Db0, _, _}) ->
DbName = fabric2_db:name(Db0),
UserCtx = #user_ctx{name = <<"foo">>, roles = [<<"bar">>]},
- ?assertThrow({forbidden, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).
+ {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+ ?assertThrow({forbidden, _}, fabric2_db:get_db_info(Db)).
-check_fail_open_no_opts({Db0, _, _}) ->
+check_fail_no_opts({Db0, _, _}) ->
DbName = fabric2_db:name(Db0),
- ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [])).
+ {ok, Db} = fabric2_db:open(DbName, []),
+ ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).
-check_fail_open_name_null({Db0, _, _}) ->
+check_fail_name_null({Db0, _, _}) ->
DbName = fabric2_db:name(Db0),
UserCtx = #user_ctx{name = null},
- ?assertThrow({unauthorized, _}, fabric2_db:open(DbName, [{user_ctx, UserCtx}])).
+ {ok, Db} = fabric2_db:open(DbName, [{user_ctx, UserCtx}]),
+ ?assertThrow({unauthorized, _}, fabric2_db:get_db_info(Db)).