Horton

Thread

Commits

  1. Protect dblink from invalid options when using postgres_fdw server

  1. dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Corey Huinker <corey.huinker@gmail.com> — 2016-11-21T00:37:15Z

    I ran into this today:

select current_database() as current_db \gset
CREATE EXTENSION postgres_fdw;
CREATE EXTENSION
CREATE EXTENSION dblink;
CREATE EXTENSION
CREATE ROLE bob LOGIN PASSWORD 'bob';
CREATE ROLE
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
CREATE SERVER
CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob',
password 'bob' );
CREATE USER MAPPING
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
 x
---
 1
(1 row)

ALTER SERVER bob_srv OPTIONS (updatable 'true');
ALTER SERVER
SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);
psql:bug_example.sql:18: ERROR:  could not establish connection
DETAIL:  invalid connection option "updatable"


Is this something we want to fix?
If so, are there any other fdw/server/user-mapping options that we don't
want to pass along to the connect string?

Steps to re-create:

bug_example.sh:#!/bin/bash
dropdb bug_example
dropuser bob
createdb bug_example
psql bug_example -f bug_example.sql


bug_example.sql:

\set ECHO all

select current_database() as current_db \gset

CREATE EXTENSION postgres_fdw;
CREATE EXTENSION dblink;
CREATE ROLE bob LOGIN PASSWORD 'bob';
CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
'localhost', dbname :'current_db' );
CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob',
password 'bob' );

SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);

ALTER SERVER bob_srv OPTIONS (updatable 'true');

SELECT *
FROM dblink('bob_srv','SELECT 1') as t(x integer);

  2. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Robert Haas <robertmhaas@gmail.com> — 2016-11-21T19:01:15Z

    On Sun, Nov 20, 2016 at 7:37 PM, Corey Huinker <corey.huinker@gmail.com> wrote:
> I ran into this today:
>
> select current_database() as current_db \gset
> CREATE EXTENSION postgres_fdw;
> CREATE EXTENSION
> CREATE EXTENSION dblink;
> CREATE EXTENSION
> CREATE ROLE bob LOGIN PASSWORD 'bob';
> CREATE ROLE
> CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
> 'localhost', dbname :'current_db' );
> CREATE SERVER
> CREATE USER MAPPING FOR public SERVER bob_srv OPTIONS ( user 'bob', password
> 'bob' );
> CREATE USER MAPPING
> SELECT *
> FROM dblink('bob_srv','SELECT 1') as t(x integer);
>  x
> ---
>  1
> (1 row)
>
> ALTER SERVER bob_srv OPTIONS (updatable 'true');
> ALTER SERVER
> SELECT *
> FROM dblink('bob_srv','SELECT 1') as t(x integer);
> psql:bug_example.sql:18: ERROR:  could not establish connection
> DETAIL:  invalid connection option "updatable"
>
>
> Is this something we want to fix?

Looks like a bug to me.

> If so, are there any other fdw/server/user-mapping options that we don't
> want to pass along to the connect string?

InitPgFdwOptions has an array labeled as /* non-libpq FDW-specific FDW
options */.  Presumably all of those should be handled in a common
way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-21T19:16:24Z

    Corey Huinker <corey.huinker@gmail.com> writes:
> I ran into this today:
> CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
> 'localhost', dbname :'current_db' );
> ...
> ALTER SERVER bob_srv OPTIONS (updatable 'true');
> SELECT *
> FROM dblink('bob_srv','SELECT 1') as t(x integer);
> psql:bug_example.sql:18: ERROR:  could not establish connection
> DETAIL:  invalid connection option "updatable"

> Is this something we want to fix?

The dblink docs recommend using dblink_fdw as the FDW for this purpose,
which would only accept legal connstr options.  However, I can see the
point of using a postgres_fdw server instead, and considering that
dblink isn't actually enforcing use of any particular FDW type, it seems
like the onus should be on it to be more wary of what the options are.

It looks like this might be fairly easy to fix by having
get_connect_string() use is_valid_dblink_option() to check each
option name, and silently ignore options that are inappropriate.

			regards, tom lane

  4. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Joe Conway <mail@joeconway.com> — 2016-11-21T20:36:37Z

    On 11/21/2016 02:16 PM, Tom Lane wrote:
> Corey Huinker <corey.huinker@gmail.com> writes:
>> I ran into this today:
>> CREATE SERVER bob_srv FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host
>> 'localhost', dbname :'current_db' );
>> ...
>> ALTER SERVER bob_srv OPTIONS (updatable 'true');
>> SELECT *
>> FROM dblink('bob_srv','SELECT 1') as t(x integer);
>> psql:bug_example.sql:18: ERROR:  could not establish connection
>> DETAIL:  invalid connection option "updatable"
> 
>> Is this something we want to fix?
> 
> The dblink docs recommend using dblink_fdw as the FDW for this purpose,
> which would only accept legal connstr options.  However, I can see the
> point of using a postgres_fdw server instead, and considering that
> dblink isn't actually enforcing use of any particular FDW type, it seems
> like the onus should be on it to be more wary of what the options are.
> 
> It looks like this might be fairly easy to fix by having
> get_connect_string() use is_valid_dblink_option() to check each
> option name, and silently ignore options that are inappropriate.

Thanks for the report and analysis. I'll take a look at creating a patch
this week.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  5. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Corey Huinker <corey.huinker@gmail.com> — 2016-11-21T20:59:32Z

    >
> The dblink docs recommend using dblink_fdw as the FDW for this purpose,
> which would only accept legal connstr options.  However, I can see the
> point of using a postgres_fdw server instead, and considering that
> dblink isn't actually enforcing use of any particular FDW type, it seems
> like the onus should be on it to be more wary of what the options are.
>

I have to admit, this was the first I'd heard of dblink_fdw. I'm glad it
exists, though. And yes, I'd like to be able to use postgres_fdw entries
because there's value in knowing that the connection for your ad-hoc SQL
exactly matches the connection used for other FDW tables.


> It looks like this might be fairly easy to fix by having
> get_connect_string() use is_valid_dblink_option() to check each
> option name, and silently ignore options that are inappropriate.
>

From what I can tell, it is very straightforward, the context oids are set
up just a few lines above where the new is_valid_dblink_option() calls
would be.

I'm happy to write the patch, for both v10 and any back-patches we feel are
necessary. However, I suspect with a patch this trivial that reviewing
another person's patch might be more work for a committer than just doing
it themselves. If that's not the case, let me know and I'll get started.

  6. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Corey Huinker <corey.huinker@gmail.com> — 2016-11-22T19:43:05Z

    >
>
>
>> It looks like this might be fairly easy to fix by having
>> get_connect_string() use is_valid_dblink_option() to check each
>> option name, and silently ignore options that are inappropriate.
>>
>
> From what I can tell, it is very straightforward, the context oids are set
> up just a few lines above where the new is_valid_dblink_option() calls
> would be.
>
> I'm happy to write the patch, for both v10 and any back-patches we feel
> are necessary. However, I suspect with a patch this trivial that reviewing
> another person's patch might be more work for a committer than just doing
> it themselves. If that's not the case, let me know and I'll get started.
>

Joe indicated that he wouldn't be able to get to the patch until this
weekend at the earliest, so I went ahead and made the patches on my own.

Nothing unusual to report for master, 9.6, 9.5, or 9.3. The patch is
basically the same for all of them and I was able to re-run the test script
at the beginning of the thread to ensure that the fix worked.

In 9.4, I encountered a complaint about flex 2.6.0. After a little research
it seems that a fix for that made it into versions 9.3+, but not 9.4. That
mini-patch is attached as well (0001.configure.94.diff). The dblink patch
for 9.4 was basically the same as the others.

The issue (no validation of connection string elements pulled from an FDW)
exists in 9.2, however, the only possible source of such options I know of
(postgres_fdw) does not. So I doubt we need to patch 9.2, but it's trivial
to do so if we want to.

  7. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-22T20:29:40Z

    Corey Huinker <corey.huinker@gmail.com> writes:
> In 9.4, I encountered a complaint about flex 2.6.0. After a little research
> it seems that a fix for that made it into versions 9.3+, but not 9.4.

Er ... what?  See 1ba874505.

			regards, tom lane

  8. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Corey Huinker <corey.huinker@gmail.com> — 2016-11-22T20:53:43Z

    On Tue, Nov 22, 2016 at 3:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huinker@gmail.com> writes:
> > In 9.4, I encountered a complaint about flex 2.6.0. After a little
> research
> > it seems that a fix for that made it into versions 9.3+, but not 9.4.
>
> Er ... what?  See 1ba874505.
>
>                         regards, tom lane
>

Maybe git fetch might didn't pick up that change. Odd that it did pick it
up on all other REL_* branches. Feel free to disregard that file.

I re-ran git pulls from my committed local branches to the corresponding
REL9_x_STABLE branch, and encountered no conflicts, so the 0001.dblink.*
patches should still be good.

  9. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Joe Conway <mail@joeconway.com> — 2016-12-18T21:48:51Z

    On 11/21/2016 03:59 PM, Corey Huinker wrote:
> On 11/21/2016 02:16 PM, Tom Lane wrote:
>> The dblink docs recommend using dblink_fdw as the FDW for this purpose,
>> which would only accept legal connstr options.  However, I can see the
>> point of using a postgres_fdw server instead, and considering that
>> dblink isn't actually enforcing use of any particular FDW type, it seems
>> like the onus should be on it to be more wary of what the options are.

> I have to admit, this was the first I'd heard of dblink_fdw. I'm glad it
> exists, though. And yes, I'd like to be able to use postgres_fdw entries
> because there's value in knowing that the connection for your ad-hoc SQL
> exactly matches the connection used for other FDW tables.

> I'm happy to write the patch, for both v10 and any back-patches we feel
> are necessary.

I looked at Corey's patch, which is straightforward enough, but I was
left wondering if dblink should be allowing any FDW at all (as it does
currently), or should it be limited to dblink_fdw and postgres_fdw? It
doesn't make sense to even try if the FDW does not connect via libpq.

Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
supports a libpq connection it would make sense to allows other FDWs
with this attribute, but since there is none the current state strikes
me as a bad idea.

Thoughts?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  10. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Michael Paquier <michael.paquier@gmail.com> — 2016-12-18T21:57:00Z

    On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway <mail@joeconway.com> wrote:
> Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
> supports a libpq connection it would make sense to allows other FDWs
> with this attribute, but since there is none the current state strikes
> me as a bad idea.
>
> Thoughts?

libpq is proper to the implementation of the FDW, not the wrapper on
top of it, so using in the CREATE FDW command a way to do the
decision-making that does not look right to me. Filtering things at
connection attempt is a better solution.
-- 
Michael

  11. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Corey Huinker <corey.huinker@gmail.com> — 2016-12-18T22:47:28Z

    On Sun, Dec 18, 2016 at 4:57 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway <mail@joeconway.com> wrote:
> > Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
> > supports a libpq connection it would make sense to allows other FDWs
> > with this attribute, but since there is none the current state strikes
> > me as a bad idea.
> >
> > Thoughts?
>
> libpq is proper to the implementation of the FDW, not the wrapper on
> top of it, so using in the CREATE FDW command a way to do the
> decision-making that does not look right to me. Filtering things at
> connection attempt is a better solution.
> --
> Michael
>

The only benefit I see would be giving the user a slightly more clear error
message like ('dblink doesn't work with %', 'mysql') instead of the error
about the failure of the connect string generated by the options that did
overlap.

Gaming out the options of a Wrapper X pointing to server Y:

1. Wrapper X does not have enough overlap in options to accidentally create
a legit connect string:
    Connection fails, user gets a message about the incompleteness of the
connection.
2. Wrapper X has enough options (i.e. user+host+dbname,etc) to generate a
legit connect string (with matching port), but server+port pointed to by X
isn't listening or doesn't speak libpq:
    Connection fails, user gets an error message.
3. Wrapper X has enough options (i.e. user+host+dbname,etc) to generate a
legit connect string (without matching port), but server+5432 pointed to by
X doesn't speak libpq:
    Whatever *is* listening on 5432 has a chance to try to handshake
libpq-style, and I guess there's a chance a hostile service listening on
that port might know enough libpq to siphon off the credentials, but the
creds it would get would be to a pgsql service on Y and Y is already
compromised. Also, that vulnerability would exist for FDWs that do speak
libpq as well.
4. Wrapper X has enough overlap in options to create a legit connect string
which happens to speak libpq:
    Connection succeeds, and it's a feature not a bug.

  12. Re: dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

    Joe Conway <mail@joeconway.com> — 2016-12-22T17:29:15Z

    On 12/18/2016 02:47 PM, Corey Huinker wrote:
> On Sun, Dec 18, 2016 at 4:57 PM, Michael Paquier wrote:
>> On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway wrote:
>>> Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
>>> supports a libpq connection it would make sense to allows other FDWs
>>> with this attribute, but since there is none the current state strikes
>>> me as a bad idea.
>>> Thoughts?

>> libpq is proper to the implementation of the FDW, not the wrapper on
>> top of it, so using in the CREATE FDW command a way to do the
>> decision-making that does not look right to me. Filtering things at
>> connection attempt is a better solution.

> The only benefit I see would be giving the user a slightly more clear
> error message like ('dblink doesn't work with %', 'mysql') instead of
> the error about the failure of the connect string generated by the
> options that did overlap. 

Ok, I committed Corey's patch with only minor whitespace changes.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development