[Tarantool-patches] [PATCH 1/2] replication: don't drop admin super privileges

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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)