Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 8, 2015 at 5:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that we'd really be better off insisting on same server (as in
>> same pg_foreign_server OID), hence automatically same FDW, and what's
>> even more important, same user mapping for any possible query execution
>> context.  The possibility that there are some corner cases where some FDWs
>> could optimize other scenarios seems to me to be poor return for the bugs
>> and security holes that will arise any time typical FDWs forget to check
>> this.

> I originally wanted to go quite the other way with this and check for
> join pushdown via handler X any time at least one of the two relations
> involved used handler X, even if the other one used some other handler
> or was a plain table.  In particular, it seems to me quite plausible
> to want to teach an FDW that a certain local table is replicated on a
> remote node, allowing a join between a foreign table and a plain table
> to be pushed down.

If we did do something like that, I think a saner implementation would
involve substituting a foreign table for the local one earlier, via view
expansion.  So by the time we are doing join planning, there would be no
need to consider cross-server joins anyway.

> This infrastructure can't be used that way anyhow,
> so maybe there's no harm in tightening it up, but I'm wary of
> circumscribing what FDW authors can do.

Somebody who's really intent on shooting themselves in the foot can always
use the set_join_pathlist_hook to inject paths for arbitrary joins.
The FDW mechanism should support reasonable use cases without undue
complication, and I doubt that what we've got now is adding anything
except complication and risk of bugs.

For the archives' sake, let me lay out a couple of reasons why an FDW
that tried to allow cross-server joins would almost certainly be broken,
and broken in security-relevant ways.  Suppose for instance that
postgres_fdw tried to be smart and drill down into foreign tables' server
IDs to allow joining of any two tables that have the same effective host
name, port, database name, user name, and anything else you think would be
relevant to its choice of connections.  The trouble with that is that the
user mapping is context dependent, in particular one local userID might
map to the same remote user name for two different server OIDs, while
another might map to different user names.  So if we plan a query under
the first userID we might decide it's okay to do the join remotely.
Then, if we re-use that plan while executing as another userID (which is
entirely possible) what will probably happen is that the remote join
query will get sent off under one or the other of the remote usernames
associated with the second local userID.  This could lead to either a
permission failure, or a remote table access that should not be allowed
to the current local userID.  Admittedly, such cases might be rare in
practice, but it's still a security hole.  Also, even if the FDW is
defensive enough to recheck full matching of the tables' connection
properties at execution time, there's not much it could do about the
situation except fail; it couldn't cause a re-plan to occur.

For another case, we do not have any mechanism whereby operations like
ALTER SERVER OPTIONS could invalidate existing plans.  Thus, even if
the two tables' connection properties matched at plan time, there's no
guarantee that they still match at execution.  This is probably not a
security hole (at least not if you assume foreign-server owners are
trusted users), but it's still a bug that exists only if someone tries
to allow cross-server joins.

For these reasons, I think that if an FDW tried to be laxer than "tables
must be on the same pg_foreign_server entry to be joined", the odds
approach unity that it would be broken, and probably dangerously broken.
So we should just make that check for the FDWs.  Anybody who thinks
they're smarter than the average bear can use set_join_pathlist_hook,
but they are probably wrong.
        regards, tom lane