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

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.