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 <masao.fujii@oss.nttdata.com> 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-superuserif the requesting user is not a member of the other? Or that's overkill because the target to discard is
justa 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
laterif 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
connectionestablished by superuser. If we didn't think that this is a problem, we also might not need to care about
issueeven 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