pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed[16+] subscription can end up in inconsistent state
25+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2024-01-18 dd3ca8cbb0 5b5318c387 Fix buildfarm error from commit 5c31669058.
2024-01-12 5c31669058 4c03ac7e2b Re-validate connection string in libpqrcv_connect().
* [16+] subscription can end up in inconsistent state
@ 2023-09-10 15:45 Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Davis @ 2023-09-10 15:45 UTC (permalink / raw)
To: pgsql-bugs; +Cc: Robert Haas <robertmhaas@gmail.com>
[ Moved from -security, where it was initially posted as a precaution.
It was determined that the normal bug process is the right way to fix
this. ]
If I understand correctly, the security requirements for checking a
non-superuser's connection string are that (a) the connection string
itself specifies a password, thereby ensuring it won't come from a
PGPASSFILE; and (b) checking PQconnectionUsedPassword() after the
connection to ensure that the password was actually used for
authentication, rather than some other method.
If subpasswordrequired=true, that causes (b) to be consistently checked
for all connections.
The checking of (a) is more complicated -- it's checked at CREATE/ALTER
time, but only if the user is not a superuser. If the superuser gives
the subscription to a non-superuser, or if the subowner loses their
superuser status, then the subscription is in a weird state:
subpasswordrequired may be true even if subconninfo doesn't have a
password. That means that (a) is not being checked, and (b) is being
checked.
Repro (16+):
Publisher:
CREATE USER repl REPLICATION PASSWORD 'secret';
CREATE TABLE t(i INT);
INSERT INTO t VALUES(1);
GRANT SELECT ON t TO repl;
CREATE PUBLICATION p1 FOR TABLE t;
Subscriber (has a PGPASSFILE for user "repl"):
CREATE USER u1;
CREATE TABLE t(i INT);
ALTER TABLE t OWNER TO u1;
-- no password specified; relies on passfile
CREATE SUBSCRIPTION s1
CONNECTION 'dbname=postgres user=repl'
PUBLICATION p1 WITH (enabled=false);
ALTER SUBSCRIPTION s1 OWNER TO u1;
SELECT COUNT(*) FROM t; -- 0
\c - u1
-- still using passfile
ALTER SUBSCRIPTION s1 ENABLE;
SELECT pg_sleep(2);
SELECT COUNT(*) FROM t; -- 1
ALTER SUBSCRIPTION s1 REFRESH PUBLICATION;
It's also possible to get into this state by the superuser modifying a
non-superuser's subscription.
The purpose of the "password_required" option is to handle the case
where a subscription was formerly owned by a superuser, or has been
modified by a superuser. The docs say:
"[password_required] ... This setting is ignored when the
subscription is owned by a superuser... Only superusers can set this
value to false."
https://www.postgresql.org/docs/16/sql-createsubscription.html
So a superuser changing the owner or modifying another user's
subscription is a supported operation, and not just a superuser doing
the wrong thing. (Perhaps the docs should be more clear on this point?)
We could address this either by:
* maintaining the invariant that if subpasswordrequired=true, then
subconninfo specifies a password; or
* calling walrcv_check_conninfo() before each connection
The former might raise some compatibility concerns with 15 (or
questions about what we can do with the default for
"password_required"), so right now the latter looks like the right fix.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-11 03:29 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 25+ messages in thread
From: Amit Kapila @ 2023-09-11 03:29 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Sun, Sep 10, 2023 at 9:15 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> [ Moved from -security, where it was initially posted as a precaution.
> It was determined that the normal bug process is the right way to fix
> this. ]
>
> If I understand correctly, the security requirements for checking a
> non-superuser's connection string are that (a) the connection string
> itself specifies a password, thereby ensuring it won't come from a
> PGPASSFILE; and (b) checking PQconnectionUsedPassword() after the
> connection to ensure that the password was actually used for
> authentication, rather than some other method.
>
> If subpasswordrequired=true, that causes (b) to be consistently checked
> for all connections.
>
> The checking of (a) is more complicated -- it's checked at CREATE/ALTER
> time, but only if the user is not a superuser. If the superuser gives
> the subscription to a non-superuser, or if the subowner loses their
> superuser status, then the subscription is in a weird state:
> subpasswordrequired may be true even if subconninfo doesn't have a
> password. That means that (a) is not being checked, and (b) is being
> checked.
>
> Repro (16+):
>
> Publisher:
> CREATE USER repl REPLICATION PASSWORD 'secret';
> CREATE TABLE t(i INT);
> INSERT INTO t VALUES(1);
> GRANT SELECT ON t TO repl;
> CREATE PUBLICATION p1 FOR TABLE t;
>
> Subscriber (has a PGPASSFILE for user "repl"):
> CREATE USER u1;
> CREATE TABLE t(i INT);
> ALTER TABLE t OWNER TO u1;
> -- no password specified; relies on passfile
> CREATE SUBSCRIPTION s1
> CONNECTION 'dbname=postgres user=repl'
> PUBLICATION p1 WITH (enabled=false);
> ALTER SUBSCRIPTION s1 OWNER TO u1;
> SELECT COUNT(*) FROM t; -- 0
> \c - u1
> -- still using passfile
> ALTER SUBSCRIPTION s1 ENABLE;
> SELECT pg_sleep(2);
> SELECT COUNT(*) FROM t; -- 1
> ALTER SUBSCRIPTION s1 REFRESH PUBLICATION;
>
> It's also possible to get into this state by the superuser modifying a
> non-superuser's subscription.
>
> The purpose of the "password_required" option is to handle the case
> where a subscription was formerly owned by a superuser, or has been
> modified by a superuser. The docs say:
>
> "[password_required] ... This setting is ignored when the
> subscription is owned by a superuser... Only superusers can set this
> value to false."
> https://www.postgresql.org/docs/16/sql-createsubscription.html
>
> So a superuser changing the owner or modifying another user's
> subscription is a supported operation, and not just a superuser doing
> the wrong thing. (Perhaps the docs should be more clear on this point?)
>
> We could address this either by:
>
> * maintaining the invariant that if subpasswordrequired=true, then
> subconninfo specifies a password; or
> * calling walrcv_check_conninfo() before each connection
>
Can we think of calling walrcv_check_conninfo() when the following
check is true (MySubscription->passwordrequired &&
!superuser_arg(MySubscription->owner))? IIUC this has to be done for
both apply_worker and tablesync_worker.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-11 19:00 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 3 replies; 25+ messages in thread
From: Jeff Davis @ 2023-09-11 19:00 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Mon, 2023-09-11 at 08:59 +0530, Amit Kapila wrote:
> Can we think of calling walrcv_check_conninfo() when the following
> check is true (MySubscription->passwordrequired &&
> !superuser_arg(MySubscription->owner))? IIUC this has to be done for
> both apply_worker and tablesync_worker.
To me, it would make sense to just have walrcv_connect() do both checks
(a) and (b) -- rather than only check (b) -- when must_use_password is
true. Separating these checks (which are really two parts of the same
check) led to this problem in the first place.
Another thought: we may also need to invalidate the subscription worker
in cases where a user loses their superuser status.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-12 10:43 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
2 siblings, 1 reply; 25+ messages in thread
From: Amit Kapila @ 2023-09-12 10:43 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Tue, Sep 12, 2023 at 12:30 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2023-09-11 at 08:59 +0530, Amit Kapila wrote:
> > Can we think of calling walrcv_check_conninfo() when the following
> > check is true (MySubscription->passwordrequired &&
> > !superuser_arg(MySubscription->owner))? IIUC this has to be done for
> > both apply_worker and tablesync_worker.
>
> To me, it would make sense to just have walrcv_connect() do both checks
> (a) and (b) -- rather than only check (b) -- when must_use_password is
> true. Separating these checks (which are really two parts of the same
> check) led to this problem in the first place.
>
Sounds reasonable to me. However, we need to think about the call to
walrcv_check_conninfo() in CreateSubscription(). Do we want to remove
that as anyway, we will do that check via walrcv_connect()? If we do
so, then there is another question. Currently, walrcv_check_conninfo
is being invoked even when we don't connect but do we need to do that
in the first place?
Another point is that if we want to unify such a check at the time of
walrcv_connect() then do we need to do it at the time of Alter
Subscription? I think it will probably be better to catch the problem
early but does removing it from Alter Subscription time and doing it
at connect time lead to security hazards?
> Another thought: we may also need to invalidate the subscription worker
> in cases where a user loses their superuser status.
>
Yeah, I think that will be a good idea, especially in this context
where it can also affect remote connection.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-12 20:33 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Davis @ 2023-09-12 20:33 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Tue, 2023-09-12 at 16:13 +0530, Amit Kapila wrote:
> Do we want to remove
> that as anyway, we will do that check via walrcv_connect()?
I think we should keep the DDL-time checks in place as a best-effort,
but not rely on them for security.
> Another point is that if we want to unify such a check at the time of
> walrcv_connect() then do we need to do it at the time of Alter
> Subscription? I think it will probably be better to catch the problem
> early
Agreed. Catching mistakes at DDL time is a better user experience.
> but does removing it from Alter Subscription time and doing it
> at connect time lead to security hazards?
We'd still be doing the same check, just later, right? If so there's
not a big security risk in removing the DDL-time checks. But it's
probably not a good idea to have non-superuser-owned subscriptions
without a password specified, so there may be some hazard there.
>
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-13 04:51 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 25+ messages in thread
From: Amit Kapila @ 2023-09-13 04:51 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Wed, Sep 13, 2023 at 2:04 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2023-09-12 at 16:13 +0530, Amit Kapila wrote:
> > Do we want to remove
> > that as anyway, we will do that check via walrcv_connect()?
>
> I think we should keep the DDL-time checks in place as a best-effort,
> but not rely on them for security.
>
> > Another point is that if we want to unify such a check at the time of
> > walrcv_connect() then do we need to do it at the time of Alter
> > Subscription? I think it will probably be better to catch the problem
> > early
>
> Agreed. Catching mistakes at DDL time is a better user experience.
>
> > but does removing it from Alter Subscription time and doing it
> > at connect time lead to security hazards?
>
> We'd still be doing the same check, just later, right?
>
Right.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-19 14:03 Robert Haas <robertmhaas@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
2 siblings, 0 replies; 25+ messages in thread
From: Robert Haas @ 2023-09-19 14:03 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Mon, Sep 11, 2023 at 3:00 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Mon, 2023-09-11 at 08:59 +0530, Amit Kapila wrote:
> > Can we think of calling walrcv_check_conninfo() when the following
> > check is true (MySubscription->passwordrequired &&
> > !superuser_arg(MySubscription->owner))? IIUC this has to be done for
> > both apply_worker and tablesync_worker.
>
> To me, it would make sense to just have walrcv_connect() do both checks
> (a) and (b) -- rather than only check (b) -- when must_use_password is
> true. Separating these checks (which are really two parts of the same
> check) led to this problem in the first place.
Sorry for the slow response. I agree with this, and I also think that
keeping the existing checks makes sense.
> Another thought: we may also need to invalidate the subscription worker
> in cases where a user loses their superuser status.
I am not sure whether there's any problem here. I don't think it's
important that the worker realizes that it's lost superuser
instantaneously. But it should probably realize it, say, at the next
transaction boundary.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-21 19:47 vignesh C <vignesh21@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
2 siblings, 1 reply; 25+ messages in thread
From: vignesh C @ 2023-09-21 19:47 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs@postgresql.org, Robert Haas <robertmhaas@gmail.com>
On Tue, 12 Sept 2023 at 01:42, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2023-09-11 at 08:59 +0530, Amit Kapila wrote:
> > Can we think of calling walrcv_check_conninfo() when the following
> > check is true (MySubscription->passwordrequired &&
> > !superuser_arg(MySubscription->owner))? IIUC this has to be done for
> > both apply_worker and tablesync_worker.
>
> To me, it would make sense to just have walrcv_connect() do both checks
> (a) and (b) -- rather than only check (b) -- when must_use_password is
> true. Separating these checks (which are really two parts of the same
> check) led to this problem in the first place.
Modified it to make the check in walrcv_connect.
> Another thought: we may also need to invalidate the subscription worker
> in cases where a user loses their superuser status.
Added invalidation of the subscription worker.
Attached patch has the changes for the same.
Regards,
Vignesh
Attachments:
v1-0001-Super-user-related-fixes-in-logical-replication.patch (text/x-patch, 18.0K) download
From 440934abc57e0117391a1f975b6e28844683af57 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Thu, 21 Sep 2023 19:10:18 +0530
Subject: [PATCH v1] Super user related fixes in logical replication.
This patch fixes a couple of super user related issues in logical
replication:
a) Make sure that the password option is provided for non super users
when password required is true.
b) Restart the apply worker if it identifies that the super user
privilege has changed for the subscription owner.
---
src/backend/commands/subscriptioncmds.c | 28 +++++-----
.../libpqwalreceiver/libpqwalreceiver.c | 51 +++++++++++++------
src/backend/replication/logical/tablesync.c | 8 +--
src/backend/replication/logical/worker.c | 25 ++++++---
src/backend/replication/walreceiver.c | 2 +-
src/include/catalog/pg_subscription.h | 1 +
src/include/replication/walreceiver.h | 14 ++---
src/test/subscription/t/027_nosuperuser.pl | 33 +++++++++++-
8 files changed, 112 insertions(+), 50 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 6fe111e98d..e6ab5fed39 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -673,7 +673,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
- walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
+ walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser(), NULL);
/* Everything ok, form a new tuple. */
memset(values, 0, sizeof(values));
@@ -733,12 +733,11 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
List *tables;
ListCell *lc;
char table_state;
- bool must_use_password;
+ bool is_superuser = superuser_arg(owner);
/* Try to connect to the publisher. */
- must_use_password = !superuser_arg(owner) && opts.passwordrequired;
- wrconn = walrcv_connect(conninfo, true, must_use_password,
- stmt->subname, &err);
+ wrconn = walrcv_connect(conninfo, true, opts.passwordrequired,
+ is_superuser, stmt->subname, &err);
if (!wrconn)
ereport(ERROR,
(errcode(ERRCODE_CONNECTION_FAILURE),
@@ -863,15 +862,14 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
} SubRemoveRels;
SubRemoveRels *sub_remove_rels;
WalReceiverConn *wrconn;
- bool must_use_password;
+ bool is_superuser = superuser_arg(sub->owner);
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Try to connect to the publisher. */
- must_use_password = !superuser_arg(sub->owner) && sub->passwordrequired;
- wrconn = walrcv_connect(sub->conninfo, true, must_use_password,
- sub->name, &err);
+ wrconn = walrcv_connect(sub->conninfo, true, sub->passwordrequired,
+ is_superuser, sub->name, &err);
if (!wrconn)
ereport(ERROR,
(errcode(ERRCODE_CONNECTION_FAILURE),
@@ -1249,7 +1247,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
walrcv_check_conninfo(stmt->conninfo,
- sub->passwordrequired && !superuser_arg(sub->owner));
+ sub->passwordrequired && !superuser_arg(sub->owner), NULL);
values[Anum_pg_subscription_subconninfo - 1] =
CStringGetTextDatum(stmt->conninfo);
@@ -1488,7 +1486,8 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
WalReceiverConn *wrconn;
Form_pg_subscription form;
List *rstates;
- bool must_use_password;
+ bool passwordrequired;
+ bool is_superuser;
/*
* Lock pg_subscription with AccessExclusiveLock to ensure that the
@@ -1519,7 +1518,8 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
form = (Form_pg_subscription) GETSTRUCT(tup);
subid = form->oid;
subowner = form->subowner;
- must_use_password = !superuser_arg(subowner) && form->subpasswordrequired;
+ passwordrequired = form->subpasswordrequired;
+ is_superuser = superuser_arg(subowner);
/* must be owner */
if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId()))
@@ -1682,8 +1682,8 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
load_file("libpqwalreceiver", false);
- wrconn = walrcv_connect(conninfo, true, must_use_password,
- subname, &err);
+ wrconn = walrcv_connect(conninfo, true, passwordrequired,
+ is_superuser, subname, &err);
if (wrconn == NULL)
{
if (!slotname)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..c0820939b7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -49,10 +49,12 @@ struct WalReceiverConn
/* Prototypes for interface functions */
static WalReceiverConn *libpqrcv_connect(const char *conninfo,
- bool logical, bool must_use_password,
- const char *appname, char **err);
+ bool logical, bool passwordrequired,
+ bool is_superuser, const char *appname,
+ char **err);
static void libpqrcv_check_conninfo(const char *conninfo,
- bool must_use_password);
+ bool must_use_password,
+ WalReceiverConn *conn);
static char *libpqrcv_get_conninfo(WalReceiverConn *conn);
static void libpqrcv_get_senderinfo(WalReceiverConn *conn,
char **sender_host, int *sender_port);
@@ -121,7 +123,7 @@ _PG_init(void)
* Establish the connection to the primary server for XLOG streaming
*
* If an error occurs, this function will normally return NULL and set *err
- * to a palloc'ed error message. However, if must_use_password is true and
+ * to a palloc'ed error message. However, if passwordrequired is true and
* the connection fails to use the password, this function will ereport(ERROR).
* We do this because in that case the error includes a detail and a hint for
* consistency with other parts of the system, and it's not worth adding the
@@ -129,8 +131,8 @@ _PG_init(void)
* case.
*/
static WalReceiverConn *
-libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
- const char *appname, char **err)
+libpqrcv_connect(const char *conninfo, bool logical, bool passwordrequired,
+ bool is_superuser, const char *appname, char **err)
{
WalReceiverConn *conn;
const char *keys[6];
@@ -187,16 +189,20 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
if (PQstatus(conn->streamConn) != CONNECTION_OK)
goto bad_connection_errmsg;
- if (must_use_password && !PQconnectionUsedPassword(conn->streamConn))
+ if (passwordrequired && !is_superuser)
{
- libpqsrv_disconnect(conn->streamConn);
- pfree(conn);
+ libpqrcv_check_conninfo(conninfo, true, conn);
+ if (!PQconnectionUsedPassword(conn->streamConn))
+ {
+ libpqsrv_disconnect(conn->streamConn);
+ pfree(conn);
- ereport(ERROR,
- (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
- errmsg("password is required"),
- errdetail("Non-superuser cannot connect if the server does not request a password."),
- errhint("Target server's authentication method must be changed, or set password_required=false in the subscription parameters.")));
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser cannot connect if the server does not request a password."),
+ errhint("Target server's authentication method must be changed, or set password_required=false in the subscription parameters.")));
+ }
}
if (logical)
@@ -239,7 +245,8 @@ bad_connection:
* connection string specifies a password and false otherwise.
*/
static void
-libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
+libpqrcv_check_conninfo(const char *conninfo, bool must_use_password,
+ WalReceiverConn *conn)
{
PQconninfoOption *opts = NULL;
PQconninfoOption *opt;
@@ -252,6 +259,12 @@ libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
char *errcopy = err ? pstrdup(err) : "out of memory";
PQfreemem(err);
+ if (conn)
+ {
+ libpqsrv_disconnect(conn->streamConn);
+ pfree(conn);
+ }
+
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid connection string syntax: %s", errcopy)));
@@ -275,10 +288,18 @@ libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
}
if (!uses_password)
+ {
+ if (conn)
+ {
+ libpqsrv_disconnect(conn->streamConn);
+ pfree(conn);
+ }
+
ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password is required"),
errdetail("Non-superusers must provide a password in the connection string.")));
+ }
}
PQconninfoFree(opts);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e2cee92cf2..7f69ddb1f6 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1267,8 +1267,8 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
char originname[NAMEDATALEN];
RepOriginId originid;
UserContext ucxt;
- bool must_use_password;
bool run_as_owner;
+ bool is_superuser = superuser_arg(MySubscription->owner);
/* Check the state of the table synchronization. */
StartTransactionCommand();
@@ -1276,10 +1276,6 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
MyLogicalRepWorker->relid,
&relstate_lsn);
- /* Is the use of a password mandatory? */
- must_use_password = MySubscription->passwordrequired &&
- !superuser_arg(MySubscription->owner);
-
/* Note that the superuser_arg call can access the DB */
CommitTransactionCommand();
@@ -1314,7 +1310,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
*/
LogRepWorkerWalRcvConn =
walrcv_connect(MySubscription->conninfo, true,
- must_use_password,
+ MySubscription->passwordrequired, is_superuser,
slotname, &err);
if (LogRepWorkerWalRcvConn == NULL)
ereport(ERROR,
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 597947410f..ddc5c63c7d 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3887,6 +3887,7 @@ maybe_reread_subscription(void)
MemoryContext oldctx;
Subscription *newsub;
bool started_tx = false;
+ bool is_superuser;
/* When cache state is valid there is nothing to do here. */
if (MySubscriptionValid)
@@ -3903,6 +3904,7 @@ maybe_reread_subscription(void)
oldctx = MemoryContextSwitchTo(ApplyContext);
newsub = GetSubscription(MyLogicalRepWorker->subid, true);
+ is_superuser = superuser_arg(MySubscription->owner);
/*
* Exit if the subscription was removed. This normally should not happen
@@ -3952,7 +3954,8 @@ maybe_reread_subscription(void)
newsub->passwordrequired != MySubscription->passwordrequired ||
strcmp(newsub->origin, MySubscription->origin) != 0 ||
newsub->owner != MySubscription->owner ||
- !equal(newsub->publications, MySubscription->publications))
+ !equal(newsub->publications, MySubscription->publications) ||
+ is_superuser != MySubscription->isownersuperuser)
{
if (am_parallel_apply_worker())
ereport(LOG,
@@ -4468,7 +4471,7 @@ run_apply_worker()
RepOriginId originid;
TimeLineID startpointTLI;
char *err;
- bool must_use_password;
+ bool is_superuser = superuser_arg(MySubscription->owner);
slotname = MySubscription->slotname;
@@ -4493,15 +4496,12 @@ run_apply_worker()
replorigin_session_origin = originid;
origin_startpos = replorigin_session_get_progress(false);
- /* Is the use of a password mandatory? */
- must_use_password = MySubscription->passwordrequired &&
- !superuser_arg(MySubscription->owner);
-
/* Note that the superuser_arg call can access the DB */
CommitTransactionCommand();
LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
- must_use_password,
+ MySubscription->passwordrequired,
+ is_superuser,
MySubscription->name, &err);
if (LogRepWorkerWalRcvConn == NULL)
@@ -4605,6 +4605,13 @@ InitializeLogRepWorker(void)
proc_exit(0);
}
+ /*
+ * Fetch subscription owner is a superuser. This value will be later
+ * checked to see when there is any change with this role and the worker
+ * will be restarted if required.
+ */
+ MySubscription->isownersuperuser = superuser_arg(MySubscription->owner);
+
MySubscriptionValid = true;
MemoryContextSwitchTo(oldctx);
@@ -4626,6 +4633,10 @@ InitializeLogRepWorker(void)
subscription_change_cb,
(Datum) 0);
+ CacheRegisterSyscacheCallback(AUTHOID,
+ subscription_change_cb,
+ (Datum) 0);
+
if (am_tablesync_worker())
ereport(LOG,
(errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has started",
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index feff709435..896dc9bc6f 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -296,7 +296,7 @@ WalReceiverMain(void)
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
/* Establish the connection to the primary for XLOG streaming */
- wrconn = walrcv_connect(conninfo, false, false,
+ wrconn = walrcv_connect(conninfo, false, false, true,
cluster_name[0] ? cluster_name : "walreceiver",
&err);
if (!wrconn)
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index be36c4a820..87f6f644a9 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -144,6 +144,7 @@ typedef struct Subscription
List *publications; /* List of publication names to subscribe to */
char *origin; /* Only publish data originating from the
* specified origin */
+ bool isownersuperuser; /* Is subscription owner superuser? */
} Subscription;
/* Disallow streaming in-progress transactions. */
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index 281626fa6f..22ed9dc8ff 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -239,7 +239,8 @@ typedef struct WalRcvExecResult
*/
typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo,
bool logical,
- bool must_use_password,
+ bool passwordrequired,
+ bool is_superuser,
const char *appname,
char **err);
@@ -249,7 +250,8 @@ typedef WalReceiverConn *(*walrcv_connect_fn) (const char *conninfo,
* Parse and validate the connection string given as of 'conninfo'.
*/
typedef void (*walrcv_check_conninfo_fn) (const char *conninfo,
- bool must_use_password);
+ bool must_use_password,
+ WalReceiverConn *conn);
/*
* walrcv_get_conninfo_fn
@@ -407,10 +409,10 @@ typedef struct WalReceiverFunctionsType
extern PGDLLIMPORT WalReceiverFunctionsType *WalReceiverFunctions;
-#define walrcv_connect(conninfo, logical, must_use_password, appname, err) \
- WalReceiverFunctions->walrcv_connect(conninfo, logical, must_use_password, appname, err)
-#define walrcv_check_conninfo(conninfo, must_use_password) \
- WalReceiverFunctions->walrcv_check_conninfo(conninfo, must_use_password)
+#define walrcv_connect(conninfo, logical, passwordrequired, is_superuser, appname, err) \
+ WalReceiverFunctions->walrcv_connect(conninfo, logical, passwordrequired, is_superuser, appname, err)
+#define walrcv_check_conninfo(conninfo, must_use_password, conn) \
+ WalReceiverFunctions->walrcv_check_conninfo(conninfo, must_use_password, conn)
#define walrcv_get_conninfo(conn) \
WalReceiverFunctions->walrcv_get_conninfo(conn)
#define walrcv_get_senderinfo(conn, sender_host, sender_port) \
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5b..bb34db8843 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -7,7 +7,8 @@ use warnings;
use PostgreSQL::Test::Cluster;
use Test::More;
-my ($node_publisher, $node_subscriber, $publisher_connstr, $result, $offset);
+my ($node_publisher, $node_subscriber, $publisher_connstr, $result, $offset,
+ $stdout, $stderr);
$offset = 0;
sub publish_insert
@@ -104,6 +105,7 @@ for my $node ($node_publisher, $node_subscriber)
CREATE ROLE regress_admin SUPERUSER LOGIN;
CREATE ROLE regress_alice NOSUPERUSER LOGIN;
GRANT CREATE ON DATABASE postgres TO regress_alice;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_alice;
SET SESSION AUTHORIZATION regress_alice;
CREATE SCHEMA alice;
GRANT USAGE ON SCHEMA alice TO regress_admin;
@@ -303,4 +305,33 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
expect_replication("alice.unpartitioned", 3, 17, 21,
"restoring SELECT permission permits replication to continue");
+# Creation of subscription should fail for non superuser alice with
+# 'password is required' error.
+($result, $stdout, $stderr) = $node_subscriber->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+CREATE SUBSCRIPTION regression_sub1 CONNECTION '$publisher_connstr' PUBLICATION alice;
+));
+ok( $stderr =~ qr/ERROR: ( [A-Z0-9]+:)? password is required/,
+ "non superuser must specify password to connect");
+
+# The apply worker should get restarted after the superuser prvileges are
+# revoked for subscription owner alice.
+grant_superuser("regress_alice");
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+CREATE SUBSCRIPTION regression_sub1 CONNECTION '$publisher_connstr' PUBLICATION alice;
+));
+
+# Check the subscriber log from now on.
+$offset = -s $node_subscriber->logfile;
+
+revoke_superuser("regress_alice");
+
+# After apply worker is restarted, it should fail with 'password is required'
+# error.
+$node_subscriber->wait_for_log(qr/ERROR: ( [A-Z0-9]+:)? password is required/,
+ $offset);
+
done_testing();
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-21 20:15 Robert Haas <robertmhaas@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Robert Haas @ 2023-09-21 20:15 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Thu, Sep 21, 2023 at 3:48 PM vignesh C <vignesh21@gmail.com> wrote:
> Attached patch has the changes for the same.
Almost everything about this patch seems incorrect to me.
It seems to rip out all of the must_use_password = passwordrequired &&
!superuser logic, which is not at all what was being discussed here,
and which I think is not desirable.
And it does stuff like this:
@@ -275,10 +288,18 @@ libpqrcv_check_conninfo(const char *conninfo,
bool must_use_password)
}
if (!uses_password)
+ {
+ if (conn)
+ {
+ libpqsrv_disconnect(conn->streamConn);
+ pfree(conn);
+ }
+
ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password is required"),
errdetail("Non-superusers must provide a password in the connection
string.")));
+ }
}
PQconninfoFree(opts);
There are zero comments explaining what this is supposed to
accomplish, but I don't think it's any of the things discussed on this
thread.
+ CacheRegisterSyscacheCallback(AUTHOID,
+ subscription_change_cb,
+ (Datum) 0);
I think if we want to do this it should be a separate patch from
adding the additional error checks. And I think it should be
accompanied by a comment update.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-22 09:32 vignesh C <vignesh21@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: vignesh C @ 2023-09-22 09:32 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Fri, 22 Sept 2023 at 01:45, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Sep 21, 2023 at 3:48 PM vignesh C <vignesh21@gmail.com> wrote:
> > Attached patch has the changes for the same.
>
> Almost everything about this patch seems incorrect to me.
>
> It seems to rip out all of the must_use_password = passwordrequired &&
> !superuser logic, which is not at all what was being discussed here,
> and which I think is not desirable.
I thought of moving the passwordrequired && !superuser logic inside
libpq connect but it is not simplifying the code. Reverted those
changes and kept only the changes to check if the password is present
in the connection string in case of must_use_password.
> And it does stuff like this:
>
> @@ -275,10 +288,18 @@ libpqrcv_check_conninfo(const char *conninfo,
> bool must_use_password)
> }
>
> if (!uses_password)
> + {
> + if (conn)
> + {
> + libpqsrv_disconnect(conn->streamConn);
> + pfree(conn);
> + }
> +
> ereport(ERROR,
> (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> errmsg("password is required"),
> errdetail("Non-superusers must provide a password in the connection
> string.")));
> + }
> }
>
> PQconninfoFree(opts);
>
> There are zero comments explaining what this is supposed to
> accomplish, but I don't think it's any of the things discussed on this
> thread.
We can have this check before the connection, so this can be removed.
> + CacheRegisterSyscacheCallback(AUTHOID,
> + subscription_change_cb,
> + (Datum) 0);
>
> I think if we want to do this it should be a separate patch from
> adding the additional error checks. And I think it should be
> accompanied by a comment update.
I will post these changes in a separate email.
The attached v2 version patch has the changes for the same.
Regards,
Vignesh
Attachments:
v2-0001-Make-sure-that-the-password-option-is-provided-fo.patch (text/x-patch, 1.4K) download
From 1c0724fdf78e8ca74aa22cf03a7a76ab91e0d700 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Fri, 22 Sep 2023 12:27:34 +0530
Subject: [PATCH v2] Make sure that the password option is provided for
non-superusers when password required is true.
Password can be set from PGPASS file, for non-superusers we should
make sure that the password option is provided when password required
is true before subscriber connects to the publisher.
---
.../replication/libpqwalreceiver/libpqwalreceiver.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..76ceaf2019 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -137,6 +137,14 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
const char *vals[6];
int i = 0;
+ /*
+ * Check if the password is specified as part of connection string itself
+ * for non-superusers. This check is required to prevent password being
+ * set from PGPASS file in case of non-superusers.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
/*
* We use the expand_dbname parameter to process the connection string (or
* URI), and pass some extra options.
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-22 18:55 vignesh C <vignesh21@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 0 replies; 25+ messages in thread
From: vignesh C @ 2023-09-22 18:55 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Fri, 22 Sept 2023 at 15:02, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 22 Sept 2023 at 01:45, Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Thu, Sep 21, 2023 at 3:48 PM vignesh C <vignesh21@gmail.com> wrote:
> > > Attached patch has the changes for the same.
> >
> > Almost everything about this patch seems incorrect to me.
> >
> > It seems to rip out all of the must_use_password = passwordrequired &&
> > !superuser logic, which is not at all what was being discussed here,
> > and which I think is not desirable.
>
> I thought of moving the passwordrequired && !superuser logic inside
> libpq connect but it is not simplifying the code. Reverted those
> changes and kept only the changes to check if the password is present
> in the connection string in case of must_use_password.
>
> > And it does stuff like this:
> >
> > @@ -275,10 +288,18 @@ libpqrcv_check_conninfo(const char *conninfo,
> > bool must_use_password)
> > }
> >
> > if (!uses_password)
> > + {
> > + if (conn)
> > + {
> > + libpqsrv_disconnect(conn->streamConn);
> > + pfree(conn);
> > + }
> > +
> > ereport(ERROR,
> > (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
> > errmsg("password is required"),
> > errdetail("Non-superusers must provide a password in the connection
> > string.")));
> > + }
> > }
> >
> > PQconninfoFree(opts);
> >
> > There are zero comments explaining what this is supposed to
> > accomplish, but I don't think it's any of the things discussed on this
> > thread.
>
> We can have this check before the connection, so this can be removed.
>
> > + CacheRegisterSyscacheCallback(AUTHOID,
> > + subscription_change_cb,
> > + (Datum) 0);
> >
> > I think if we want to do this it should be a separate patch from
> > adding the additional error checks. And I think it should be
> > accompanied by a comment update.
>
> I will post these changes in a separate email.
Posted this changes to hackers at [1]
[1] - https://www.postgresql.org/message-id/CALDaNm2Dxmhq08nr4P6G%2B24QvdBo_GAVyZ_Q1TcGYK%2B8NHs9xw%40mail...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-09-24 18:51 vignesh C <vignesh21@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: vignesh C @ 2023-09-24 18:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Fri, 22 Sept 2023 at 15:02, vignesh C <vignesh21@gmail.com> wrote:
> The attached v2 version patch has the changes for the same.
Added a test for the issue, the attached v3 patch has the changes for the same.
Regards,
Vignesh
Attachments:
v3-0001-Make-sure-that-the-password-option-is-provided-fr.patch (text/x-patch, 4.5K) download
From 0931a8ce0cbf71efb1d9f359a4de749163081ae9 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Fri, 22 Sep 2023 12:27:34 +0530
Subject: [PATCH v3] Make sure that the password option is provided from
connection string for non-superusers when password required is true.
Password can be specified from PGPASS file or PGPASSWORD environment,
when password required is true then for non-superusers we should make sure that
the password option is provided from connection string.
---
.../libpqwalreceiver/libpqwalreceiver.c | 8 +++
src/test/subscription/t/027_nosuperuser.pl | 71 +++++++++++++++++++
2 files changed, 79 insertions(+)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..e0d7bfc1fa 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -137,6 +137,14 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
const char *vals[6];
int i = 0;
+ /*
+ * Check if the password is specified as part of connection string itself
+ * for non-superusers. This check is required to prevent password being
+ * set from PGPASSWORD environment or PGPASS file in case of non-superusers.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
/*
* We use the expand_dbname parameter to process the connection string (or
* URI), and pass some extra options.
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5b..3a85687fff 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -303,4 +303,75 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
expect_replication("alice.unpartitioned", 3, 17, 21,
"restoring SELECT permission permits replication to continue");
+# If the superuser owned subscription which was using a connection string
+# (without password) with the password coming from the PGPASSWORD environment
+# or PGPASS FILE transfers ownership to a non-superuser, then the next
+# subscription command(which connects to the publisher) should fail with
+# password required error.
+$ENV{"PGPASSWORD"} = 'secret';
+
+my $node_publisher1 = PostgreSQL::Test::Cluster->new('publisher1');
+my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
+$node_publisher1->init(allows_streaming => 'logical');
+$node_subscriber1->init;
+$node_publisher1->start;
+$node_subscriber1->start;
+my $publisher_connstr1 =
+ $node_publisher1->connstr . ' user=regress_test dbname=postgres';
+
+for my $node ($node_publisher1, $node_subscriber1)
+{
+ $node->safe_psql(
+ 'postgres', qq(
+ CREATE ROLE regress_test PASSWORD 'secret' LOGIN REPLICATION;
+ GRANT CREATE ON DATABASE postgres TO regress_test;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_test;
+ ));
+}
+
+$node_publisher1->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test;
+CREATE PUBLICATION test;
+));
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION test_sub CONNECTION '$publisher_connstr1' PUBLICATION test WITH (enabled=false);
+));
+
+$node_subscriber1->safe_psql('postgres',
+ qq(ALTER SUBSCRIPTION test_sub ENABLE;));
+
+# Wait for initial sync to finish
+$node_subscriber1->wait_for_subscription_sync($node_publisher1, 'test_sub');
+
+# Setup pg_hba configuration so that logical replication connection without
+# password is not allowed.
+unlink($node_publisher1->data_dir . '/pg_hba.conf');
+$node_publisher1->append_conf('pg_hba.conf',
+ qq{local all regress_test md5});
+$node_publisher1->reload;
+
+# Change the subscription owner to a non-superuser
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+ALTER SUBSCRIPTION test_sub OWNER TO regress_test;
+));
+
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test;
+ALTER SUBSCRIPTION test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zerof exit for subscription whose owner is a non-superuser must specify password through connection string"
+);
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password through connection string'
+);
+
+delete $ENV{"PGPASSWORD"};
+
done_testing();
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2023-10-05 02:21 Peter Smith <smithpb2250@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Peter Smith @ 2023-10-05 02:21 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
Hi Vignesh. Here are some review comments for the latest patch v3-0001
======
Commit message
1.
Password can be specified from PGPASS file or PGPASSWORD environment,
when password required is true then for non-superusers we should make sure that
the password option is provided from connection string.
~~
SUGGESTION
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the
connection string, not using a PGPASS file or PGPASSWORD environment
variable.
======
GENERAL
2. PG DOCS
Currently the CREATE SUBSCRIPTION [1] notes say:
password_required (boolean)
Specifies whether connections to the publisher made as a result of
this subscription must use password authentication. This setting is
ignored when the subscription is owned by a superuser. The default is
true. Only superusers can set this value to false.
~
Should there be more information here to say (when 'password_required'
is true) *how* non-superusers are required to specify the password?
~~~
3. Connection string option 'passfile'?
IIUC the subscription "password_required" option means that the
password must be in the connection string. I'm not sure how the other
connection string "passfile" option [2] fits into this rule. The
current patch looks like it would reject the passfile option (because
IIUC libpqrcv_check_conninfo only looks for "password") -- is it
deliberate? And, should it be documented/commented somewhere?
======
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
4.
+ /*
+ * Check if the password is specified as part of connection string itself
+ * for non-superusers. This check is required to prevent password being
+ * set from PGPASSWORD environment or PGPASS file in case of non-superusers.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
Consider using the same wording in the comment as the suggested commit message.
======
src/test/subscription/t/027_nosuperuser.pl
5.
+# If the superuser owned subscription which was using a connection string
+# (without password) with the password coming from the PGPASSWORD environment
+# or PGPASS FILE transfers ownership to a non-superuser, then the next
+# subscription command(which connects to the publisher) should fail with
+# password required error.
Below is my para-phrasing of the comment. Choose whichever you like
best, but if you keep the original please fix the spaces.
SUGGESTION
If the subscription connection requires a password ('password
required' = true) then a non-superuser must specify that password in
the connection string.
This test starts out with a superuser-owned subscription having a
connection string lacking a password -- instead, the password is
coming from the PGPASSWORD environment or PGPASS FILE. Subscription
ownership is then transferred to a non-superuser. The next
subscription command (which connects to the publisher) should fail
with a password required error.
~~~
6. GENERAL - improved names in the test...
a. I felt it might be clearer to name things slightly differently.
b. Perhaps you also wanted to prefix the pub/sub names with 'regress_'
So,
regress_test ==> regress_test_user
PUBLICATION test ==> regress_test_pub
SUBSCRIPTION test_sub ==> regress_test_sub
~~~
7.
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION test_sub CONNECTION '$publisher_connstr1'
PUBLICATION test WITH (enabled=false);
+));
+
+$node_subscriber1->safe_psql('postgres',
+ qq(ALTER SUBSCRIPTION test_sub ENABLE;));
Why does the test create the subscription with enabled=false only to
immediately enable it on the next line?
~~~
8.
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test;
+ALTER SUBSCRIPTION test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zerof exit for subscription whose owner is a non-superuser must
specify password through connection string"
+);
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password
through connection string'
+);
8a.
+# Non-superuser must specify password in the connection string
Refer to my prior question -- what about the 'passfile' connection
string option?
~
8b.
typo /zerof/zero/
~~~
9. what about positive test?
I felt this test should conclude with you changing the connection
string so it *does* include the secret password, to demonstrate the
non-superuser connecting successfully.
======
[1] https://www.postgresql.org/docs/current/sql-createsubscription.html
[2] https://www.postgresql.org/docs/devel/libpq-connect.html#LIBPQ-CONNECT-PASSFILE
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-10-05 10:49 vignesh C <vignesh21@gmail.com>
parent: Peter Smith <smithpb2250@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: vignesh C @ 2023-10-05 10:49 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Thu, 5 Oct 2023 at 07:51, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh. Here are some review comments for the latest patch v3-0001
>
> ======
> Commit message
>
> 1.
> Password can be specified from PGPASS file or PGPASSWORD environment,
> when password required is true then for non-superusers we should make sure that
> the password option is provided from connection string.
>
> ~~
>
> SUGGESTION
> When the 'password_required' subscription parameter is true, a
> non-superuser is only allowed to specify the password via the
> connection string, not using a PGPASS file or PGPASSWORD environment
> variable.
Modified
> ======
> GENERAL
>
> 2. PG DOCS
>
> Currently the CREATE SUBSCRIPTION [1] notes say:
>
> password_required (boolean)
> Specifies whether connections to the publisher made as a result of
> this subscription must use password authentication. This setting is
> ignored when the subscription is owned by a superuser. The default is
> true. Only superusers can set this value to false.
>
> ~
>
> Should there be more information here to say (when 'password_required'
> is true) *how* non-superusers are required to specify the password?
Modified
> ~~~
>
> 3. Connection string option 'passfile'?
>
> IIUC the subscription "password_required" option means that the
> password must be in the connection string. I'm not sure how the other
> connection string "passfile" option [2] fits into this rule. The
> current patch looks like it would reject the passfile option (because
> IIUC libpqrcv_check_conninfo only looks for "password") -- is it
> deliberate? And, should it be documented/commented somewhere?
>
> ======
> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
I felt we should not allow password from the passfile for
non-superusers, if password_required is true, the password can be
specified only through the password option of the connection string.
The same has been documented.
> 4.
> + /*
> + * Check if the password is specified as part of connection string itself
> + * for non-superusers. This check is required to prevent password being
> + * set from PGPASSWORD environment or PGPASS file in case of non-superusers.
> + */
> + if (must_use_password)
> + libpqrcv_check_conninfo(conninfo, true);
> +
>
> Consider using the same wording in the comment as the suggested commit message.
>
Modified
> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> 5.
> +# If the superuser owned subscription which was using a connection string
> +# (without password) with the password coming from the PGPASSWORD environment
> +# or PGPASS FILE transfers ownership to a non-superuser, then the next
> +# subscription command(which connects to the publisher) should fail with
> +# password required error.
>
> Below is my para-phrasing of the comment. Choose whichever you like
> best, but if you keep the original please fix the spaces.
>
> SUGGESTION
> If the subscription connection requires a password ('password
> required' = true) then a non-superuser must specify that password in
> the connection string.
>
> This test starts out with a superuser-owned subscription having a
> connection string lacking a password -- instead, the password is
> coming from the PGPASSWORD environment or PGPASS FILE. Subscription
> ownership is then transferred to a non-superuser. The next
> subscription command (which connects to the publisher) should fail
> with a password required error.
Modified
> ~~~
>
> 6. GENERAL - improved names in the test...
>
> a. I felt it might be clearer to name things slightly differently.
> b. Perhaps you also wanted to prefix the pub/sub names with 'regress_'
>
> So,
>
> regress_test ==> regress_test_user
>
> PUBLICATION test ==> regress_test_pub
>
> SUBSCRIPTION test_sub ==> regress_test_sub
Modified
> ~~~
>
> 7.
> +$node_subscriber1->safe_psql(
> + 'postgres', qq(
> +CREATE SUBSCRIPTION test_sub CONNECTION '$publisher_connstr1'
> PUBLICATION test WITH (enabled=false);
> +));
> +
> +$node_subscriber1->safe_psql('postgres',
> + qq(ALTER SUBSCRIPTION test_sub ENABLE;));
>
> Why does the test create the subscription with enabled=false only to
> immediately enable it on the next line?
Modified
> ~~~
>
> 8.
> +# Non-superuser must specify password in the connection string
> +my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
> + 'postgres', qq(
> +SET SESSION AUTHORIZATION regress_test;
> +ALTER SUBSCRIPTION test_sub REFRESH PUBLICATION;
> +));
> +isnt($ret, 0,
> + "non zerof exit for subscription whose owner is a non-superuser must
> specify password through connection string"
> +);
> +is( $stderr, 'psql:<stdin>:3: ERROR: password is required
> +DETAIL: Non-superusers must provide a password in the connection string.',
> + 'subscription whose owner is a non-superuser must specify password
> through connection string'
> +);
>
> 8a.
> +# Non-superuser must specify password in the connection string
>
> Refer to my prior question -- what about the 'passfile' connection
> string option?
Password from the passfile are not allowed, it should be only from the
connection string.
> ~
>
> 8b.
> typo /zerof/zero/
Modified
> ~~~
>
> 9. what about positive test?
>
> I felt this test should conclude with you changing the connection
> string so it *does* include the secret password, to demonstrate the
> non-superuser connecting successfully.
Added a test for this
Thanks for the comments, the attached patch has the changes for the same.
Regards,
Vignesh
Attachments:
v4-0001-A-non-superuser-is-only-allowed-to-specify-the-pa.patch (text/x-patch, 6.0K) download
From 91a20685a428b4c8f3d988d52e104479af17cd2d Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Fri, 22 Sep 2023 12:27:34 +0530
Subject: [PATCH v4] A non-superuser is only allowed to specify the password
via the password option of connection string.
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the password
option of connection string, not using a PGPASS file or PGPASSWORD
environment variable.
---
doc/src/sgml/ref/create_subscription.sgml | 10 ++-
.../libpqwalreceiver/libpqwalreceiver.c | 8 ++
src/test/subscription/t/027_nosuperuser.pl | 80 +++++++++++++++++++
3 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 71652fd918..082fafd3c9 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -358,10 +358,12 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<listitem>
<para>
Specifies whether connections to the publisher made as a result
- of this subscription must use password authentication. This setting
- is ignored when the subscription is owned by a superuser.
- The default is <literal>true</literal>. Only superusers can set
- this value to <literal>false</literal>.
+ of this subscription must use password authentication. If
+ <literal>true</literal>, a non-superuser is only allowed to specify
+ the password via the password option in connection string. This
+ setting is ignored when the subscription is owned by a superuser. The
+ default is <literal>true</literal>. Only superusers can set this
+ value to <literal>false</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..d050ffee85 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -137,6 +137,14 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
const char *vals[6];
int i = 0;
+ /*
+ * A non-superuser is only allowed to specify the password via the
+ * password option in connection string, not using a PGPASS file or
+ * PGPASSWORD environment variable.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
/*
* We use the expand_dbname parameter to process the connection string (or
* URI), and pass some extra options.
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5b..c9e1da4b86 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -303,4 +303,84 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
expect_replication("alice.unpartitioned", 3, 17, 21,
"restoring SELECT permission permits replication to continue");
+# If the subscription connection requires a password ('password_required'
+# is true) then a non-superuser must specify that password in the connection
+# string.
+$ENV{"PGPASSWORD"} = 'secret';
+
+my $node_publisher1 = PostgreSQL::Test::Cluster->new('publisher1');
+my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
+$node_publisher1->init(allows_streaming => 'logical');
+$node_subscriber1->init;
+$node_publisher1->start;
+$node_subscriber1->start;
+my $publisher_connstr1 =
+ $node_publisher1->connstr . ' user=regress_test_user dbname=postgres';
+my $publisher_connstr2 =
+ $node_publisher1->connstr
+ . ' user=regress_test_user dbname=postgres password=secret';
+
+for my $node ($node_publisher1, $node_subscriber1)
+{
+ $node->safe_psql(
+ 'postgres', qq(
+ CREATE ROLE regress_test_user PASSWORD 'secret' LOGIN REPLICATION;
+ GRANT CREATE ON DATABASE postgres TO regress_test_user;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_test_user;
+ ));
+}
+
+$node_publisher1->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+CREATE PUBLICATION regress_test_pub;
+));
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr1' PUBLICATION regress_test_pub;
+));
+
+# Wait for initial sync to finish
+$node_subscriber1->wait_for_subscription_sync($node_publisher1,
+ 'regress_test_sub');
+
+# Setup pg_hba configuration so that logical replication connection without
+# password is not allowed.
+unlink($node_publisher1->data_dir . '/pg_hba.conf');
+$node_publisher1->append_conf('pg_hba.conf',
+ qq{local all regress_test_user md5});
+$node_publisher1->reload;
+
+# Change the subscription owner to a non-superuser
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+ALTER SUBSCRIPTION regress_test_sub OWNER TO regress_test_user;
+));
+
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zero exit for subscription whose owner is a non-superuser must specify password through connection string"
+);
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password through connection string'
+);
+
+delete $ENV{"PGPASSWORD"};
+
+# It should be successful after including the password in connection string
+($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr2';
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+is($ret, 0,
+ "Non-superuser will be able to refresh the publication when the password is specified in connection string"
+);
done_testing();
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2023-10-06 01:13 Peter Smith <smithpb2250@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Peter Smith @ 2023-10-06 01:13 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
Here are some review comments for v4-0001
======
1. GENERAL - terminology
AFAICT from existing documentation, things like 'password' are
referred to as "parameters" of the connection string (i.e. not
"options" of the connection string), so most review comments below are
just repetitions of this point to make all the wording consistent.
======
Commit Message
2.
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the password
option of connection string, not using a PGPASS file or PGPASSWORD
environment variable.
~
(change the term and add the missing word 'the')
/the password option of connection string/the password parameter of
the connection string/
======
doc/src/sgml/ref/create_subscription.sgml
3.
<para>
Specifies whether connections to the publisher made as a result
- of this subscription must use password authentication. This setting
- is ignored when the subscription is owned by a superuser.
- The default is <literal>true</literal>. Only superusers can set
- this value to <literal>false</literal>.
+ of this subscription must use password authentication. If
+ <literal>true</literal>, a non-superuser is only allowed to specify
+ the password via the password option in connection string. This
+ setting is ignored when the subscription is owned by a superuser. The
+ default is <literal>true</literal>. Only superusers can set this
+ value to <literal>false</literal>.
</para>
3a.
(same wording as commit message)
/the password option in connection string/the password parameter of
the connection string/
~
3b.
Also, consider using a link, or at least a <literal> for that
"password" parameter.
======
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
4. libpqrcv_connect
+ /*
+ * A non-superuser is only allowed to specify the password via the
+ * password option in connection string, not using a PGPASS file or
+ * PGPASSWORD environment variable.
+ */
(same wording as commit message)
/the password option in connection string/the password parameter of
the connection string/
======
src/test/subscription/t/027_nosuperuser.pl
5.
+isnt($ret, 0,
+ "non zero exit for subscription whose owner is a non-superuser must
specify password through connection string"
+);
(same wording as commit message)
/password through connection string/password parameter of the connection string/
~~~
6.
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password
through connection string'
(same wording as commit message)
/password through connection string/password parameter of the connection string/
~~~
7.
+# It should be successful after including the password in connection string
/It should be successful/It should succeed/
~
(same wording as commit message)
/password in connection string/password parameter of the connection string/
~~~
8.
+is($ret, 0,
+ "Non-superuser will be able to refresh the publication when the
password is specified in connection string"
+);
(similar wording as commit message)
/when the password is specified in connection string/after specifying
the password parameter of the connection string/
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-10-06 06:38 vignesh C <vignesh21@gmail.com>
parent: Peter Smith <smithpb2250@gmail.com>
0 siblings, 0 replies; 25+ messages in thread
From: vignesh C @ 2023-10-06 06:38 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Fri, 6 Oct 2023 at 06:43, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v4-0001
>
> ======
> 1. GENERAL - terminology
>
> AFAICT from existing documentation, things like 'password' are
> referred to as "parameters" of the connection string (i.e. not
> "options" of the connection string), so most review comments below are
> just repetitions of this point to make all the wording consistent.
Modified
> ======
> Commit Message
>
> 2.
> When the 'password_required' subscription parameter is true, a
> non-superuser is only allowed to specify the password via the password
> option of connection string, not using a PGPASS file or PGPASSWORD
> environment variable.
>
> ~
>
> (change the term and add the missing word 'the')
>
> /the password option of connection string/the password parameter of
> the connection string/
Modified
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 3.
> <para>
> Specifies whether connections to the publisher made as a result
> - of this subscription must use password authentication. This setting
> - is ignored when the subscription is owned by a superuser.
> - The default is <literal>true</literal>. Only superusers can set
> - this value to <literal>false</literal>.
> + of this subscription must use password authentication. If
> + <literal>true</literal>, a non-superuser is only allowed to specify
> + the password via the password option in connection string. This
> + setting is ignored when the subscription is owned by a superuser. The
> + default is <literal>true</literal>. Only superusers can set this
> + value to <literal>false</literal>.
> </para>
>
> 3a.
>
> (same wording as commit message)
>
> /the password option in connection string/the password parameter of
> the connection string/
Modified
> ~
>
> 3b.
> Also, consider using a link, or at least a <literal> for that
> "password" parameter.
Modified
> ======
> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>
> 4. libpqrcv_connect
>
> + /*
> + * A non-superuser is only allowed to specify the password via the
> + * password option in connection string, not using a PGPASS file or
> + * PGPASSWORD environment variable.
> + */
>
> (same wording as commit message)
>
> /the password option in connection string/the password parameter of
> the connection string/
Modified
> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> 5.
> +isnt($ret, 0,
> + "non zero exit for subscription whose owner is a non-superuser must
> specify password through connection string"
> +);
>
> (same wording as commit message)
>
> /password through connection string/password parameter of the connection string/
Modified
> ~~~
>
> 6.
> +is( $stderr, 'psql:<stdin>:3: ERROR: password is required
> +DETAIL: Non-superusers must provide a password in the connection string.',
> + 'subscription whose owner is a non-superuser must specify password
> through connection string'
>
> (same wording as commit message)
>
> /password through connection string/password parameter of the connection string/
Modified
> ~~~
>
> 7.
> +# It should be successful after including the password in connection string
>
> /It should be successful/It should succeed/
>
> ~
>
> (same wording as commit message)
>
> /password in connection string/password parameter of the connection string/
Modified
> ~~~
>
> 8.
> +is($ret, 0,
> + "Non-superuser will be able to refresh the publication when the
> password is specified in connection string"
> +);
>
> (similar wording as commit message)
>
> /when the password is specified in connection string/after specifying
> the password parameter of the connection string/
Modified
The attached v5 version patch has the changes for the same.
Regards,
Vignesh
Attachments:
v5-0001-A-non-superuser-is-only-allowed-to-specify-the-pa.patch (text/x-patch, 6.1K) download
From 7999c05c9e9dcca000c1c0b5dbd062ac7d1f1a63 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Fri, 22 Sep 2023 12:27:34 +0530
Subject: [PATCH v5] A non-superuser is only allowed to specify the password
via the password parameter of the connection string.
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the password
parameter of the connection string, not using a PGPASS file or PGPASSWORD
environment variable.
---
doc/src/sgml/ref/create_subscription.sgml | 10 ++-
.../libpqwalreceiver/libpqwalreceiver.c | 8 ++
src/test/subscription/t/027_nosuperuser.pl | 81 +++++++++++++++++++
3 files changed, 95 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 71652fd918..d38d0001cc 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -358,10 +358,12 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<listitem>
<para>
Specifies whether connections to the publisher made as a result
- of this subscription must use password authentication. This setting
- is ignored when the subscription is owned by a superuser.
- The default is <literal>true</literal>. Only superusers can set
- this value to <literal>false</literal>.
+ of this subscription must use password authentication. If
+ <literal>true</literal>, a non-superuser is only allowed to specify
+ the password via the <literal>password</literal> parameter of the
+ connection string. This setting is ignored when the subscription is
+ owned by a superuser. The default is <literal>true</literal>. Only
+ superusers can set this value to <literal>false</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..47f7a9ffd9 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -137,6 +137,14 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
const char *vals[6];
int i = 0;
+ /*
+ * A non-superuser is only allowed to specify the password via the password
+ * parameter of the connection string, not using a PGPASS file or
+ * PGPASSWORD environment variable.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
/*
* We use the expand_dbname parameter to process the connection string (or
* URI), and pass some extra options.
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5b..6ed72028d3 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -303,4 +303,85 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
expect_replication("alice.unpartitioned", 3, 17, 21,
"restoring SELECT permission permits replication to continue");
+# If the subscription connection requires a password ('password_required'
+# is true) then a non-superuser must specify that password in the connection
+# string.
+$ENV{"PGPASSWORD"} = 'secret';
+
+my $node_publisher1 = PostgreSQL::Test::Cluster->new('publisher1');
+my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
+$node_publisher1->init(allows_streaming => 'logical');
+$node_subscriber1->init;
+$node_publisher1->start;
+$node_subscriber1->start;
+my $publisher_connstr1 =
+ $node_publisher1->connstr . ' user=regress_test_user dbname=postgres';
+my $publisher_connstr2 =
+ $node_publisher1->connstr
+ . ' user=regress_test_user dbname=postgres password=secret';
+
+for my $node ($node_publisher1, $node_subscriber1)
+{
+ $node->safe_psql(
+ 'postgres', qq(
+ CREATE ROLE regress_test_user PASSWORD 'secret' LOGIN REPLICATION;
+ GRANT CREATE ON DATABASE postgres TO regress_test_user;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_test_user;
+ ));
+}
+
+$node_publisher1->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+CREATE PUBLICATION regress_test_pub;
+));
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr1' PUBLICATION regress_test_pub;
+));
+
+# Wait for initial sync to finish
+$node_subscriber1->wait_for_subscription_sync($node_publisher1,
+ 'regress_test_sub');
+
+# Setup pg_hba configuration so that logical replication connection without
+# password is not allowed.
+unlink($node_publisher1->data_dir . '/pg_hba.conf');
+$node_publisher1->append_conf('pg_hba.conf',
+ qq{local all regress_test_user md5});
+$node_publisher1->reload;
+
+# Change the subscription owner to a non-superuser
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+ALTER SUBSCRIPTION regress_test_sub OWNER TO regress_test_user;
+));
+
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zero exit for subscription whose owner is a non-superuser must specify password parameter of the connection string"
+);
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password parameter of the connection string'
+);
+
+delete $ENV{"PGPASSWORD"};
+
+# It should succeed after including the password parameter of the connection
+# string.
+($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr2';
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+is($ret, 0,
+ "Non-superuser will be able to refresh the publication after specifying the password parameter of the connection string"
+);
done_testing();
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2023-10-06 06:55 Peter Smith <smithpb2250@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Peter Smith @ 2023-10-06 06:55 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Thu, Oct 5, 2023 at 9:50 PM vignesh C <vignesh21@gmail.com> wrote:
>
> > 3. Connection string option 'passfile'?
> >
> > IIUC the subscription "password_required" option means that the
> > password must be in the connection string. I'm not sure how the other
> > connection string "passfile" option [2] fits into this rule. The
> > current patch looks like it would reject the passfile option (because
> > IIUC libpqrcv_check_conninfo only looks for "password") -- is it
> > deliberate? And, should it be documented/commented somewhere?
> >
> > ======
> > src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>
> I felt we should not allow password from the passfile for
> non-superusers, if password_required is true, the password can be
> specified only through the password option of the connection string.
> The same has been documented.
>
Assuming that everybody is happy with the above decision to only allow
the 'password' parameter, then your latest v5-0001 patch looked good
to me.
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2023-11-27 04:56 vignesh C <vignesh21@gmail.com>
parent: Peter Smith <smithpb2250@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: vignesh C @ 2023-11-27 04:56 UTC (permalink / raw)
To: Peter Smith <smithpb2250@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Fri, 6 Oct 2023 at 12:26, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Oct 5, 2023 at 9:50 PM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> > > 3. Connection string option 'passfile'?
> > >
> > > IIUC the subscription "password_required" option means that the
> > > password must be in the connection string. I'm not sure how the other
> > > connection string "passfile" option [2] fits into this rule. The
> > > current patch looks like it would reject the passfile option (because
> > > IIUC libpqrcv_check_conninfo only looks for "password") -- is it
> > > deliberate? And, should it be documented/commented somewhere?
> > >
> > > ======
> > > src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
> >
> > I felt we should not allow password from the passfile for
> > non-superusers, if password_required is true, the password can be
> > specified only through the password option of the connection string.
> > The same has been documented.
> >
>
> Assuming that everybody is happy with the above decision to only allow
> the 'password' parameter, then your latest v5-0001 patch looked good
> to me.
The patch was not applying on HEAD because of recent commits, the
attached v6 version patch is rebased on top of HEAD.
Regards,
Vignesh
Attachments:
v6-0001-A-non-superuser-is-only-allowed-to-specify-the-pa.patch (text/x-patch, 6.4K) download
From 7f588672299586de5f74aa76ef96d1138da5617e Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 27 Nov 2023 08:46:12 +0530
Subject: [PATCH v6] A non-superuser is only allowed to specify the password
via the password parameter of the connection string.
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the password
parameter of the connection string, not using a PGPASS file or PGPASSWORD
environment variable.
Reported-by: Jeff Davis
Author: Vignesh C
Reviewed-by: Peter Smith, Robert Haas, Amit Kapila
Discussion: https://www.postgresql.org/message-id/flat/e5892973ae2a80a1a3e0266806640dae3c428100.camel%40j-davis.com
---
doc/src/sgml/ref/create_subscription.sgml | 10 ++-
.../libpqwalreceiver/libpqwalreceiver.c | 8 ++
src/test/subscription/t/027_nosuperuser.pl | 81 +++++++++++++++++++
3 files changed, 95 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index f1c20b3a46..82ec7c4291 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -358,10 +358,12 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<listitem>
<para>
Specifies whether connections to the publisher made as a result
- of this subscription must use password authentication. This setting
- is ignored when the subscription is owned by a superuser.
- The default is <literal>true</literal>. Only superusers can set
- this value to <literal>false</literal>.
+ of this subscription must use password authentication. If
+ <literal>true</literal>, a non-superuser is only allowed to specify
+ the password via the <literal>password</literal> parameter of the
+ connection string. This setting is ignored when the subscription is
+ owned by a superuser. The default is <literal>true</literal>. Only
+ superusers can set this value to <literal>false</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..47f7a9ffd9 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -137,6 +137,14 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
const char *vals[6];
int i = 0;
+ /*
+ * A non-superuser is only allowed to specify the password via the password
+ * parameter of the connection string, not using a PGPASS file or
+ * PGPASSWORD environment variable.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+
/*
* We use the expand_dbname parameter to process the connection string (or
* URI), and pass some extra options.
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index 642baa5d7c..2a62e05472 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -327,4 +327,85 @@ $node_subscriber->wait_for_log(
qr/LOG: ( [A-Z0-9]+:)? logical replication worker for subscription \"regression_sub\" will restart because the subscription owner's superuser privileges have been revoked/,
$offset);
+# If the subscription connection requires a password ('password_required'
+# is true) then a non-superuser must specify that password in the connection
+# string.
+$ENV{"PGPASSWORD"} = 'secret';
+
+my $node_publisher1 = PostgreSQL::Test::Cluster->new('publisher1');
+my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
+$node_publisher1->init(allows_streaming => 'logical');
+$node_subscriber1->init;
+$node_publisher1->start;
+$node_subscriber1->start;
+my $publisher_connstr1 =
+ $node_publisher1->connstr . ' user=regress_test_user dbname=postgres';
+my $publisher_connstr2 =
+ $node_publisher1->connstr
+ . ' user=regress_test_user dbname=postgres password=secret';
+
+for my $node ($node_publisher1, $node_subscriber1)
+{
+ $node->safe_psql(
+ 'postgres', qq(
+ CREATE ROLE regress_test_user PASSWORD 'secret' LOGIN REPLICATION;
+ GRANT CREATE ON DATABASE postgres TO regress_test_user;
+ GRANT PG_CREATE_SUBSCRIPTION TO regress_test_user;
+ ));
+}
+
+$node_publisher1->safe_psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+CREATE PUBLICATION regress_test_pub;
+));
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr1' PUBLICATION regress_test_pub;
+));
+
+# Wait for initial sync to finish
+$node_subscriber1->wait_for_subscription_sync($node_publisher1,
+ 'regress_test_sub');
+
+# Setup pg_hba configuration so that logical replication connection without
+# password is not allowed.
+unlink($node_publisher1->data_dir . '/pg_hba.conf');
+$node_publisher1->append_conf('pg_hba.conf',
+ qq{local all regress_test_user md5});
+$node_publisher1->reload;
+
+# Change the subscription owner to a non-superuser
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+ALTER SUBSCRIPTION regress_test_sub OWNER TO regress_test_user;
+));
+
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zero exit for subscription whose owner is a non-superuser must specify password parameter of the connection string"
+);
+is( $stderr, 'psql:<stdin>:3: ERROR: password is required
+DETAIL: Non-superusers must provide a password in the connection string.',
+ 'subscription whose owner is a non-superuser must specify password parameter of the connection string'
+);
+
+delete $ENV{"PGPASSWORD"};
+
+# It should succeed after including the password parameter of the connection
+# string.
+($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr2';
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+is($ret, 0,
+ "Non-superuser will be able to refresh the publication after specifying the password parameter of the connection string"
+);
done_testing();
--
2.34.1
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-03 03:51 Jeff Davis <pgsql@j-davis.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 0 replies; 25+ messages in thread
From: Jeff Davis @ 2024-01-03 03:51 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; Peter Smith <smithpb2250@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Mon, 2023-11-27 at 10:26 +0530, vignesh C wrote:
> > Assuming that everybody is happy with the above decision to only
> > allow
> > the 'password' parameter, then your latest v5-0001 patch looked
> > good
> > to me.
>
> The patch was not applying on HEAD because of recent commits, the
> attached v6 version patch is rebased on top of HEAD.
Robert, are you intending to take this or should I?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-12 21:55 Jeff Davis <pgsql@j-davis.com>
parent: vignesh C <vignesh21@gmail.com>
1 sibling, 2 replies; 25+ messages in thread
From: Jeff Davis @ 2024-01-12 21:55 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; Peter Smith <smithpb2250@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Mon, 2023-11-27 at 10:26 +0530, vignesh C wrote:
> The patch was not applying on HEAD because of recent commits, the
> attached v6 version patch is rebased on top of HEAD.
Thank you, committed with minor modifications.
I decided to do:
libpqrcv_check_conninfo(conninfo, must_use_password);
rather than:
if (must_use_password)
libpqrcv_check_conninfo(conninfo, true);
because if there's some parsing error then we wouldn't like it to be
conditional on must_use_password. Of course, there should not be any
parsing error because it was already validated at DDL time, but in case
there's another way to get into an inconsistent state, we might as well
re-validate here.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-15 16:17 vignesh C <vignesh21@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 25+ messages in thread
From: vignesh C @ 2024-01-15 16:17 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Sat, 13 Jan 2024 at 03:25, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2023-11-27 at 10:26 +0530, vignesh C wrote:
> > The patch was not applying on HEAD because of recent commits, the
> > attached v6 version patch is rebased on top of HEAD.
>
> Thank you, committed with minor modifications.
Thanks for committing this patch.
Regards,
Vignesh
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-17 10:01 vignesh C <vignesh21@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 25+ messages in thread
From: vignesh C @ 2024-01-17 10:01 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs
On Sat, 13 Jan 2024 at 03:25, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2023-11-27 at 10:26 +0530, vignesh C wrote:
> > The patch was not applying on HEAD because of recent commits, the
> > attached v6 version patch is rebased on top of HEAD.
>
> Thank you, committed with minor modifications.
>
> I decided to do:
>
> libpqrcv_check_conninfo(conninfo, must_use_password);
>
> rather than:
>
> if (must_use_password)
> libpqrcv_check_conninfo(conninfo, true);
>
> because if there's some parsing error then we wouldn't like it to be
> conditional on must_use_password. Of course, there should not be any
> parsing error because it was already validated at DDL time, but in case
> there's another way to get into an inconsistent state, we might as well
> re-validate here.
There was a buildfarm failure at [1], I was able to reproduce it in my
environment. The changes suggested by Tom Lane at [2] fixes the
problem. Apart from that pg_hba.conf should have authentication
configuration for the host which does not use unix domain sockets. The
attached patch has the changes for the same.
[1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2024-01-16%2013%3A33%3A08
[2] - https://www.postgresql.org/message-id/1641594.1705374393%40sss.pgh.pa.us
Regards,
Vignesh
Attachments:
buildfarm_failure_fix.patch (application/octet-stream, 1.1K) download
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index 6783e02ca3..dd3c1e4fae 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -334,7 +334,8 @@ $ENV{"PGPASSWORD"} = 'secret';
my $node_publisher1 = PostgreSQL::Test::Cluster->new('publisher1');
my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
-$node_publisher1->init(allows_streaming => 'logical');
+$node_publisher1->init(allows_streaming => 'logical',
+ 'auth_extra' => [ '--create-role', 'regress_test_user' ]);
$node_subscriber1->init;
$node_publisher1->start;
$node_subscriber1->start;
@@ -372,7 +373,8 @@ $node_subscriber1->wait_for_subscription_sync($node_publisher1,
# password is not allowed.
unlink($node_publisher1->data_dir . '/pg_hba.conf');
$node_publisher1->append_conf('pg_hba.conf',
- qq{local all regress_test_user md5});
+ qq{local all regress_test_user md5
+host all regress_test_user 127.0.0.1/32 md5});
$node_publisher1->reload;
# Change the subscription owner to a non-superuser
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-18 01:52 Jeff Davis <pgsql@j-davis.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Davis @ 2024-01-18 01:52 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Wed, 2024-01-17 at 15:31 +0530, vignesh C wrote:
> There was a buildfarm failure at [1], I was able to reproduce it in
> my
> environment. The changes suggested by Tom Lane at [2] fixes the
> problem. Apart from that pg_hba.conf should have authentication
> configuration for the host which does not use unix domain sockets.
> The
> attached patch has the changes for the same.
I don't think adding --create-role is quite the right solution. The
point of the test is to distinguish between a password that comes from
the environment ($publisher_connstr1) vs specified in the connection
string ($publisher_connstr2).
There's no reason to use SSPI for that, and therefore no reason to
configure SSPI with --create-role. The change in your patch does allow
the connection initiated as part of CREATE SUBSCRIPTION to succeed, but
by a different path than what we need to test later anyway, so it seems
confusing to me. We might as well just get pg_hba.conf configured
correctly before issuing the CREATE SUBSCRIPTION.
We can't just add a "host" line to the pg_hba.conf, though, because
that violates this idea here (Cluster.pm):
Authentication is set up so that only the current OS user can
access the cluster. On Unix, we use Unix domain socket
connections, with the socket in a directory that's only
accessible to the current user to ensure that. On Windows, we
use SSPI authentication to ensure the same (by pg_regress
--config-auth).
The 001_password.pl test just doesn't run if !$use_unix_sockets and I
think we should do something similar.
We still need to move the pg_hba.conf changes up above the CREATE
SUBSCRIPTION.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-18 06:52 vignesh C <vignesh21@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 25+ messages in thread
From: vignesh C @ 2024-01-18 06:52 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Thu, 18 Jan 2024 at 07:22, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Wed, 2024-01-17 at 15:31 +0530, vignesh C wrote:
> > There was a buildfarm failure at [1], I was able to reproduce it in
> > my
> > environment. The changes suggested by Tom Lane at [2] fixes the
> > problem. Apart from that pg_hba.conf should have authentication
> > configuration for the host which does not use unix domain sockets.
> > The
> > attached patch has the changes for the same.
>
> I don't think adding --create-role is quite the right solution. The
> point of the test is to distinguish between a password that comes from
> the environment ($publisher_connstr1) vs specified in the connection
> string ($publisher_connstr2).
>
> There's no reason to use SSPI for that, and therefore no reason to
> configure SSPI with --create-role. The change in your patch does allow
> the connection initiated as part of CREATE SUBSCRIPTION to succeed, but
> by a different path than what we need to test later anyway, so it seems
> confusing to me. We might as well just get pg_hba.conf configured
> correctly before issuing the CREATE SUBSCRIPTION.
>
> We can't just add a "host" line to the pg_hba.conf, though, because
> that violates this idea here (Cluster.pm):
>
> Authentication is set up so that only the current OS user can
> access the cluster. On Unix, we use Unix domain socket
> connections, with the socket in a directory that's only
> accessible to the current user to ensure that. On Windows, we
> use SSPI authentication to ensure the same (by pg_regress
> --config-auth).
>
> The 001_password.pl test just doesn't run if !$use_unix_sockets and I
> think we should do something similar.
Yes, this seems better. I think we can just skip this particular test
case using SKIP: like in 002_database test file.
> We still need to move the pg_hba.conf changes up above the CREATE
> SUBSCRIPTION.
I felt the current test was simpler by using the default hba.conf to
CREATE SUBSCRIPTION and make the change hba config only for the actual
test verification REFRESH PUBLICATION. Using non default hba conf for
CREATE SUBSCRIPTION might require identifying the CURRENT USER like
for me it runs as vignesh that is running and setting the hba for this
user too and removing the trust for all users. We need to remove the
trust from all users as otherwise we will hit "Non-superuser cannot
connect if the server does not request a password". I did not find an
easy way to do this. Maybe you have a simpler approach for this.If you
have an easy approach do you not mind posting a patch for this?
Regards,
Vignesh
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [16+] subscription can end up in inconsistent state
@ 2024-01-18 23:24 Jeff Davis <pgsql@j-davis.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 0 replies; 25+ messages in thread
From: Jeff Davis @ 2024-01-18 23:24 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Peter Smith <smithpb2250@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; pgsql-bugs@postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
On Thu, 2024-01-18 at 12:22 +0530, vignesh C wrote:
> I felt the current test was simpler by using the default hba.conf to
> CREATE SUBSCRIPTION and make the change hba config only for the
> actual
> test verification REFRESH PUBLICATION.
OK, I left that part as-is. I just put the 3 tests into a SKIP block.
I also saved/restored the PGPASSWORD environment variable, and moved
down where PGPASSWORD is set slightly, which seemed a bit more clear.
Thank you. Let me know if you see any more problems.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2024-01-18 23:24 UTC | newest]
Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-10 15:45 [16+] subscription can end up in inconsistent state Jeff Davis <pgsql@j-davis.com>
2023-09-11 03:29 ` Amit Kapila <amit.kapila16@gmail.com>
2023-09-11 19:00 ` Jeff Davis <pgsql@j-davis.com>
2023-09-12 10:43 ` Amit Kapila <amit.kapila16@gmail.com>
2023-09-12 20:33 ` Jeff Davis <pgsql@j-davis.com>
2023-09-13 04:51 ` Amit Kapila <amit.kapila16@gmail.com>
2023-09-19 14:03 ` Robert Haas <robertmhaas@gmail.com>
2023-09-21 19:47 ` vignesh C <vignesh21@gmail.com>
2023-09-21 20:15 ` Robert Haas <robertmhaas@gmail.com>
2023-09-22 09:32 ` vignesh C <vignesh21@gmail.com>
2023-09-22 18:55 ` vignesh C <vignesh21@gmail.com>
2023-09-24 18:51 ` vignesh C <vignesh21@gmail.com>
2023-10-05 02:21 ` Peter Smith <smithpb2250@gmail.com>
2023-10-05 10:49 ` vignesh C <vignesh21@gmail.com>
2023-10-06 01:13 ` Peter Smith <smithpb2250@gmail.com>
2023-10-06 06:38 ` vignesh C <vignesh21@gmail.com>
2023-10-06 06:55 ` Peter Smith <smithpb2250@gmail.com>
2023-11-27 04:56 ` vignesh C <vignesh21@gmail.com>
2024-01-03 03:51 ` Jeff Davis <pgsql@j-davis.com>
2024-01-12 21:55 ` Jeff Davis <pgsql@j-davis.com>
2024-01-15 16:17 ` vignesh C <vignesh21@gmail.com>
2024-01-17 10:01 ` vignesh C <vignesh21@gmail.com>
2024-01-18 01:52 ` Jeff Davis <pgsql@j-davis.com>
2024-01-18 06:52 ` vignesh C <vignesh21@gmail.com>
2024-01-18 23:24 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox