Thread: [HACKERS] expand_dbname in postgres_fdw
Hi,
Is there any particular reason why postgres_fdw forbids usage of
connection strings by passing expand_dbname = false to
PQconnectdbParams? Attached patch shows where this happens.
From 6bf3741976b833379f5bb370aa41f73a51b99afd Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Tue, 25 Jul 2017 21:36:45 +0300
Subject: [PATCH] expand_dbname=true in postgres_fdw
---
contrib/postgres_fdw/connection.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index be4ec07cf9..bfcd9ed2e3 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -263,7 +263,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* verify connection parameters and make connection */
check_conn_params(keywords, values);
- conn = PQconnectdbParams(keywords, values, false);
+ conn = PQconnectdbParams(keywords, values, true);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
--
2.11.0
--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 26, 2017 at 12:17 AM, Arseny Sher <a.sher@postgrespro.ru> wrote: > Hi, > > Is there any particular reason why postgres_fdw forbids usage of > connection strings by passing expand_dbname = false to > PQconnectdbParams? Attached patch shows where this happens. > According to F.34.1.1 at [1] passing connection string as dbname option should work, so your question is valid. I am not aware of any discussion around this on hackers. Comments in connect_pg_server() don't help either. But I guess, we expect users to set up individual foreign server and user mapping options instead of putting those in a connection string. I can not think of any reason except that it improves readability. If postgres_fdw wants to take certain actions based on the values of individual options, having them separate is easier to handle than parsing them out of a connection string. Any way, if we are not going to change current behaviour, we should change the documentation and say that option dbname means "database name" and not a connection string. [1] https://www.postgresql.org/docs/10/static/postgres-fdw.html#idm44880567492496 -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Wed, Jul 26, 2017 at 5:38 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > According to F.34.1.1 at [1] passing connection string as dbname > option should work, so your question is valid. I am not aware of any > discussion around this on hackers. Comments in connect_pg_server() > don't help either. But I guess, we expect users to set up individual > foreign server and user mapping options instead of putting those in a > connection string. I can not think of any reason except that it > improves readability. If postgres_fdw wants to take certain actions > based on the values of individual options, having them separate is > easier to handle than parsing them out of a connection string. > > Any way, if we are not going to change current behaviour, we should > change the documentation and say that option dbname means "database > name" and not a connection string. I kind of wonder if this had some security aspect to it? But not sure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Wed, Jul 26, 2017 at 5:38 AM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: > > According to F.34.1.1 at [1] passing connection string as dbname > > option should work, so your question is valid. I am not aware of any > > discussion around this on hackers. Comments in connect_pg_server() > > don't help either. But I guess, we expect users to set up individual > > foreign server and user mapping options instead of putting those in a > > connection string. I can not think of any reason except that it > > improves readability. If postgres_fdw wants to take certain actions > > based on the values of individual options, having them separate is > > easier to handle than parsing them out of a connection string. > > > > Any way, if we are not going to change current behaviour, we should > > change the documentation and say that option dbname means "database > > name" and not a connection string. > > I kind of wonder if this had some security aspect to it? But not sure. Yeah, me too. As I recall, if the flag is not set, parameters set by the FDW server earlier in the conninfo can be changed by params that appear in the dbname. Offhand I can't see any obvious security implications, but then I've not thought about it very hard. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jul 26, 2017 at 5:38 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> According to F.34.1.1 at [1] passing connection string as dbname
>> option should work, so your question is valid. I am not aware of any
>> discussion around this on hackers.
> I kind of wonder if this had some security aspect to it? But not sure.
The main problem to my mind is that a connection string could possibly
override items meant to be specified elsewhere. In particular it ought
not be allowed to specify the remote username or password, because those
are supposed to come from the user mapping object not the server object.
I suspect you could break things by trying to specify client_encoding
there, as well.
In any case, I entirely reject the argument that the existing
documentation says this should work. It says that you can specify (most
of) the same fields that are allowed in a connection string, not that one
of those fields might be taken to *be* a connection string.
regards, tom lane
On Thu, Jul 27, 2017 at 12:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Jul 26, 2017 at 5:38 AM, Ashutosh Bapat >> <ashutosh.bapat@enterprisedb.com> wrote: >>> According to F.34.1.1 at [1] passing connection string as dbname >>> option should work, so your question is valid. I am not aware of any >>> discussion around this on hackers. > >> I kind of wonder if this had some security aspect to it? But not sure. > > The main problem to my mind is that a connection string could possibly > override items meant to be specified elsewhere. In particular it ought > not be allowed to specify the remote username or password, because those > are supposed to come from the user mapping object not the server object. > I suspect you could break things by trying to specify client_encoding > there, as well. +1. > > In any case, I entirely reject the argument that the existing > documentation says this should work. It says that you can specify (most > of) the same fields that are allowed in a connection string, not that one > of those fields might be taken to *be* a connection string. > Section F.34.1.1. at [1] says "A foreign server using the postgres_fdw foreign data wrapper can have the same options that libpq accepts in connection strings, as described in Section 33.1.2, except that these options are not allowed:". When it says, " accepts same options", users would interpret it as "accept in the same manner as specified in the referenced section". Also, dbname is not one of the listed exceptions, so a user would expect same behaviour when the same value for dbname option is provided in foreign server options and libpq connection string. In the referenced section "dbname" is described as -- dbname The database name. Defaults to be the same as the user name. In certain contexts, the value is checked for extended formats; see Section 33.1.1 for more details on those. -- There is some grey area where different people will interpret those sentences in different manner. So, may be better to say that "dbname" option in foreign server accepts only database names. [1] https://www.postgresql.org/docs/10/static/postgres-fdw.html#idm44880567492496 -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Thu, Jul 27, 2017 at 12:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The main problem to my mind is that a connection string could possibly
> override items meant to be specified elsewhere. In particular it ought
> not be allowed to specify the remote username or password, because those
> are supposed to come from the user mapping object not the server object.
> I suspect you could break things by trying to specify client_encoding
> there, as well.
Attached patch allows dbname expansion and makes sure that it doesn't
contain any invalid options. Whether you decide to commit it or not
(at the moment I don't see any security implications, at least not more than
in usual dbname expansion usage, e.g. in psql, but who knows), it seems
to me that the documentation should be updated since currently it is not
clear on the subject, as the beginning of this thread proves.
From 09e3924501d219331b8b8fca12a9ea35a689ba10 Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Thu, 27 Jul 2017 12:41:17 +0300
Subject: [PATCH] Allow 'dbname' option contain full-fledged connstring in
postgres_fdw
And if this is the case, validate that it doesn't contain any invalid options
such as client_encoding or user.
---
contrib/postgres_fdw/connection.c | 2 +-
contrib/postgres_fdw/option.c | 80 ++++++++++++++++++++++++++++++++-------
2 files changed, 68 insertions(+), 14 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index be4ec07cf9..bfcd9ed2e3 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -263,7 +263,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* verify connection parameters and make connection */
check_conn_params(keywords, values);
- conn = PQconnectdbParams(keywords, values, false);
+ conn = PQconnectdbParams(keywords, values, true);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 67e1c59951..26c11e5179 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -50,8 +50,10 @@ static PQconninfoOption *libpq_options;
* Helper functions
*/
static void InitPgFdwOptions(void);
+static void validate_dbname(const char *dbname);
+static void get_opts_hint(StringInfo buf, Oid context, bool libpq_options);
static bool is_valid_option(const char *keyword, Oid context);
-static bool is_libpq_option(const char *keyword);
+static bool is_libpq_option(const char *keyword, Oid context);
/*
@@ -86,16 +88,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
* Unknown option specified, complain about it. Provide a hint
* with list of valid options for the object.
*/
- PgFdwOption *opt;
StringInfoData buf;
-
- initStringInfo(&buf);
- for (opt = postgres_fdw_options; opt->keyword; opt++)
- {
- if (catalog == opt->optcontext)
- appendStringInfo(&buf, "%s%s", (buf.len > 0) ? ", " : "",
- opt->keyword);
- }
+ get_opts_hint(&buf, catalog, false);
ereport(ERROR,
(errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
@@ -143,6 +137,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
errmsg("%s requires a non-negative integer value",
def->defname)));
}
+ else if (strcmp(def->defname, "dbname") == 0)
+ {
+ validate_dbname(defGetString(def));
+ }
}
PG_RETURN_VOID();
@@ -250,6 +248,59 @@ InitPgFdwOptions(void)
}
/*
+ * If dbname param specified, make sure it doesn't carry invalid
+ * options if it is a connstring.
+ */
+static void
+validate_dbname(const char *dbname)
+{
+ PQconninfoOption *lopts;
+ PQconninfoOption *lopt;
+
+ /* If it is not a connstring, just skip the check */
+ if ((lopts = PQconninfoParse(dbname, NULL)) != NULL)
+ {
+ for (lopt = lopts; lopt->keyword; lopt++)
+ {
+ if (lopt->val != NULL && !is_libpq_option(lopt->keyword,
+ ForeignServerRelationId))
+ {
+ StringInfoData buf;
+ get_opts_hint(&buf, ForeignServerRelationId, true);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_FDW_INVALID_OPTION_NAME),
+ errmsg("invalid option in dbname connstring \"%s\"",
+ lopt->keyword),
+ errhint("Valid options in this context are: %s",
+ buf.data)));
+ }
+ }
+ PQconninfoFree(lopts);
+ }
+}
+
+/*
+ * Put to 'buf' a hint with list of valid options for the object 'context'. If
+ * libpq_options is true, only libpq options are provided. 'buf' must point to
+ * existing StringInfoData struct when called.
+ */
+static void
+get_opts_hint(StringInfo buf, Oid context, bool libpq_options)
+{
+ PgFdwOption *opt;
+
+ initStringInfo(buf);
+ for (opt = postgres_fdw_options; opt->keyword; opt++)
+ {
+ if (context == opt->optcontext &&
+ (!libpq_options || opt->is_libpq_opt))
+ appendStringInfo(buf, "%s%s", (buf->len > 0) ? ", " : "",
+ opt->keyword);
+ }
+}
+
+/*
* Check whether the given option is one of the valid postgres_fdw options.
* context is the Oid of the catalog holding the object the option is for.
*/
@@ -271,9 +322,11 @@ is_valid_option(const char *keyword, Oid context)
/*
* Check whether the given option is one of the valid libpq options.
+ * context is the Oid of the catalog holding the object the option is for;
+ * if it is InvalidOid, don't check the catalog.
*/
static bool
-is_libpq_option(const char *keyword)
+is_libpq_option(const char *keyword, Oid context)
{
PgFdwOption *opt;
@@ -281,7 +334,8 @@ is_libpq_option(const char *keyword)
for (opt = postgres_fdw_options; opt->keyword; opt++)
{
- if (opt->is_libpq_opt && strcmp(opt->keyword, keyword) == 0)
+ if (opt->is_libpq_opt && strcmp(opt->keyword, keyword) == 0 &&
+ (context == InvalidOid || context == opt->optcontext))
return true;
}
@@ -308,7 +362,7 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
{
DefElem *d = (DefElem *) lfirst(lc);
- if (is_libpq_option(d->defname))
+ if (is_libpq_option(d->defname, InvalidOid))
{
keywords[i] = d->defname;
values[i] = defGetString(d);
--
2.11.0
--
Arseny Sher
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arseny Sher <a.sher@postgrespro.ru> writes:
> Attached patch allows dbname expansion and makes sure that it doesn't
> contain any invalid options.
I'm pretty much against this in principle. It complicates both the code
and the conceptual API, for no serious gain, even if you take it on faith
that it doesn't and never will produce any security issues.
> Whether you decide to commit it or not
> (at the moment I don't see any security implications, at least not more than
> in usual dbname expansion usage, e.g. in psql, but who knows), it seems
> to me that the documentation should be updated since currently it is not
> clear on the subject, as the beginning of this thread proves.
I really don't see anything wrong with the FDW's documentation. To claim
that it's not clear, you have to suppose that a connstring's dbname field
is allowed to recursively contain a connstring. However, if you've got a
concrete suggestion about improving the wording, let's see it.
Now on the other hand, libpq's documentation seems a little confusing
on this point independently of the FDW: so far as I can see, what "certain
contexts" means is not clearly defined anywhere, and for that matter
"checked for extended formats" is a masterpiece of unhelpful obfuscation.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes:
> I really don't see anything wrong with the FDW's documentation. To claim
> that it's not clear, you have to suppose that a connstring's dbname field
> is allowed to recursively contain a connstring. However, if you've got a
> concrete suggestion about improving the wording, let's see it.
>
> Now on the other hand, libpq's documentation seems a little confusing
> on this point independently of the FDW: so far as I can see, what "certain
> contexts" means is not clearly defined anywhere, and for that matter
> "checked for extended formats" is a masterpiece of unhelpful obfuscation.
>
> regards, tom lane
I have to admit that you are right: strictly speaking, FDW doc says it
accepts the same options as libpq connstring => libpq connstring itself
can't contain another connstring => expansion is not allowed. However,
we might probably save the readers a few mental cycles if we avoid the
words 'connection strings' and just say that recognized options are the
same as of libpq ones, with (probably, see below) an explicit addition
that dbname is not expanded.
Regarding "checking for extended formats" phrase, I see three solutions:
1) In the description of 'dbname' parameter mention all the cases where
it is possibly expanded, which doesn't sound as a good idea;
2) Specify whether dbname is expanded in every reference to the list of
libpq options, which is arguably better.
3) We can also replace "In certain contexts" with "Where explicitly
mentioned" in the desciption of 'dbname', and, while referencing
options, never say anything about dbname if it is not expanded.
Besides, why not substitute "checked for extended formats" with "might
be recognized as a connection string" -- are there any other formats
than connstr?
The changes with point 3 chosen might look as in attached file.
From 96d74f050724a8373c04e48205510a5a7e80d447 Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Thu, 27 Jul 2017 22:45:04 +0300
Subject: [PATCH] libpq docs: dbname param is expanded only when explicitly
mentioned.
Also, avoid mentioning connstring in the desciption of parameters accepted by
postgres_fdw server.
---
doc/src/sgml/libpq.sgml | 7 +++----
doc/src/sgml/postgres-fdw.sgml | 6 +++---
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ad5e9b95b4..dde6647fc5 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1040,10 +1040,9 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
<term><literal>dbname</literal></term>
<listitem>
<para>
- The database name. Defaults to be the same as the user name.
- In certain contexts, the value is checked for extended
- formats; see <xref linkend="libpq-connstring"> for more details on
- those.
+ The database name. Defaults to be the same as the user name. Where
+ explicitly said, the value can be recognized as a connection string;
+ see <xref linkend="libpq-connstring"> for more details on those.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index d83fc9e52b..e3c41bfd97 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -100,9 +100,9 @@
<para>
A foreign server using the <filename>postgres_fdw</> foreign data wrapper
- can have the same options that <application>libpq</> accepts in
- connection strings, as described in <xref linkend="libpq-paramkeywords">,
- except that these options are not allowed:
+ can have the same options that are recognized by <application>libpq</>, as
+ described in <xref linkend="libpq-paramkeywords">, except that these
+ options are not allowed:
<itemizedlist spacing="compact">
<listitem>
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers