Обсуждение: Questions about the new subscription parameter: password_required

Hi,

I am confused about the new subscription parameter: password_required.

I have two instances. The publisher's pg_hba is configured too allow 
connections without authentication. On the subscriber, I have an 
unprivileged user with pg_create_subscription and CREATE on the database.

I tried using a superuser to create a subsciption without setting the 
password_required parameter (the default is true). Then I changed the 
owner to the unprivileged user.

This user can use the subscription without limitation (including ALTER 
SUBSCRIPTION ENABLE / DISABLE). The \dRs+ metacommand shows that a 
password is requiered, which is not the case (or it is but it's not 
enforced).

Is this normal? I was expecting the ALTER SUBSCRIPTION .. OWNER to fail.

When I try to drop the subscription with the unprivileged user or a 
superuser, I get an error:

ERROR:  password is required
DETAIL:  Non-superuser cannot connect if the server does not request a 
password.
HINT:  Target server's authentication method must be changed, or set 
password_required=false in the subscription parameters.

I have to re-change the subscription owner to the superuser, to be able 
to drop it.

(See password_required.sql and password_required.log)

I tried the same setup and changed the connexion string to add an 
application_name with the unprivileged user. In this case, I am reminded 
that I need a password. I tried modifying password_required to false 
with the superuser and modify the connexion string with the unprivilege 
user again. It fails with:

HINT:  Subscriptions with the password_required option set to false may 
only be created or modified by the superuser.

I think that this part works as intended.

I tried dropping the subscription with the unprivilege user: it works. 
Is it normal (given the previous message)?

(see password_required2.sql and password_required2.log)

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On Thu, Sep 21, 2023 at 8:03 AM Benoit Lobréau
<benoit.lobreau@dalibo.com> wrote:
> I am confused about the new subscription parameter: password_required.
>
> I have two instances. The publisher's pg_hba is configured too allow
> connections without authentication. On the subscriber, I have an
> unprivileged user with pg_create_subscription and CREATE on the database.
>
> I tried using a superuser to create a subsciption without setting the
> password_required parameter (the default is true). Then I changed the
> owner to the unprivileged user.
>
> This user can use the subscription without limitation (including ALTER
> SUBSCRIPTION ENABLE / DISABLE). The \dRs+ metacommand shows that a
> password is requiered, which is not the case (or it is but it's not
> enforced).
>
> Is this normal? I was expecting the ALTER SUBSCRIPTION .. OWNER to fail.

Which one? I see 2 ALTER SUBSCRIPTION ... OWNER commands in
password_required.log and 1 more in password_required2.log, but
they're all performed by the superuser, who is entitled to do anything
they want.

The intention here is that most subscriptions will have
passwordrequired=true. If such a subscription is owned by a superuser,
the superuser can still use them however they like. If owned by a
non-superuser, they can use them however they like *provided* that the
password must be used to authenticate. If the superuser wants a
non-superuser to be able to own a subscription that doesn't use a
password, the superuser can set that up by configuring
passwordrequired=false. But then that non-superuser is not allowed to
further manipulate that subscription.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/21/23 20:29, Robert Haas wrote:
> Which one? I see 2 ALTER SUBSCRIPTION ... OWNER commands in
> password_required.log and 1 more in password_required2.log, but
> they're all performed by the superuser, who is entitled to do anything
> they want.

Thank you for taking the time to respond!

I expected the ALTER SUBSCRIPTION ... OWNER command in 
password_required.log to fail because the end result of the command is a 
non-superuser owning a subscription with password_required=true, but the 
connection string has no password keyword, and the authentication scheme 
used doesn't require one anyway.

The description of the password_required parameter doesn't clearly state 
what will fail or when the configuration is enforced (during CREATE 
SUBSCRIPTION and ALTER SUBSCRIPTION .. CONNECTION):

""" https://www.postgresql.org/docs/16/sql-createsubscription.html
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.
"""

The description of pg_subscription.subpasswordrequired doesn't either:

""" https://www.postgresql.org/docs/16/catalog-pg-subscription.html
If true, the subscription will be required to specify a password for 
authentication
"""

Can we consider adding something like this to clarify?

"""
This parameter is enforced when the CREATE SUBSCRIPTION or ALTER 
SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's 
possible to alter the ownership of a subscription with 
password_required=true to a non-superuser.
"""

Is the DROP SUBSCRIPTION failure in password_required.log expected for 
both superuser and non-superuser?

Is the DROP SUBSCRIPTION success in password_required2.log expected?
(i.e., with password_require=false, the only action a non-superuser can 
perform is dropping the subscription. Since they own it, it is 
understandable).

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On Fri, Sep 22, 2023 at 4:25 AM Benoit Lobréau
<benoit.lobreau@dalibo.com> wrote:
> Can we consider adding something like this to clarify?
>
> """
> This parameter is enforced when the CREATE SUBSCRIPTION or ALTER
> SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's
> possible to alter the ownership of a subscription with
> password_required=true to a non-superuser.
> """

I'm not sure of the exact wording, but there was another recent thread
complaining about this being unclear, so it seems like some
clarification is needed.

[ adding Jeff Davis in case he wants to weigh in here ]

> Is the DROP SUBSCRIPTION failure in password_required.log expected for
> both superuser and non-superuser?
>
> Is the DROP SUBSCRIPTION success in password_required2.log expected?
> (i.e., with password_require=false, the only action a non-superuser can
> perform is dropping the subscription. Since they own it, it is
> understandable).

I haven't checked this, but I think what's happening here is that DROP
SUBSCRIPTION tries to drop the remote slot, which requires making a
connection, which can trigger the error. You might get different
results if you did ALTER SUBSCRIPTION ... SET (slot_name = none)
first.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/22/23 14:36, Robert Haas wrote:
> I haven't checked this, but I think what's happening here is that DROP
> SUBSCRIPTION tries to drop the remote slot, which requires making a
> connection, which can trigger the error. You might get different
> results if you did ALTER SUBSCRIPTION ... SET (slot_name = none)
> first.

You're right, it comes from the connection to drop the slot.

But the code in for DropSubscription in 
src/backend/commands/subscriptioncmds.c tries to connect before testing 
if the slot is NONE / NULL. So it doesn't work to DISABLE the 
subscription and set the slot to NONE.


   1522 >~~~must_use_password = !superuser_arg(subowner) && 
form->subpasswordrequired;
   ...
   1685 >~~~wrconn = walrcv_connect(conninfo, true, must_use_password,
      1 >~~~>~~~>~~~>~~~>~~~>~~~>~~~subname, &err);
      2 >~~~if (wrconn == NULL)
      3 >~~~{
      4 >~~~>~~~if (!slotname)
      5 >~~~>~~~{
      6 >~~~>~~~>~~~/* be tidy */
      7 >~~~>~~~>~~~list_free(rstates);
      8 >~~~>~~~>~~~table_close(rel, NoLock);
      9 >~~~>~~~>~~~return;
     10 >~~~>~~~}
     11 >~~~>~~~else
     12 >~~~>~~~{
     13 >~~~>~~~>~~~ReportSlotConnectionError(rstates, subid, slotname, 
err);
     14 >~~~>~~~}
     15 >~~~}


Reading the code, I think I understand why the postgres user cannot drop 
the slot:

* the owner is sub_owner (not a superuser)
* and form->subpasswordrequired is true

Should there be a test to check if the user executing the query is 
superuser? maybe it's handled differently? (I am not very familiar with 
the code).

I dont understand (yet?) why I can do ALTER SUBSCRIPTIONs after changing 
the ownership to an unpriviledged user (must_use_password should be true 
also in that case).

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On Fri, Sep 22, 2023 at 10:59 AM Benoit Lobréau
<benoit.lobreau@dalibo.com> wrote:
> You're right, it comes from the connection to drop the slot.
>
> But the code in for DropSubscription in
> src/backend/commands/subscriptioncmds.c tries to connect before testing
> if the slot is NONE / NULL. So it doesn't work to DISABLE the
> subscription and set the slot to NONE.

So I'm seeing this:

    if (!slotname && rstates == NIL)
    {
        table_close(rel, NoLock);
        return;
    }

    load_file("libpqwalreceiver", false);

    wrconn = walrcv_connect(conninfo, true, must_use_password,
                            subname, &err);

That looks like it's intended to return if there's nothing to do, and
the comments say as much. But that (!slotname && rstates == NIL) test
looks sketchy. It seems like we should bail out early if *either*
!slotname *or* rstates == NIL, or for that matter if all of the
rstates have rstate->relid == 0 or rstate->state ==
SUBREL_STATE_SYNCDONE. Maybe we need to push setting up the connection
inside the foreach(lc, rstates) loop and do it only once we're sure we
want to do something. Or at least, I don't understand why we don't
bail out immediately in all cases where slotname is NULL, regardless
of rstates. Am I missing something here?

> Reading the code, I think I understand why the postgres user cannot drop
> the slot:
>
> * the owner is sub_owner (not a superuser)
> * and form->subpasswordrequired is true
>
> Should there be a test to check if the user executing the query is
> superuser? maybe it's handled differently? (I am not very familiar with
> the code).

I think that there normally shouldn't be any problem here, because if
form->subpasswordrequired is true, we expect that the connection
string should contain a password which the remote side actually uses,
or we expect the subscription to be owned by the superuser. If neither
of those things is the case, then either the superuser made a
subscription that doesn't use a password owned by a non-superuser
without fixing subpasswordrequired, or else the configuration on the
remote side has changed so that it now doesn't use the password when
formerly it did. In the first case, perhaps it would be fine to go
ahead and drop the slot, but in the second case I don't think it's OK
from a security point view, because the command is going to behave the
same way no matter who executes the drop command, and a non-superuser
who drops the slot shouldn't be permitted to rely on the postgres
processes's identity to do anything on a remote node -- including
dropping a relation slot. So I tentatively think that this behavior is
correct.

> I dont understand (yet?) why I can do ALTER SUBSCRIPTIONs after changing
> the ownership to an unpriviledged user (must_use_password should be true
> also in that case).

Maybe you're altering it in a way that doesn't involve any connections
or any changes to the connection string? There's no security issue if,
say, you rename it.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, 2023-09-22 at 08:36 -0400, Robert Haas wrote:
> On Fri, Sep 22, 2023 at 4:25 AM Benoit Lobréau
> <benoit.lobreau@dalibo.com> wrote:
> > Can we consider adding something like this to clarify?
> >
> > """
> > This parameter is enforced when the CREATE SUBSCRIPTION or ALTER
> > SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's
> > possible to alter the ownership of a subscription with
> > password_required=true to a non-superuser.
> > """
>
> I'm not sure of the exact wording, but there was another recent
> thread
> complaining about this being unclear, so it seems like some
> clarification is needed.

IIUC there is really one use case here, which is for superuser to
define a subscription including the connection, and then change the
owner to a non-superuser to actually run it (without being able to
touch the connection string itself). I'd just document that in its own
section, and mention a few caveats / mistakes to avoid. For instance,
when the superuser is defining the connection, don't forget to set
password_required=false, so that when you reassign to a non-superuser
then the connection doesn't break.

Regards,
    Jeff Davis

On 9/22/23 21:58, Robert Haas wrote
> I think that there normally shouldn't be any problem here, because if
> form->subpasswordrequired is true, we expect that the connection
> string should contain a password which the remote side actually uses,
> or we expect the subscription to be owned by the superuser. If neither
> of those things is the case, then either the superuser made a
> subscription that doesn't use a password owned by a non-superuser
> without fixing subpasswordrequired, or else the configuration on the
> remote side has changed so that it now doesn't use the password when
> formerly it did. In the first case, perhaps it would be fine to go
> ahead and drop the slot, but in the second case I don't think it's OK
> from a security point view, because the command is going to behave the
> same way no matter who executes the drop command, and a non-superuser
> who drops the slot shouldn't be permitted to rely on the postgres
> processes's identity to do anything on a remote node -- including
> dropping a relation slot. So I tentatively think that this behavior is
> correct.

I must admin I hadn't considered the implication when the configuration 
on the remote side has changed and we use a non-superuser. I see how it 
could be problematic.

I will try to come up with a documentation patch.

> Maybe you're altering it in a way that doesn't involve any connections
> or any changes to the connection string? There's no security issue if,
> say, you rename it.

I looked at the code again. Indeed, of the ALTER SUBSCRIPTION commands, 
only ALTER SUBSCRIPTION .. CONNECTION uses walrcv_check_conninfo().

I checked the other thread (Re: [16+] subscription can end up in 
inconsistent state [1]) and will try the patch. Is it the thread you 
where refering to earlier ?

[1] 

https://www.postgresql.org/message-id/flat/5dff4caf26f45ce224a33a5e18e110b93a351b2f.camel%40j-davis.com#ff4a06505de317b1ad436b8102a69446

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On 9/26/23 16:27, Benoit Lobréau wrote:
> I will try to come up with a documentation patch.

This is my attempt at a documentation patch.

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On Tue, 2023-09-26 at 18:21 +0200, Benoit Lobréau wrote:
> On 9/26/23 16:27, Benoit Lobréau wrote:
> > I will try to come up with a documentation patch.
>
> This is my attempt at a documentation patch.
>


  +   If the ownership of a subscription with
<literal>password_required=true</literal>
  +   is transferred to a non-superuser, they will gain full control
over the subscription
  +   but will not be able to modify it's connection string.

I think you mean false, right?

  +   If the ownership of a subscription with
<literal>password_required=true</literal>
  +   has been transferred to a non-superuser, it must be reverted to a
superuser for
  +   the DROP operation to succeed.

That's only needed if the superuser transfers a subscription with
password_required=true to a non-superuser and the connection string
does not contain a password. In that case, the subscription is already
in a failing state, not just for DROP. Ideally we'd have some other
warning in the docs not to do that -- maybe in CREATE and ALTER.

Also, if the subscription is in that kind of failing state, there are
other ways to get out of it as well, like disabling it and setting
connection=none, then dropping it.

The whole thing is fairly delicate. As soon as you work slightly
outside of the intended use, password_required starts causing
unexpected things to happen.

As I said earlier, I think the best thing to do is to just have a
section that describes when to use password_required, what specific
things you should do to satisfy that case, and what caveats you should
avoid. Something like:

  "If you want to have a subscription using a connection string without
a password managed by a non-superuser, then: [ insert SQL steps here ].
Warning: if the connection string doesn't contain a password, make sure
to set password_required=false before transferring ownership, otherwise
it will start failing."

Documenting the behavior is good, too, but I find the behavior
difficult to document, so examples will help.

Regards,
    Jeff Davis

On Tue, Sep 26, 2023 at 1:00 PM Jeff Davis <pgsql@j-davis.com> wrote:
> As I said earlier, I think the best thing to do is to just have a
> section that describes when to use password_required, what specific
> things you should do to satisfy that case, and what caveats you should
> avoid. Something like:
>
>   "If you want to have a subscription using a connection string without
> a password managed by a non-superuser, then: [ insert SQL steps here ].
> Warning: if the connection string doesn't contain a password, make sure
> to set password_required=false before transferring ownership, otherwise
> it will start failing."
>
> Documenting the behavior is good, too, but I find the behavior
> difficult to document, so examples will help.

Yeah, I think something like that could make sense, with an
appropriate amount of word-smithing.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/26/23 19:00, Jeff Davis wrote:
>    +   If the ownership of a subscription with
> <literal>password_required=true</literal>
>    +   is transferred to a non-superuser, they will gain full control
> over the subscription
>    +   but will not be able to modify it's connection string.
> 
> I think you mean false, right?

No, but I was wrong. At the beginning of the thread, I was surprised 
was even possible to change the ownership to a non-superuser because It 
shouldn't work and commands like ENABLE didn't complain in the terminal.
Then Robert Haas explained to me that it's ok because the superuser can 
do whatever he wants. I came back to it later and somehow convinced 
myself it was working. Sorry.

>    +   If the ownership of a subscription with
> <literal>password_required=true</literal>
>    +   has been transferred to a non-superuser, it must be reverted to a
> superuser for
>    +   the DROP operation to succeed.
> 
> That's only needed if the superuser transfers a subscription with
> password_required=true to a non-superuser and the connection string
> does not contain a password. In that case, the subscription is already
> in a failing state, not just for DROP. Ideally we'd have some other
> warning in the docs not to do that -- maybe in CREATE and ALTER.

Yes, I forgot the connection string bit.

> Also, if the subscription is in that kind of failing state, there are
> other ways to get out of it as well, like disabling it and setting
> connection=none, then dropping i
The code in for DropSubscription in
src/backend/commands/subscriptioncmds.c tries to connect before testing
if the slot is NONE / NULL. So it doesn't work to DISABLE the
subscription and set the slot to NONE.

Robert Haas proposed something in the following message but I am a 
little out of my depth here ...

https://www.postgresql.org/message-id/af9435ae-18df-6a9e-2374-2de23009518c%40dalibo.com

> The whole thing is fairly delicate. As soon as you work slightly
> outside of the intended use, password_required starts causing
> unexpected things to happen.
> 
> As I said earlier, I think the best thing to do is to just have a
> section that describes when to use password_required, what specific
> things you should do to satisfy that case, and what caveats you should
> avoid. Something like:
> 
>    "If you want to have a subscription using a connection string without
> a password managed by a non-superuser, then: [ insert SQL steps here ].
> Warning: if the connection string doesn't contain a password, make sure
> to set password_required=false before transferring ownership, otherwise
> it will start failing."

Ok, I will do it that way. Would you prefer this section to be in the 
ALTER SUBSCRIPTION on the CREATE SUBSCIPTION doc ?

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On 9/23/23 03:57, Jeff Davis wrote:
> IIUC there is really one use case here, which is for superuser to
> define a subscription including the connection, and then change the
> owner to a non-superuser to actually run it (without being able to
> touch the connection string itself). I'd just document that in its own
> section, and mention a few caveats / mistakes to avoid. For instance,
> when the superuser is defining the connection, don't forget to set
> password_required=false, so that when you reassign to a non-superuser
> then the connection doesn't break.

Hi,

I tried adding a section in "Logical Replication > Subscription" with 
the text you suggested and links in the CREATE / ALTER SUBSRIPTION commands.

Is it better ?

-- 
Benoit Lobréau
Consultant
http://dalibo.com

On Fri, 2023-10-13 at 11:18 +0200, Benoit Lobréau wrote:
> I tried adding a section in "Logical Replication > Subscription" with
> the text you suggested and links in the CREATE / ALTER SUBSRIPTION
> commands.
>
> Is it better ?


Minor comments:

 * Use possessive "its" instead of the contraction, i.e. "before
transferring its ownership".
 * I like that docs cover the case where a password is specified, but
the remote server doesn't require one. But the warning is the wrong
place to explain that, it should be in the main behavioral description
in 31.2.2.
 * The warning feels like it has too many negatives and confused me at
first. I struggled myself a bit to come up with something less
confusing, but perhaps less is more: "Ensure that password_required is
properly set before transferring ownership of a subscription to a non-
superuser, otherwise the subscription may start to fail."
 * Missing space in the warning after "password_required = true"
 * Mention that a non-superuser-owned subscription with
password_required = false is partially locked down, e.g. the owner
can't change the connection string any more.
 * 31.2.2 could probably be in the CREATE SUBSCRIPTION docs instead,
and linked from the ALTER docs. That's fairly normal for other commands
and I'm not sure there needs to be a separate section in logical
replication. I don't have a strong opinion here.

I like the changes; this is a big improvement. I'll leave it to Robert
to commit it, so that he can ensure it matches how he expected the
feature to be used and sufficiently covers the behavioral aspects.

Regards,
    Jeff Davis

Hi, how about having links (instead of just
<literal>password_required=false</literal>) in alter_subscription.sgml
and logical-replication.sgml so the user can navigate easily back to
the CREATE SUBSCRIPTION parameters "password_required" part.

For example, alter_subscription.sgml does this already for "two_phase"
and "copy_data" but not for "password_required" (??)

======
Kind Regards,
Peter Smith
Fujitsu Australia

On Sat, Oct 14, 2023 at 5:57 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2023-10-13 at 11:18 +0200, Benoit Lobréau wrote:
> > I tried adding a section in "Logical Replication > Subscription" with
> > the text you suggested and links in the CREATE / ALTER SUBSRIPTION
> > commands.
> >
> > Is it better ?
>
>
> Minor comments:
>
>  * Use possessive "its" instead of the contraction, i.e. "before
> transferring its ownership".
>  * I like that docs cover the case where a password is specified, but
> the remote server doesn't require one. But the warning is the wrong
> place to explain that, it should be in the main behavioral description
> in 31.2.2.
>  * The warning feels like it has too many negatives and confused me at
> first. I struggled myself a bit to come up with something less
> confusing, but perhaps less is more: "Ensure that password_required is
> properly set before transferring ownership of a subscription to a non-
> superuser, otherwise the subscription may start to fail."
>  * Missing space in the warning after "password_required = true"
>  * Mention that a non-superuser-owned subscription with
> password_required = false is partially locked down, e.g. the owner
> can't change the connection string any more.
>  * 31.2.2 could probably be in the CREATE SUBSCRIPTION docs instead,
> and linked from the ALTER docs. That's fairly normal for other commands
> and I'm not sure there needs to be a separate section in logical
> replication. I don't have a strong opinion here.
>
> I like the changes; this is a big improvement. I'll leave it to Robert
> to commit it, so that he can ensure it matches how he expected the
> feature to be used and sufficiently covers the behavioral aspects.
>
> Regards,
>         Jeff Davis
>
>
>