Re: [16+] subscription can end up in inconsistent state

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