On Mon, Jan 25, 2021 at 1:20 PM Fujii Masao <[email protected]> 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
