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

On 2021/01/23 13:40, Bharath Rupireddy wrote:
> On Fri, Jan 22, 2021 at 6:43 PM Fujii Masao <masao.fujii@oss.nttdata.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-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?
 

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.
 


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


Regards,

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