Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

2015-05-22 18:37 GMT+09:00 Shigeru Hanada <shigeru.hanada@gmail.com>:
> 2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas@gmail.com>:
>> On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
>> <shigeru.hanada@gmail.com> wrote:
>>> d) All relations must have the same effective user id
>>> This check is done with userid stored in PgFdwRelationInfo, which is
>>> valid only when underlying relations have the same effective user id.
>>> Here "effective user id" means id of the user executing the query, or
>>> the owner of the view when the foreign table is accessed via view.
>>> Tom suggested that it is necessary to check that user mapping matches
>>> for security reason, and now I think it's better than checking
>>> effective user as current postgres_fdw does.
>>
>> So, should this be a separate patch?

I wrote patches for this issue.  Let me describe their design.

To require the matching of user mapping between two relations (base or
join) which involves foreign tables, it would require these stuffs:

a) Add new field umid to RelOptInfo to hold OID of user mapping used
for the relation and children
b) Propagate umid up to join relation only when both outer and inner
have the save valid values
c) Check matching of umid between two relations to be joined before
calling GetForeignJoinPaths

For a), adding an OID field would not be a serious burden.  Obtaining
the OID of user mapping can be accomplished by calling GetUserMapping
in get_relation_info, but it allocates an unnecessary UserMapping
object, so I added GetUserMappingId which just returns OID.

One concern about getting user mapping is checkAsUser.  Currently FDW
authors are required to consider which user is valid as argument of
GetUserMapping, because it is different from the result of GetUserId
when the target relation is accessed via a view which is owned by
another user.  This requirement would be easily ignored by FDW authors
and the ignorance causes terrible security issues.  So IMO it should
be hidden in the core code, and FDW authors should use user mappings
determined by the core.  This would break FDW I/F, so we can't change
it right now, but making GetUserMapping obsolete and mention it in the
documents would guide FDW authors to the right direction.

For b), such check should be done in build_join_rel, similarly to serverid.

For c), we can reuse the check about RelOptInfo#fdwroutine in
add_paths_to_joinrel, because all of serverid, umid and fdwroutine are
valid only when both the servers and the user mappings match between
outer and inner relations.

Attached are the patches which implement the idea above except
checkAsUser issue.
usermapping_matching.patch: check matching of user mapping OIDs
add_GetUserMappingById.patch: add helper function which is handy for
FDWs to obtain UserMapping
foreign_join_v16.patch: postgres_fdw which assumes user mapping
matching is done in core

Another idea about a) is to have an entire UserMapping object for each
RelOptInfo, but it would require UserMapping to have extra field of
its Oid, say umid, to compare them by OID.  But IMO creating
UserMapping for every RelOptInfo seems waste of space and time.

Thoughts?
--
Shigeru HANADA