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

On 2021/01/23 13:40, Bharath Rupireddy wrote:
> On Fri, Jan 22, 2021 at 6:43 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>> Please review the v16 patch set further.
>>>
>>> Thanks! Will review that later.
>>
>> + /*
>> + * For the given server, if we closed connection or it is still in
>> + * use, then no need of scanning the cache further. We do this
>> + * because the cache can not have multiple cache entries for a
>> + * single foreign server.
>> + */
>>
>> On second thought, ISTM that single foreign server can have multiple cache
>> entries. For example,
>>
>> CREATE ROLE foo1 SUPERUSER;
>> CREATE ROLE foo2 SUPERUSER;
>> CREATE EXTENSION postgres_fdw;
>> CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw OPTIONS (port '5432');
>> CREATE USER MAPPING FOR foo1 SERVER loopback OPTIONS (user 'postgres');
>> CREATE USER MAPPING FOR foo2 SERVER loopback OPTIONS (user 'postgres');
>> CREATE TABLE t (i int);
>> CREATE FOREIGN TABLE ft (i int) SERVER loopback OPTIONS (table_name 't');
>> SET SESSION AUTHORIZATION foo1;
>> SELECT * FROM ft;
>> SET SESSION AUTHORIZATION foo2;
>> SELECT * FROM ft;
>>
>>
>> Then you can see there are multiple open connections for the same server
>> as follows. So we need to scan all the entries even when the serverid is
>> specified.
>>
>> SELECT * FROM postgres_fdw_get_connections();
>>
>> server_name | valid
>> -------------+-------
>> loopback | t
>> loopback | t
>> (2 rows)
>
> This is a great finding. Thanks a lot. I will remove
> hash_seq_term(&scan); in disconnect_cached_connections and add this as
> a test case for postgres_fdw_get_connections function, just to show
> there can be multiple connections with a single server name.
>
>> This means that user (even non-superuser) can disconnect the connection
>> established by another user (superuser), by using postgres_fdw_disconnect_all().
>> Is this really OK?
>
> 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?

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.

>
> +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.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION