Обсуждение: [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