Обсуждение: [16+] subscription can end up in inconsistent state
[ 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
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.
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
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.
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
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.
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
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
Вложения
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
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
Вложения
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.gmail.com
Regards,
Vignesh
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
Вложения
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
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
Вложения
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
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
Вложения
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.
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
Вложения
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
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
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
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
Вложения
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
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
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