Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
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
signature.asc
Description: OpenPGP digital signature
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
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.
> --
> 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.
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. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
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 signature.asc Description: OpenPGP digital signature
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
On Tue, Nov 22, 2016 at 3:29 PM, Tom Lane wrote: > Corey Huinker 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.
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
Corey Huinker 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
>
>
>
>> 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.
diff --git a/configure b/configure
index 5204869..7da2dac 100755
--- a/configure
+++ b/configure
@@ -7166,7 +7166,7 @@ else
echo '%%' > conftest.l
if $pgac_candidate -t conftest.l 2>/dev/null | grep FLEX_SCANNER
>/dev/null 2>&1; then
pgac_flex_version=`$pgac_candidate --version 2>/dev/null`
- if echo "$pgac_flex_version" | sed 's/[.a-z]/ /g' | $AWK '{ if ($1 =
2 && $2 = 5 && $3 >= 31) exit 0; else exit 1;}'
+ if echo "$pgac_flex_version" | sed 's/[.a-z]/ /g' | $AWK '{ if ($1
== 2 && ($2 > 5 || ($2 == 5 && $3 >= 31))) exit 0; else exit 1;}'
then
pgac_cv_path_flex=$pgac_candidate
break 2
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index af062fa..491dec8 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -40,6 +40,7 @@
#include "access/reloptions.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_type.h"
#include "catalog/pg_user_mapping.h"
@@ -2731,6 +2732,24 @@ get_connect_string(const char *servername)
ForeignDataWrapper *fdw;
AclResult aclresult;
char *srvname;
+ static const PQconninfoOption *options = NULL;
+
+ /*
+* Get list of valid libpq options.
+*
+* To avoid unnecessary work, we get the list once and use it throughout
+* the lifetime of this backend process. We don't need to care about
+* memory context issues, because PQconndefaults allocates with malloc.
+*/
+ if (!options)
+ {
+ options = PQconndefaults();
+ if (!options) /* assume reason for failure is
OOM */
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
+errmsg("out of memory"),
+errdetail("could not get libpq's default connection
options")));
+ }
/* first gather the server connstr options */
srvname = pstrdup(servername);
@@ -2755,16 +2774,18 @@ get_connect_string(const char *servername)
{
DefElem*def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
-
escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname,
ForeignDataWrapperRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+
escape_param_str(strVal(def->arg)));
}
foreach(cell, foreign_server->options)
{
DefElem*def = lfirst(cell);
- appendStringInfo(buf, "%s='%s' ", def->defname,
-
escape_param_str(strVal(def->arg)));
+ if ( is_valid_dblink_option(options, def->defname,
ForeignServerRelationId ) )
+ appendStringInfo(buf, "%s='%s' ", def->defname,
+
escape_param_str(strVal(def->arg)));
}
foreach(cell, user_mapping->
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
> > 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: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
On 11/21/2016 02:16 PM, Tom Lane wrote:
> Corey Huinker 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
signature.asc
Description: OpenPGP digital signature
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
Corey Huinker 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
On Sun, Nov 20, 2016 at 7:37 PM, Corey Huinker 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.
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);
