* [Tarantool-patches] [PATCH 0/2] Admin universe access @ 2019-11-01 21:42 Vladislav Shpilevoy 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-11-01 21:42 UTC (permalink / raw) To: tarantool-patches, kostja.osipov The patchset makes so the admin user never can loose its universe access rights. Apparently, Tarantool can't even bootstrap nor recovery without universe granted to the admin, because this user owns the fibers doing recovery and bootstrap. First patch fixes the problem, which was revealed by the online credentials update patch. Appeared, that admin user is very fragile, and any update of its rights, before universe was recovered, led to recovery/bootstrap error. The second patch makes it impossible to break the admin user explicitly. Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4606-replication-universe-v4 Issue: https://github.com/tarantool/tarantool/issues/4606 Vladislav Shpilevoy (2): replication: don't drop admin super privileges access: forbid to drop admin's universe access src/box/session.cc | 23 ------- src/box/user.cc | 28 +++++++++ test/box/access.result | 8 +++ test/box/access.test.lua | 6 ++ test/replication/gh-4606-admin-creds.result | 63 +++++++++++++++++++ test/replication/gh-4606-admin-creds.test.lua | 26 ++++++++ test/replication/suite.cfg | 1 + 7 files changed, 132 insertions(+), 23 deletions(-) create mode 100644 test/replication/gh-4606-admin-creds.result create mode 100644 test/replication/gh-4606-admin-creds.test.lua -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges 2019-11-01 21:42 [Tarantool-patches] [PATCH 0/2] Admin universe access Vladislav Shpilevoy @ 2019-11-01 21:42 ` Vladislav Shpilevoy 2019-11-05 12:40 ` Vladislav Shpilevoy 2019-11-05 18:20 ` Konstantin Osipov 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access Vladislav Shpilevoy 2019-11-12 9:37 ` [Tarantool-patches] [PATCH 0/2] Admin " Kirill Yukhin 2 siblings, 2 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-11-01 21:42 UTC (permalink / raw) To: tarantool-patches, kostja.osipov The admin user has universal privileges before bootstrap or recovery are done. That allows to, for example, bootstrap from a remote master, because to do that the admin should be able to insert into system spaces, such as _priv. But after the patch on online credentials update was implemented (#2763, 48d00b0e8b043b4697a474e6593a7608f2d9b0f1) the admin could loose its universal access if, for example, a role was granted to him before universal access was recovered. That happened by two reasons: - Any change in access rights, even in granted roles, led to rebuild of universal access; - Any change in access rights updated the universal access in all existing sessions, thanks to #2763. What happened: two tarantools were started. One of them master, granted 'replication' role to admin. Second node, slave, tried to bootstrap from the master. The slave created an admin session and started loading data. After it loaded 'grant replication role to admin' command, this nullified admin universal access everywhere, including this session. Next rows could not be applied. Closes #4606 --- src/box/session.cc | 23 ------- src/box/user.cc | 22 +++++++ test/replication/gh-4606-admin-creds.result | 63 +++++++++++++++++++ test/replication/gh-4606-admin-creds.test.lua | 26 ++++++++ test/replication/suite.cfg | 1 + 5 files changed, 112 insertions(+), 23 deletions(-) create mode 100644 test/replication/gh-4606-admin-creds.result create mode 100644 test/replication/gh-4606-admin-creds.test.lua diff --git a/src/box/session.cc b/src/box/session.cc index b171e2374..461d1cf25 100644 --- a/src/box/session.cc +++ b/src/box/session.cc @@ -173,11 +173,6 @@ session_create_on_demand() /* Add a trigger to destroy session on fiber stop */ trigger_add(&fiber()->on_stop, &s->fiber_on_stop); credentials_reset(&s->credentials, admin_user); - /* - * At bootstrap, admin user access is not loaded yet (is - * 0), force global access. @sa comment in session_init() - */ - s->credentials.universal_access = ~(user_access_t) 0; fiber_set_session(fiber(), s); fiber_set_user(fiber(), &s->credentials); return s; @@ -253,24 +248,6 @@ session_init() panic("out of memory"); mempool_create(&session_pool, &cord()->slabc, sizeof(struct session)); credentials_create(&admin_credentials, admin_user); - /* - * For performance reasons, we do not always explicitly - * look at user id in access checks, while still need to - * ensure 'admin' user has full access to all objects in - * the universe. - * - * This is why _priv table contains a record with grants - * of full access to universe to 'admin' user. - * - * Making a record in _priv table is, however, - * insufficient, since some checks are done at bootstrap, - * before _priv table is read (e.g. when we're - * bootstrapping a replica in applier fiber). - * - * When session_init() is called, admin user access is not - * loaded yet (is 0), force global access. - */ - admin_credentials.universal_access = ~((user_access_t) 0); } void diff --git a/src/box/user.cc b/src/box/user.cc index 03b4b2e3b..cdddf237b 100644 --- a/src/box/user.cc +++ b/src/box/user.cc @@ -49,6 +49,10 @@ static struct user_map user_map_nil; struct mh_i32ptr_t *user_registry; +enum { + USER_ACCESS_FULL = (user_access_t)~0, +}; + /* {{{ user_map */ static inline int @@ -570,6 +574,24 @@ user_cache_init() def->type = SC_USER; user = user_cache_replace(def); admin_def_guard.is_active = false; + /* + * For performance reasons, we do not always explicitly + * look at user id in access checks, while still need to + * ensure 'admin' user has full access to all objects in + * the universe. + * + * This is why _priv table contains a record with grants + * of full access to universe to 'admin' user. + * + * Making a record in _priv table is, however, + * insufficient, since some checks are done at bootstrap, + * before _priv table is read (e.g. when we're + * bootstrapping a replica in applier fiber). + * + * When user_cache_init() is called, admin user access is + * not loaded yet (is 0), force global access. + */ + universe.access[ADMIN].effective = USER_ACCESS_FULL; /* ADMIN is both the auth token and user id for 'admin' user. */ assert(user->def->uid == ADMIN && user->auth_token == ADMIN); } diff --git a/test/replication/gh-4606-admin-creds.result b/test/replication/gh-4606-admin-creds.result new file mode 100644 index 000000000..de882b34c --- /dev/null +++ b/test/replication/gh-4606-admin-creds.result @@ -0,0 +1,63 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... +-- +-- gh-4606: the admin user has universal privileges before +-- bootstrap or recovery are done. That allows to, for example, +-- bootstrap from a remote master, because to do that the admin +-- should be able to insert into system spaces, such as _priv. +-- +-- But the admin could lost its universal access if, for +-- example, a role was granted to him before universal access was +-- recovered. Because any change in access rights, even in granted +-- roles, led to rebuild of universal access. +-- +box.schema.user.passwd('admin', '111') + | --- + | ... +box.schema.user.grant('admin', 'replication') + | --- + | ... +test_run:cmd("create server replica_auth with rpl_master=default, script='replication/replica_auth.lua'") + | --- + | - true + | ... +test_run:cmd("start server replica_auth with args='admin:111 0.1'") + | --- + | - true + | ... +test_run:switch('replica_auth') + | --- + | - true + | ... +i = box.info + | --- + | ... +i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i + | --- + | - true + | ... +test_run:switch('default') + | --- + | - true + | ... +test_run:cmd("stop server replica_auth") + | --- + | - true + | ... +test_run:cmd("cleanup server replica_auth") + | --- + | - true + | ... +test_run:cmd("delete server replica_auth") + | --- + | - true + | ... + +box.schema.user.passwd('admin', '') + | --- + | ... +box.schema.user.revoke('admin', 'replication') + | --- + | ... diff --git a/test/replication/gh-4606-admin-creds.test.lua b/test/replication/gh-4606-admin-creds.test.lua new file mode 100644 index 000000000..7b1d310d3 --- /dev/null +++ b/test/replication/gh-4606-admin-creds.test.lua @@ -0,0 +1,26 @@ +test_run = require('test_run').new() +-- +-- gh-4606: the admin user has universal privileges before +-- bootstrap or recovery are done. That allows to, for example, +-- bootstrap from a remote master, because to do that the admin +-- should be able to insert into system spaces, such as _priv. +-- +-- But the admin could lost its universal access if, for +-- example, a role was granted to him before universal access was +-- recovered. Because any change in access rights, even in granted +-- roles, led to rebuild of universal access. +-- +box.schema.user.passwd('admin', '111') +box.schema.user.grant('admin', 'replication') +test_run:cmd("create server replica_auth with rpl_master=default, script='replication/replica_auth.lua'") +test_run:cmd("start server replica_auth with args='admin:111 0.1'") +test_run:switch('replica_auth') +i = box.info +i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i +test_run:switch('default') +test_run:cmd("stop server replica_auth") +test_run:cmd("cleanup server replica_auth") +test_run:cmd("delete server replica_auth") + +box.schema.user.passwd('admin', '') +box.schema.user.revoke('admin', 'replication') diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg index eb25077d8..13b9131cc 100644 --- a/test/replication/suite.cfg +++ b/test/replication/suite.cfg @@ -11,6 +11,7 @@ "on_schema_init.test.lua": {}, "long_row_timeout.test.lua": {}, "join_without_snap.test.lua": {}, + "gh-4606-admin-creds.test.lua": {}, "*": { "memtx": {"engine": "memtx"}, "vinyl": {"engine": "vinyl"} -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy @ 2019-11-05 12:40 ` Vladislav Shpilevoy 2019-11-05 18:20 ` Konstantin Osipov 1 sibling, 0 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-11-05 12:40 UTC (permalink / raw) To: tarantool-patches, kostja.osipov Sorry, found a bug in the test. > diff --git a/test/replication/gh-4606-admin-creds.result b/test/replication/gh-4606-admin-creds.result > new file mode 100644 > index 000000000..de882b34c > --- /dev/null > +++ b/test/replication/gh-4606-admin-creds.result > @@ -0,0 +1,63 @@ > +test_run:switch('replica_auth') > + | --- > + | - true > + | ... > +i = box.info > + | --- > + | ... > +i.replication[(i.id + 1) % 2].upstream.status == 'follow' or i Actually, ID of the other instance is 'i.id % 2 + 1'. I force pushed the fixed test. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy 2019-11-05 12:40 ` Vladislav Shpilevoy @ 2019-11-05 18:20 ` Konstantin Osipov 1 sibling, 0 replies; 7+ messages in thread From: Konstantin Osipov @ 2019-11-05 18:20 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/02 00:37]: > The admin user has universal privileges before bootstrap or > recovery are done. That allows to, for example, bootstrap from a > remote master, because to do that the admin should be able to > insert into system spaces, such as _priv. lgtm -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access 2019-11-01 21:42 [Tarantool-patches] [PATCH 0/2] Admin universe access Vladislav Shpilevoy 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy @ 2019-11-01 21:42 ` Vladislav Shpilevoy 2019-11-05 18:21 ` Konstantin Osipov 2019-11-12 9:37 ` [Tarantool-patches] [PATCH 0/2] Admin " Kirill Yukhin 2 siblings, 1 reply; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-11-01 21:42 UTC (permalink / raw) To: tarantool-patches, kostja.osipov Bootstrap and recovery work on behalf of admin. Without the universe access they are not able to even fill system spaces with data. It is better to forbid this ability until someone made their cluster unrecoverable. --- src/box/user.cc | 6 ++++++ test/box/access.result | 8 ++++++++ test/box/access.test.lua | 6 ++++++ 3 files changed, 20 insertions(+) diff --git a/src/box/user.cc b/src/box/user.cc index cdddf237b..6b4a5565e 100644 --- a/src/box/user.cc +++ b/src/box/user.cc @@ -764,6 +764,12 @@ priv_grant(struct user *grantee, struct priv_def *priv) struct access *object = access_find(priv->object_type, priv->object_id); if (object == NULL) return 0; + if (grantee->auth_token == ADMIN && priv->object_type == SC_UNIVERSE && + priv->access != USER_ACCESS_FULL) { + diag_set(ClientError, ER_GRANT, + "can't revoke universe from the admin user"); + return -1; + } struct access *access = &object[grantee->auth_token]; access->granted = priv->access; if (rebuild_effective_grants(grantee) != 0) diff --git a/test/box/access.result b/test/box/access.result index dc339038d..01126a94b 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -2099,3 +2099,11 @@ box.schema.user.drop("user2") box.schema.user.drop("user3") --- ... +-- +-- Check that admin user privileges can't be removed. Otherwise an +-- instance could not bootstrap nor recovery. +-- +box.space._priv:delete{1, 'universe', 0} +--- +- error: 'Incorrect grant arguments: can''t revoke universe from the admin user' +... diff --git a/test/box/access.test.lua b/test/box/access.test.lua index a9843d155..759827721 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -800,3 +800,9 @@ box.space._user:select{} box.schema.user.drop("user1") box.schema.user.drop("user2") box.schema.user.drop("user3") + +-- +-- Check that admin user privileges can't be removed. Otherwise an +-- instance could not bootstrap nor recovery. +-- +box.space._priv:delete{1, 'universe', 0} -- 2.21.0 (Apple Git-122.2) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access Vladislav Shpilevoy @ 2019-11-05 18:21 ` Konstantin Osipov 0 siblings, 0 replies; 7+ messages in thread From: Konstantin Osipov @ 2019-11-05 18:21 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/02 00:37]: > Bootstrap and recovery work on behalf of admin. Without the > universe access they are not able to even fill system spaces with > data. lgtm -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Admin universe access 2019-11-01 21:42 [Tarantool-patches] [PATCH 0/2] Admin universe access Vladislav Shpilevoy 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access Vladislav Shpilevoy @ 2019-11-12 9:37 ` Kirill Yukhin 2 siblings, 0 replies; 7+ messages in thread From: Kirill Yukhin @ 2019-11-12 9:37 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hello, On 01 ноя 22:42, Vladislav Shpilevoy wrote: > The patchset makes so the admin user never can loose its universe > access rights. Apparently, Tarantool can't even bootstrap nor > recovery without universe granted to the admin, because this user > owns the fibers doing recovery and bootstrap. > > First patch fixes the problem, which was revealed by the online > credentials update patch. Appeared, that admin user is very > fragile, and any update of its rights, before universe was > recovered, led to recovery/bootstrap error. > > The second patch makes it impossible to break the admin user > explicitly. > > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4606-replication-universe-v4 > Issue: https://github.com/tarantool/tarantool/issues/4606 > > Vladislav Shpilevoy (2): > replication: don't drop admin super privileges I've checked the patch into 1.10, 2.1, 2.2 and master. > access: forbid to drop admin's universe access I've checked the patch into master. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-12 9:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-01 21:42 [Tarantool-patches] [PATCH 0/2] Admin universe access Vladislav Shpilevoy 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges Vladislav Shpilevoy 2019-11-05 12:40 ` Vladislav Shpilevoy 2019-11-05 18:20 ` Konstantin Osipov 2019-11-01 21:42 ` [Tarantool-patches] [PATCH 2/2] access: forbid to drop admin's universe access Vladislav Shpilevoy 2019-11-05 18:21 ` Konstantin Osipov 2019-11-12 9:37 ` [Tarantool-patches] [PATCH 0/2] Admin " Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox