You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by dj...@apache.org on 2026/10/22 09:53:33 UTC
(cloudberry) 03/03: Restrict gp_add_master_standby() to superusers
This is an automated email from the ASF dual-hosted git repository.
tuhaihe pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit 1991c4d102a5118270f3ffb45eba21c4d00a6e81
Author: Jianghua Yang <yj...@gmail.com>
AuthorDate: Thu Oct 22 10:37:44 2026 +0800
Restrict gp_add_master_standby() to superusers
gp_add_master_standby() was the only segadmin function whose
mirroring_sanity_check() flags did not include SUPERUSER, and like the
others it had proacl = NULL (PUBLIC EXECUTE). From a utility-mode
connection, a plain role could therefore insert a standby row into
gp_segment_configuration on a cluster that has no standby, after which
gpinitstandby refuses to add a real one until a superuser removes the
bogus row.
The function now passes SUPERUSER to mirroring_sanity_check(), like
gp_remove_master_standby() and the segment functions, and runs that
check before it validates its arguments. mirroring_sanity_check() checks
the privilege before the mode and location checks, with
ERRCODE_INSUFFICIENT_PRIVILEGE, so an unprivileged caller is refused
without learning anything else. The check takes effect on a binary
upgrade; system_functions.sql additionally revokes both signatures from
PUBLIC for new initdbs.
The privileges regression test checks both layers: a regular role is
denied at the privilege layer, and with EXECUTE granted (the state of a
cluster upgraded without initdb, where proacl stays NULL) by the
function itself, also when the arguments are NULL.
gpinitstandby, the in-tree caller, runs as the superuser and is
unaffected.
---
src/backend/catalog/system_functions.sql | 8 ++++++++
src/backend/utils/gp/segadmin.c | 25 ++++++++++++++++---------
src/test/regress/expected/privileges.out | 26 ++++++++++++++++++++++++++
src/test/regress/sql/privileges.sql | 25 +++++++++++++++++++++++++
4 files changed, 75 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 0beaeac06d1..aa00caa681d 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -781,6 +781,14 @@ REVOKE EXECUTE ON FUNCTION gp_expand_lock_catalog() FROM public;
REVOKE EXECUTE ON FUNCTION gp_expand_bump_version() FROM public;
+-- Adding a standby coordinator is reserved to superusers, like every other
+-- change to the segment configuration made by the management tools. The
+-- function enforces that itself as well; this is defense in depth for new
+-- initdbs.
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text) FROM public;
+
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text, integer) FROM public;
+
--
-- We also set up some things as accessible to standard roles.
diff --git a/src/backend/utils/gp/segadmin.c b/src/backend/utils/gp/segadmin.c
index e8b9b309cb6..947e695a725 100644
--- a/src/backend/utils/gp/segadmin.c
+++ b/src/backend/utils/gp/segadmin.c
@@ -128,6 +128,19 @@ get_maxcontentid()
static void
mirroring_sanity_check(int flags, const char *func)
{
+ /*
+ * Check the privilege first: a role that may not call the function at all
+ * is refused before it learns anything about the mode or the role of this
+ * server.
+ */
+ if ((flags & SUPERUSER) == SUPERUSER)
+ {
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("%s can only be run by a superuser", func)));
+ }
+
if ((flags & MASTER_ONLY) == MASTER_ONLY)
{
if (GpIdentity.dbid == UNINITIALIZED_GP_IDENTITY_VALUE)
@@ -149,12 +162,6 @@ mirroring_sanity_check(int flags, const char *func)
elog(ERROR, "%s must be run in single-user mode", func);
}
- if ((flags & SUPERUSER) == SUPERUSER)
- {
- if (!superuser())
- elog(ERROR, "%s can only be run by a superuser", func);
- }
-
if ((flags & SEGMENT_ONLY) == SEGMENT_ONLY)
{
if (GpIdentity.dbid == UNINITIALIZED_GP_IDENTITY_VALUE)
@@ -589,6 +596,9 @@ gp_add_master_standby(PG_FUNCTION_ARGS)
GpSegConfigEntry *config;
Relation gprel;
+ mirroring_sanity_check(MASTER_ONLY | UTILITY_MODE | SUPERUSER,
+ "gp_add_master_standby");
+
if (PG_ARGISNULL(0))
elog(ERROR, "host name cannot be NULL");
if (PG_ARGISNULL(1))
@@ -596,9 +606,6 @@ gp_add_master_standby(PG_FUNCTION_ARGS)
if (PG_ARGISNULL(2))
elog(ERROR, "datadir cannot be NULL");
- mirroring_sanity_check(MASTER_ONLY | UTILITY_MODE,
- "gp_add_master_standby");
-
/* Check if the system is ok */
if (standby_exists())
elog(ERROR, "only a single master standby may be defined");
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index b26f447b99e..6136cbb617f 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -3320,3 +3320,29 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+-- gp_add_master_standby() is reserved to superusers: a regular role is
+-- denied at the privilege layer, and with EXECUTE granted - the state of a
+-- cluster upgraded without initdb, where proacl stays NULL - by the function
+-- itself.
+CREATE ROLE regress_standby_user;
+SET SESSION AUTHORIZATION regress_standby_user;
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent');
+ERROR: permission denied for function gp_add_master_standby
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent', 5432);
+ERROR: permission denied for function gp_add_master_standby
+RESET SESSION AUTHORIZATION;
+GRANT EXECUTE ON FUNCTION gp_add_master_standby(text, text, text) TO regress_standby_user;
+GRANT EXECUTE ON FUNCTION gp_add_master_standby(text, text, text, integer) TO regress_standby_user;
+SET SESSION AUTHORIZATION regress_standby_user;
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent');
+ERROR: gp_add_master_standby can only be run by a superuser
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent', 5432);
+ERROR: gp_add_master_standby can only be run by a superuser
+-- the privilege is checked before the arguments
+SELECT gp_add_master_standby(NULL, NULL, NULL);
+ERROR: gp_add_master_standby can only be run by a superuser
+RESET SESSION AUTHORIZATION;
+-- clean up
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text) FROM regress_standby_user;
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text, integer) FROM regress_standby_user;
+DROP ROLE regress_standby_user;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 89a01ed421e..3af2501b542 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -2127,3 +2127,28 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+
+-- gp_add_master_standby() is reserved to superusers: a regular role is
+-- denied at the privilege layer, and with EXECUTE granted - the state of a
+-- cluster upgraded without initdb, where proacl stays NULL - by the function
+-- itself.
+CREATE ROLE regress_standby_user;
+
+SET SESSION AUTHORIZATION regress_standby_user;
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent');
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent', 5432);
+RESET SESSION AUTHORIZATION;
+
+GRANT EXECUTE ON FUNCTION gp_add_master_standby(text, text, text) TO regress_standby_user;
+GRANT EXECUTE ON FUNCTION gp_add_master_standby(text, text, text, integer) TO regress_standby_user;
+SET SESSION AUTHORIZATION regress_standby_user;
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent');
+SELECT gp_add_master_standby('localhost', 'localhost', '/nonexistent', 5432);
+-- the privilege is checked before the arguments
+SELECT gp_add_master_standby(NULL, NULL, NULL);
+RESET SESSION AUTHORIZATION;
+
+-- clean up
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text) FROM regress_standby_user;
+REVOKE EXECUTE ON FUNCTION gp_add_master_standby(text, text, text, integer) FROM regress_standby_user;
+DROP ROLE regress_standby_user;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org