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

On Mon, Jan 25, 2021 at 7:20 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/01/25 19:28, Bharath Rupireddy wrote:
> > On Mon, Jan 25, 2021 at 3:17 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>> 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?
> >>
> >> Yes, I was also worried about that. But I found that there are other similar cases, for example,
> >>
> >> - a cursor that superuser declared can be closed by non-superuser (set by SET ROLE or SET SESSION AUTHORIZATION)
inthe same session.
 
> >> - a prepared statement that superuser created can be deallocated by non-superuser in the same session.
> >>
> >> This makes me think that it's OK even for non-superuser to disconnect the connections established by superuser in
thesame session. For now I've not found any real security issue by doing that yet. Thought? Am I missing something?
 
> >
> > Oh, and added to that list is dblink_disconnect(). I don't know
> > whether there's any security risk if we allow non-superusers to
> > discard the super users connections.
>
> I guess that's ok because superuser and nonsuperuser are running in the same session. That is, since this is the case
wheresuperuser switches to nonsuperuser intentionally, interactions between them is also intentional.
 
>
> OTOH, if nonsuperuser in one session can affect superuser in another session that way, which would be problematic.
So,for example, for now pg_stat_activity disallows nonsuperuser to see the query that superuser in another session is
running,from it.
 

Hmm, that makes sense.

> > In this case, the super users
> > will just have to re make the connection.
> >
> >>> For now I'm thinking that it might better to add the restriction like pg_terminate_backend() at first and relax
thatlater 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?
> >
> > Maybe we can do the opposite of the above that is not doing any
> > superuser checks in disconnect functions for now, and later if some
> > users complain we can add it?
>
> +1

Thanks, will send the updated patch set soon.

> > We can leave a comment there that "As of
> > now we don't see any security risks if a non-super user disconnects
> > the connections made by super users. If required, non-supers can be
> > disallowed to disconnct the connections" ?
>
> Yes. Also we should note that that's ok because they are in the same session.

I will add this comment in disconnect_cached_connections so that we
don't lose track of it.

I will provide the updated patch set soon.

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