expand_dbname in postgres_fdw

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]https://www.postgresql.org/docs/10/static/postgres-fdw.html#idm44880567492496 -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company 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
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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]https://www.postgresql.org/docs/10/static/postgres-fdw.html#idm44880567492496 -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company 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
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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 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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.