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