From 3ff2b473663ccc32e06c72b91c8a6db7bcf19cd9 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Wed, 19 Dec 2018 05:40:55 +0100 Subject: [PATCH] Restrict visibility of get_*_active_transactions functions to pg_monitor --- .../distributed/transaction/backend_data.c | 24 +++++ src/include/distributed/backend_data.h | 1 + .../isolation_get_all_active_transactions.out | 87 +++++++++++++++ src/test/regress/isolation_schedule | 1 + ...isolation_get_all_active_transactions.spec | 102 ++++++++++++++++++ 5 files changed, 215 insertions(+) create mode 100644 src/test/regress/expected/isolation_get_all_active_transactions.out create mode 100644 src/test/regress/specs/isolation_get_all_active_transactions.spec diff --git a/src/backend/distributed/transaction/backend_data.c b/src/backend/distributed/transaction/backend_data.c index 119d5d1c1..70820b219 100644 --- a/src/backend/distributed/transaction/backend_data.c +++ b/src/backend/distributed/transaction/backend_data.c @@ -15,6 +15,7 @@ #include "funcapi.h" #include "access/htup_details.h" +#include "catalog/pg_authid.h" #include "catalog/pg_type.h" #include "datatype/timestamp.h" #include "distributed/backend_data.h" @@ -94,6 +95,8 @@ PG_FUNCTION_INFO_V1(get_all_active_transactions); Datum assign_distributed_transaction_id(PG_FUNCTION_ARGS) { + Oid userId = GetUserId(); + CheckCitusVersion(ERROR); /* MyBackendData should always be avaliable, just out of paranoia */ @@ -120,6 +123,7 @@ assign_distributed_transaction_id(PG_FUNCTION_ARGS) } MyBackendData->databaseId = MyDatabaseId; + MyBackendData->userId = userId; MyBackendData->transactionId.initiatorNodeIdentifier = PG_GETARG_INT32(0); MyBackendData->transactionId.transactionNumber = PG_GETARG_INT64(1); @@ -385,6 +389,8 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto Datum values[ACTIVE_TRANSACTION_COLUMN_COUNT]; bool isNulls[ACTIVE_TRANSACTION_COLUMN_COUNT]; + bool showAllTransactions = superuser(); + const Oid userId = GetUserId(); /* * We don't want to initialize memory while spinlock is held so we @@ -394,6 +400,11 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto memset(values, 0, sizeof(values)); memset(isNulls, false, sizeof(isNulls)); + if (is_member_of_role(userId, DEFAULT_ROLE_MONITOR)) + { + showAllTransactions = true; + } + /* we're reading all distributed transactions, prevent new backends */ LockBackendSharedMemory(LW_SHARED); @@ -412,6 +423,16 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto continue; } + /* + * Unless the user has a role that allows seeing all transactions (superuser, + * pg_monitor), skip over transactions belonging to other users. + */ + if (!showAllTransactions && currentBackend->userId != userId) + { + SpinLockRelease(¤tBackend->mutex); + continue; + } + values[0] = ObjectIdGetDatum(currentBackend->databaseId); values[1] = Int32GetDatum(ProcGlobal->allProcs[backendIndex].pid); values[2] = Int32GetDatum(currentBackend->citusBackend.initiatorNodeIdentifier); @@ -693,6 +714,7 @@ UnSetDistributedTransactionId(void) SpinLockAcquire(&MyBackendData->mutex); MyBackendData->databaseId = 0; + MyBackendData->userId = 0; MyBackendData->transactionId.initiatorNodeIdentifier = 0; MyBackendData->transactionId.transactionOriginator = false; MyBackendData->transactionId.transactionNumber = 0; @@ -781,10 +803,12 @@ AssignDistributedTransactionId(void) uint64 nextTransactionNumber = pg_atomic_fetch_add_u64(transactionNumberSequence, 1); int localGroupId = GetLocalGroupId(); TimestampTz currentTimestamp = GetCurrentTimestamp(); + Oid userId = GetUserId(); SpinLockAcquire(&MyBackendData->mutex); MyBackendData->databaseId = MyDatabaseId; + MyBackendData->userId = userId; MyBackendData->transactionId.initiatorNodeIdentifier = localGroupId; MyBackendData->transactionId.transactionOriginator = true; diff --git a/src/include/distributed/backend_data.h b/src/include/distributed/backend_data.h index f52b69125..cc0f04ca8 100644 --- a/src/include/distributed/backend_data.h +++ b/src/include/distributed/backend_data.h @@ -47,6 +47,7 @@ typedef struct CitusInitiatedBackend typedef struct BackendData { Oid databaseId; + Oid userId; slock_t mutex; bool cancelledDueToDeadlock; CitusInitiatedBackend citusBackend; diff --git a/src/test/regress/expected/isolation_get_all_active_transactions.out b/src/test/regress/expected/isolation_get_all_active_transactions.out new file mode 100644 index 000000000..3c69faafd --- /dev/null +++ b/src/test/regress/expected/isolation_get_all_active_transactions.out @@ -0,0 +1,87 @@ +Parsed test spec with 3 sessions + +starting permutation: s1-grant s1-begin-insert s2-begin-insert s3-as-admin s3-as-user-1 s3-as-readonly s3-as-monitor s1-commit s2-commit +run_command_on_workers + +(localhost,57637,t,"GRANT ROLE") +(localhost,57638,t,"GRANT ROLE") +step s1-grant: + GRANT ALL ON test_table TO test_user_1; + SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_1'); + + GRANT ALL ON test_table TO test_user_2; + SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_2'); + +bool_and + +t +bool_and + +t +step s1-begin-insert: + BEGIN; + SET ROLE test_user_1; + INSERT INTO test_table VALUES (100, 100); + +step s2-begin-insert: + BEGIN; + SET ROLE test_user_2; + INSERT INTO test_table VALUES (200, 200); + +step s3-as-admin: + -- Admin should be able to see all transactions + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); + +count + +2 +count + +4 +step s3-as-user-1: + -- User should only be able to see its own transactions + SET ROLE test_user_1; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); + +count + +1 +count + +2 +step s3-as-readonly: + -- Other user should not see transactions + SET ROLE test_readonly; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); + +count + +0 +count + +0 +step s3-as-monitor: + -- Monitor should see all transactions + SET ROLE test_monitor; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); + +count + +2 +count + +4 +step s1-commit: + COMMIT; + +step s2-commit: + COMMIT; + +run_command_on_workers + +(localhost,57637,f,"ERROR: role ""test_user_1"" cannot be dropped because some objects depend on it") +(localhost,57638,f,"ERROR: role ""test_user_1"" cannot be dropped because some objects depend on it") diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index fdbd3ffc4..a457c145d 100644 --- a/src/test/regress/isolation_schedule +++ b/src/test/regress/isolation_schedule @@ -43,6 +43,7 @@ test: isolation_truncate_vs_all test: isolation_drop_vs_all test: isolation_ddl_vs_all test: isolation_citus_dist_activity +test: isolation_get_all_active_transactions test: isolation_validate_vs_insert test: isolation_insert_select_conflict diff --git a/src/test/regress/specs/isolation_get_all_active_transactions.spec b/src/test/regress/specs/isolation_get_all_active_transactions.spec new file mode 100644 index 000000000..2fcddcc65 --- /dev/null +++ b/src/test/regress/specs/isolation_get_all_active_transactions.spec @@ -0,0 +1,102 @@ +setup +{ + SET citus.shard_replication_factor TO 1; + + CREATE TABLE test_table(column1 int, column2 int); + SELECT create_distributed_table('test_table', 'column1'); + + CREATE USER test_user_1; + SELECT run_command_on_workers('CREATE USER test_user_1'); + + CREATE USER test_user_2; + SELECT run_command_on_workers('CREATE USER test_user_2'); + + CREATE USER test_readonly; + SELECT run_command_on_workers('CREATE USER test_readonly'); + + CREATE USER test_monitor; + SELECT run_command_on_workers('CREATE USER test_monitor'); + + GRANT pg_monitor TO test_monitor; + SELECT run_command_on_workers('GRANT pg_monitor TO test_monitor'); +} + +teardown +{ + DROP TABLE test_table; + DROP USER test_user_1, test_user_2, test_readonly, test_monitor; + SELECT run_command_on_workers('DROP USER test_user_1, test_user_2, test_readonly, test_monitor'); +} + +session "s1" + +# run_command_on_placements is done in a separate step because the setup is executed as a single transaction +step "s1-grant" +{ + GRANT ALL ON test_table TO test_user_1; + SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_1'); + + GRANT ALL ON test_table TO test_user_2; + SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_2'); +} + +step "s1-begin-insert" +{ + BEGIN; + SET ROLE test_user_1; + INSERT INTO test_table VALUES (100, 100); +} + +step "s1-commit" +{ + COMMIT; +} + +session "s2" + +step "s2-begin-insert" +{ + BEGIN; + SET ROLE test_user_2; + INSERT INTO test_table VALUES (200, 200); +} + +step "s2-commit" +{ + COMMIT; +} + +session "s3" + +step "s3-as-admin" +{ + -- Admin should be able to see all transactions + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); +} + +step "s3-as-user-1" +{ + -- User should only be able to see its own transactions + SET ROLE test_user_1; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); +} + +step "s3-as-readonly" +{ + -- Other user should not see transactions + SET ROLE test_readonly; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); +} + +step "s3-as-monitor" +{ + -- Monitor should see all transactions + SET ROLE test_monitor; + SELECT count(*) FROM get_all_active_transactions(); + SELECT count(*) FROM get_global_active_transactions(); +} + +permutation "s1-grant" "s1-begin-insert" "s2-begin-insert" "s3-as-admin" "s3-as-user-1" "s3-as-readonly" "s3-as-monitor" "s1-commit" "s2-commit"