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

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

From

Corey Huinker

Date:

21 November 2016, 00:37:41

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);

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

From

Robert Haas

Date:

21 November 2016, 19:01:20

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

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

From

Tom Lane

Date:

21 November 2016, 19:16:38

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

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

From

Joe Conway

Date:

21 November 2016, 20:36:49

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

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

From

Corey Huinker

Date:

21 November 2016, 20:59:57

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.

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

From

Corey Huinker

Date:

22 November 2016, 19:43:30

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.

Attachment

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

From

Tom Lane

Date:

22 November 2016, 20:29:48

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

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

From

Corey Huinker

Date:

22 November 2016, 20:54:07

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 22, 2016 at 3:29 PM, Tom Lane <span
dir="ltr"><<ahref="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span> wrote:<br
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span
class="">CoreyHuinker <<a href="mailto:corey.huinker@gmail.com">corey.huinker@gmail.com</a>> writes:<br /> >
In9.4, I encountered a complaint about flex 2.6.0. After a little research<br /> > it seems that a fix for that made
itinto versions 9.3+, but not 9.4.<br /><br /></span>Er ... what?  See 1ba874505.<br /><br />                        
regards,tom lane<br /></blockquote></div><br /></div><div class="gmail_extra">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.</div><div
class="gmail_extra"><br/></div><div class="gmail_extra">I re-ran git pulls from my committed local branches to the
correspondingREL9_x_STABLE branch, and encountered no conflicts, so the 0001.dblink.* patches should still be
good.</div><divclass="gmail_extra"><br /></div><div class="gmail_extra"><br /></div></div>

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

From

Joe Conway

Date:

19 December 2016, 00:48:51

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

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

From

Michael Paquier

Date:

19 December 2016, 00:57:00

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

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

From

Corey Huinker

Date:

19 December 2016, 01:47:28

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.

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

From

Joe Conway

Date:

22 December 2016, 20:29:15

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