Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Mon, Jan 25, 2021 at 1:20 PM Fujii Masao  wrote:
> > Yeah, connections can be discarded by non-super users using
> > postgres_fdw_disconnect_all and postgres_fdw_disconnect. Given the
> > fact that a non-super user requires a password to access foreign
> > tables [1], IMO a non-super user changing something related to a super
> > user makes no sense at all. If okay, we can have a check in
> > disconnect_cached_connections something like below:
>
> Also like pg_terminate_backend(), we should disallow non-superuser to disconnect the connections established by other non-superuser if the requesting user is not a member of the other? Or that's overkill because the target to discard is just a connection and it can be established again if necessary?

Yes, if required backends can establish the connection again. But my
worry is this - a non-super user disconnecting all or a given
connection created by a super user?

> For now I'm thinking that it might better to add the restriction like pg_terminate_backend() at first and relax that later if possible. But I'd like hear more opinions about this.

I agree. If required we can lift it later, once we get the users using
these functions? Maybe we can have a comment near superchecks in
disconnect_cached_connections saying, we can lift this in future?

Do you want me to add these checks like in pg_signal_backend?

    /* Only allow superusers to signal superuser-owned backends. */
    if (superuser_arg(proc->roleId) && !superuser())
        return SIGNAL_BACKEND_NOSUPERUSER;

    /* Users can signal backends they have role membership in. */
    if (!has_privs_of_role(GetUserId(), proc->roleId) &&
        !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SIGNAL_BACKENDID))
        return SIGNAL_BACKEND_NOPERMISSION;

or only below is enough?

+    /* Non-super users are not allowed to disconnect cached connections. */
+    if (!superuser())
+        ereport(ERROR,
+                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                 errmsg("must be superuser to discard open connections")));

> > +static bool
> > +disconnect_cached_connections(Oid serverid)
> > +{
> > +    HASH_SEQ_STATUS    scan;
> > +    ConnCacheEntry    *entry;
> > +    bool    all = !OidIsValid(serverid);
> > +    bool    result = false;
> > +
> > +    if (!superuser())
> > +        ereport(ERROR,
> > +                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > +                 errmsg("must be superuser to discard open connections")));
> > +
> > +    if (!ConnectionHash)
> >
> > Having said that, it looks like dblink_disconnect doesn't perform
> > superuser checks.
>
> Also non-superuser (set by SET ROLE or SET SESSION AUTHORIZATION) seems to be able to run SQL using the dblink connection established by superuser. If we didn't think that this is a problem, we also might not need to care about issue even for postgres_fdw.

IMO, we can have superuser checks for postgres_fdw new functions for now.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com