Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
postgres_fdw_sslparms-1.patchtext/x-patch; name=postgres_fdw_sslparms-1.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 298f652afc..c2661320bc 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -174,6 +174,18 @@ WARNING: extension "bar" is not installed
ALTER SERVER testserver1 OPTIONS (DROP extensions);
ALTER USER MAPPING FOR public SERVER testserver1
OPTIONS (DROP user, DROP password);
+-- Attempt to add a valid option that's not allowed in a user mapping
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslmode 'require');
+ERROR: invalid option "sslmode"
+HINT: Valid options in this context are: user, password, sslpassword, password_required, sslcert, sslkey
+-- But we can add valid ones fine
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslpassword 'dummy');
+-- Ensure valid options we haven't used in a user mapping yet are
+-- permitted to check validation.
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 07fc7fa00a..0f35dfe54f 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -196,6 +196,16 @@ InitPgFdwOptions(void)
{"fetch_size", ForeignServerRelationId, false},
{"fetch_size", ForeignTableRelationId, false},
{"password_required", UserMappingRelationId, false},
+ /*
+ * Extra room for the user mapping copies of sslcert and sslkey. These
+ * are really libpq options but we repeat them here to allow them to
+ * appear in both foreign server context (when we generate libpq
+ * options) and user mapping context (from here). Bit of a hack
+ * putting this in "non_libpq_options".
+ */
+ {"sslcert", UserMappingRelationId, true},
+ {"sslkey", UserMappingRelationId, true},
+
{NULL, InvalidOid, false}
};
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 5f50c65566..bfdd5dd4d2 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -188,6 +188,19 @@ ALTER SERVER testserver1 OPTIONS (DROP extensions);
ALTER USER MAPPING FOR public SERVER testserver1
OPTIONS (DROP user, DROP password);
+-- Attempt to add a valid option that's not allowed in a user mapping
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslmode 'require');
+
+-- But we can add valid ones fine
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslpassword 'dummy');
+
+-- Ensure valid options we haven't used in a user mapping yet are
+-- permitted to check validation.
+ALTER USER MAPPING FOR public SERVER testserver1
+ OPTIONS (ADD sslkey 'value', ADD sslcert 'value');
+
ALTER FOREIGN TABLE ft1 OPTIONS (schema_name 'S 1', table_name 'T 1');
ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1');
ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1');
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index bdd6ff8ae4..ef203d225c 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -107,13 +107,13 @@
A foreign server using the <filename>postgres_fdw</filename> foreign data wrapper
can have the same options that <application>libpq</application> accepts in
connection strings, as described in <xref linkend="libpq-paramkeywords"/>,
- except that these options are not allowed:
+ except that these options are not allowed or have special handling:
<itemizedlist spacing="compact">
<listitem>
<para>
<literal>user</literal>, <literal>password</literal> and <literal>sslpassword</literal> (specify these
- in a user mapping, instead)
+ in a user mapping, instead, or use a service file)
</para>
</listitem>
<listitem>
@@ -128,6 +128,14 @@
<literal>postgres_fdw</literal>)
</para>
</listitem>
+ <listitem>
+ <para>
+ <literal>sslkey</literal> and <literal>sslpassword</literal> - these may
+ appear in <emphasis>either or both</emphasis> a connection and a user
+ mapping. If both are present, the user mapping setting overrides the
+ connection setting.
+ </para>
+ </listitem>
</itemizedlist>
</para>
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.
The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.
--
Michael
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.
That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-12-02 00:12, Andrew Dunstan wrote:
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.
The prerequisite patch has been committed, so please see about getting
this patch moving forward.
The patch is very small, of course, but I don't understand the "bit of a
hack" comment. Could you explain that?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jan 8, 2020 at 7:36 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2019-12-02 00:12, Andrew Dunstan wrote:
On 11/30/19 8:48 PM, Michael Paquier wrote:
On Thu, Oct 31, 2019 at 07:54:41PM -0400, Andrew Dunstan wrote:
This patch achieves $SUBJECT and also provides some testing of the
sslpassword setting.The patch does not apply anymore, so a rebase is needed. As it has
not been reviewed, I am moving it to next CF, waiting on author.That's OK. This patch is dependent, as it always has been, on the patch
to allow passwordless user mappings for postgres_fdw. I hope to commit
that soon, but I'd prefer some signoff from prominent hackers, as I
don't want to go too far down this road and then encounter a bunch of
objections.The prerequisite patch has been committed, so please see about getting
this patch moving forward.The patch is very small, of course, but I don't understand the "bit of a
hack" comment. Could you explain that?
I have rewritten the comment, and committed the feature.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Andrew Dunstan 2019-11-01 <f941b95e-27ad-cb5c-2495-13c44f90b1bc@2ndQuadrant.com>
{"password_required", UserMappingRelationId, false}, + /* + * Extra room for the user mapping copies of sslcert and sslkey. These + * are really libpq options but we repeat them here to allow them to + * appear in both foreign server context (when we generate libpq + * options) and user mapping context (from here). Bit of a hack + * putting this in "non_libpq_options". + */ + {"sslcert", UserMappingRelationId, true}, + {"sslkey", UserMappingRelationId, true},
Nice feature, we were actually looking for exactly this yesterday.
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.
In short, I believe these options need explicit superuser checks.
Christoph
Re: To Andrew Dunstan 2020-01-09 <20200109103014.GA4192@msg.df7cb.de>
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem.
(A nice addition here which would avoid the problems would be the
possibility to pass in the certificates as strings, but that needs
support in libpq.)
Christoph
Re: To Andrew Dunstan 2020-01-09 <20200109103014.GA4192@msg.df7cb.de>
I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.
They are indeed:
stat("/var/lib/postgresql/.postgresql/root.crt", 0x7ffcff3e2bb0) = -1 ENOENT (Datei oder Verzeichnis nicht gefunden)
stat("/foo", 0x7ffcff3e2bb0) = -1 ENOENT (Datei oder Verzeichnis nicht gefunden)
^^^^ sslcert
I'm not sure if that could be exploited in any way, but let's just
forbid it.
Christoph
On Thu, Jan 9, 2020 at 5:30 AM Christoph Berg <myon@debian.org> wrote:
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.In short, I believe these options need explicit superuser checks.
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jan 9, 2020 at 5:30 AM Christoph Berg <myon@debian.org> wrote:
I have some concerns about security, though. It's true that the
sslcert/sslkey options can only be set/modified by superusers when
"password_required" is set. But when password_required is not set, any
user and create user mappings that reference arbitrary files on the
server filesystem. I believe the options are still used in that case
for creating connections, even when that means the remote server isn't
set up for cert auth, which needs password_required=false to succeed.In short, I believe these options need explicit superuser checks.
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 9 Jan 2020, at 22:38, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
I've seen successful exploits made from parts that I in my wildest imagination
couldn't think be useful, so FWIW +1 for adding belts to suspenders and
restricting this.
cheers ./daniel
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.
It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.
Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.
regards, tom lane
On Fri, Jan 10, 2020 at 8:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.
OK, that's convincing enough. Will do it before long.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 10 Jan 2020 at 06:16, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
wrote:
On Fri, Jan 10, 2020 at 8:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On Fri, Jan 10, 2020 at 1:21 AM Robert Haas <robertmhaas@gmail.com>
wrote:
I share the concern about the security issue here. I can't testify to
whether Christoph's whole analysis is here, but as a general point,
non-superusers can't be allowed to do things that cause the server to
access arbitrary local files.It's probably fairly easy to do (c.f. 6136e94dcb). I'm not (yet)
convinced that there is any significant security threat here. This
doesn't give the user or indeed any postgres code any access to the
contents of these files. But if there is a consensus to restrict this
I'll do it.Well, even without access to the file contents, the mere ability to
probe the existence of a file is something we don't want unprivileged
users to have. And (I suppose) this is enough for that, by looking
at what error you get back from trying it.OK, that's convincing enough. Will do it before long.
Thanks. I'm 100% convinced the superuser restriction should be imposed. I
can imagine there being a risk of leaking file contents in error output
such as parse errors from OpenSSL that we pass on for example. Tricking Pg
into reading from a fifo could be problematic too.
I should've applied that restriction from the start, the same way as
passwordless connections are restricted.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise