From 4eef163f2724ab4043fa39321aeeed5975da33c9 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 30 Jul 2026 13:47:29 -0700 Subject: [PATCH v4 4/8] 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 9083c5762cc..f1e8b624d8e 100644 --- a/src/backend/catalog/pg_subscription.c +++ b/src/backend/catalog/pg_subscription.c @@ -196,7 +196,7 @@ GetSubscription(Oid subid, bool missing_ok) * connect thus never hit them, which matters during restore. */ char * -SubscriptionConninfo(Subscription *sub, bool aclcheck) +SubscriptionConninfo(Subscription *sub) { HeapTuple tup; Form_pg_subscription subform; @@ -216,18 +216,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 4ff5a15fc53..fff4a0cb01f 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 e4baf29a206..d60825f9683 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