From 331840bce6463b99416fe7614464e10498972232 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 30 Jul 2026 13:47:29 -0700 Subject: [PATCH v3 07/11] Always check foreign-server USAGE when resolving subscription conninfo. Previously, this was skipped in some cases to avoid raising errors when conninfo wasn't even needed. That was wrong in cases where conninfo was needed. Now that we only build conninfo when needed, always perform the USAGE check. Addresses finding 7 in report from linked discussion. Co-authored-by: Shlok Kyal Reported-by: Noah Misch Reviewed-by: Hayato Kuroda (Fujitsu) Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com Backpatch-through: 19 --- src/backend/catalog/pg_subscription.c | 23 ++++++++++------------ src/backend/commands/subscriptioncmds.c | 9 +-------- src/backend/replication/logical/worker.c | 4 ++-- src/include/catalog/pg_subscription.h | 2 +- src/test/regress/expected/subscription.out | 3 +++ src/test/regress/sql/subscription.sql | 3 +++ 6 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c index 8fa2a460a25..76f09836bee 100644 --- a/src/backend/catalog/pg_subscription.c +++ b/src/backend/catalog/pg_subscription.c @@ -191,7 +191,7 @@ GetSubscription(Oid subid, bool missing_ok) * missing user mapping). */ char * -SubscriptionConninfo(Subscription *sub, bool aclcheck) +SubscriptionConninfo(Subscription *sub) { HeapTuple tup; Form_pg_subscription subform; @@ -211,18 +211,15 @@ SubscriptionConninfo(Subscription *sub, bool aclcheck) server = GetForeignServer(subform->subserver); - if (aclcheck) - { - aclresult = object_aclcheck(ForeignServerRelationId, - subform->subserver, - sub->owner, ACL_USAGE); - if (aclresult != ACLCHECK_OK) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"", - GetUserNameFromId(sub->owner, false), - server->servername))); - } + aclresult = object_aclcheck(ForeignServerRelationId, + subform->subserver, + sub->owner, ACL_USAGE); + if (aclresult != ACLCHECK_OK) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"", + GetUserNameFromId(sub->owner, false), + server->servername))); conninfo = ForeignServerConnectionString(sub->owner, server); } diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index d05ef3fb4b2..77e10d78e64 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1773,15 +1773,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, break; } - /* - * Skip ACL checks on the subscription's foreign server, if any. If - * changing the server (or replacing it with a raw connection), then the - * old one will be removed anyway. If changing something unrelated, - * there's no need to do an additional ACL check here; that will be done - * by the subscription worker. - */ if (orig_conninfo_needed) - orig_conninfo = SubscriptionConninfo(sub, false); + orig_conninfo = SubscriptionConninfo(sub); retain_dead_tuples = sub->retaindeadtuples; origin = sub->origin; diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 1cdd28f5049..9c4c31a5cbc 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -5114,7 +5114,7 @@ maybe_reread_subscription(void) * is enabled. Allocated in transaction context; must be copied to * ApplyContext when we set MySubscriptionConninfo. */ - new_conninfo = SubscriptionConninfo(newsub, true); + new_conninfo = SubscriptionConninfo(newsub); /* !slotname should never happen when enabled is true. */ Assert(newsub->slotname); @@ -5879,7 +5879,7 @@ InitializeLogRepWorker(void) */ MySubscriptionConninfo = MemoryContextStrdup(ApplyContext, - SubscriptionConninfo(MySubscription, true)); + SubscriptionConninfo(MySubscription)); MySubscriptionValid = true; diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h index 5a9c07fe8d6..d2781a0b837 100644 --- a/src/include/catalog/pg_subscription.h +++ b/src/include/catalog/pg_subscription.h @@ -222,7 +222,7 @@ typedef struct Subscription #endif /* EXPOSE_TO_CLIENT_CODE */ extern Subscription *GetSubscription(Oid subid, bool missing_ok); -extern char *SubscriptionConninfo(Subscription *sub, bool aclcheck); +extern char *SubscriptionConninfo(Subscription *sub); extern void DisableSubscription(Oid subid); extern int CountDBSubscriptions(Oid dbid); diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index d0955ca1159..f67ffab1f54 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -215,6 +215,9 @@ SET SESSION AUTHORIZATION regress_subscription_user3; BEGIN; ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret'; ABORT; +-- fail, connecting forms recheck USAGE on the foreign server +ALTER SUBSCRIPTION regress_testsub6 REFRESH PUBLICATION; +ERROR: subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server" -- fails, cannot drop slot DROP SUBSCRIPTION regress_testsub6; ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server" diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 98304737adc..47e2b6ef09c 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -161,6 +161,9 @@ BEGIN; ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret'; ABORT; +-- fail, connecting forms recheck USAGE on the foreign server +ALTER SUBSCRIPTION regress_testsub6 REFRESH PUBLICATION; + -- fails, cannot drop slot DROP SUBSCRIPTION regress_testsub6; -- 2.43.0