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

On Sun, Sep 10, 2023 at 9:15 PM Jeff Davis <pgsql(at)j-davis(dot)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.