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(dot)fujii(at)oss(dot)nttdata(dot)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-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