Re: Questions about the new subscription parameter: password_required

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