Horton

Thread

Commits

  1. Adjust test case added by commit 6136e94dc.

  2. libpq should expose GSS-related parameters even when not implemented.

  3. Superuser can permit passwordless connections on postgres_fdw

  1. pgsql: Superuser can permit passwordless connections on postgres_fdw

    Andrew Dunstan <andrew@dunslane.net> — 2019-12-20T05:55:10Z

    Superuser can permit passwordless connections on postgres_fdw

Currently postgres_fdw doesn't permit a non-superuser to connect to a
foreign server without specifying a password, or to use an
authentication mechanism that doesn't use the password. This is to avoid
using the settings and identity of the user running Postgres.

However, this doesn't make sense for all authentication methods. We
therefore allow a superuser to set "password_required 'false'" for user
mappings for the postgres_fdw. The superuser must ensure that the
foreign server won't try to rely solely on the server identity (e.g.
trust, peer, ident) or use an authentication mechanism that relies on the
password settings (e.g. md5, scram-sha-256).

This feature is a prelude to better support for sslcert and sslkey
settings in user mappings.

Author: Craig Ringer.
Discussion: https://postgr.es/m/075135da-545c-f958-fed0-5dcb462d6dae@2ndQuadrant.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6136e94dcb88c50b6156aa646746565400e373d4

Modified Files
--------------
contrib/postgres_fdw/connection.c              | 42 +++++++++---
contrib/postgres_fdw/expected/postgres_fdw.out | 94 ++++++++++++++++++++++++++
contrib/postgres_fdw/option.c                  | 19 ++++++
contrib/postgres_fdw/sql/postgres_fdw.sql      | 86 +++++++++++++++++++++++
doc/src/sgml/postgres-fdw.sgml                 | 24 +++++++
5 files changed, 257 insertions(+), 8 deletions(-)

  2. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Michael Paquier <michael@paquier.xyz> — 2019-12-20T12:02:08Z

    Hi Andrew,

On Fri, Dec 20, 2019 at 05:55:10AM +0000, Andrew Dunstan wrote:
> Superuser can permit passwordless connections on postgres_fdw
> 
> Currently postgres_fdw doesn't permit a non-superuser to connect to a
> foreign server without specifying a password, or to use an
> authentication mechanism that doesn't use the password. This is to avoid
> using the settings and identity of the user running Postgres.
> 
> However, this doesn't make sense for all authentication methods. We
> therefore allow a superuser to set "password_required 'false'" for user
> mappings for the postgres_fdw. The superuser must ensure that the
> foreign server won't try to rely solely on the server identity (e.g.
> trust, peer, ident) or use an authentication mechanism that relies on the
> password settings (e.g. md5, scram-sha-256).
> 
> This feature is a prelude to better support for sslcert and sslkey
> settings in user mappings.

After this commit a couple of buildfarm animals are unhappy with the
regression tests of postgres_fdw:
 CREATE ROLE nosuper NOSUPERUSER;
+WARNING:  roles created by regression test cases should have names
 starting with "regress_"
 GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO nosuper;
It is a project policy to only user roles prefixed by "regress_" in
regression tests.

These is also a second type of failure:
-HINT:  Valid options in this context are: [...] krbsrvname [...]
+HINT:  Valid options in this context are: [...]
The diff here is that krbsrvname is not part of the list of valid
options.  Anyway, as this list is build-dependent, I think that this
test needs some more design effort.
--
Michael

  3. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-20T19:04:37Z

    [ redirecting to -hackers ]

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Dec 20, 2019 at 05:55:10AM +0000, Andrew Dunstan wrote:
>> Superuser can permit passwordless connections on postgres_fdw

> After this commit a couple of buildfarm animals are unhappy with the
> regression tests of postgres_fdw:

Yeah, the buildfarm is *very* unhappy with this.

>  CREATE ROLE nosuper NOSUPERUSER;
> +WARNING:  roles created by regression test cases should have names
>  starting with "regress_"

That one is just failure to follow the guidelines, and is easily
fixed by adjusting the test case.

> These is also a second type of failure:
> -HINT:  Valid options in this context are: [...] krbsrvname [...]
> +HINT:  Valid options in this context are: [...]
> The diff here is that krbsrvname is not part of the list of valid
> options.  Anyway, as this list is build-dependent, I think that this
> test needs some more design effort.

This is a bit messier.  But I think that the discrepancy is not
really the fault of this patch: rather, it's a bug in the way the
GSS support was put into libpq.  I thought we had a policy that
all builds would recognize all possible parameters and then
perhaps fail later.  Certainly the SSL parameters are implemented
that way.  The #if's disabling GSS stuff in PQconninfoOptions[]
are just broken, according to that policy.

			regards, tom lane

  4. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-20T19:42:22Z

    I wrote:
> This is a bit messier.  But I think that the discrepancy is not
> really the fault of this patch: rather, it's a bug in the way the
> GSS support was put into libpq.  I thought we had a policy that
> all builds would recognize all possible parameters and then
> perhaps fail later.  Certainly the SSL parameters are implemented
> that way.  The #if's disabling GSS stuff in PQconninfoOptions[]
> are just broken, according to that policy.

Concretely, I think we ought to do (and back-patch) the attached.

I notice in testing this that the "nosuper" business added by
6136e94dc is broken in more ways than what the buildfarm is
complaining about: it leaves the role around at the end of the
test.  That's a HUGE violation of project policy, for security
reasons as well as the fact that it makes it impossible to run
"make installcheck" twice without getting different results.

			regards, tom lane

  5. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Michael Paquier <michael@paquier.xyz> — 2019-12-21T02:18:06Z

    On Fri, Dec 20, 2019 at 02:42:22PM -0500, Tom Lane wrote:
> Concretely, I think we ought to do (and back-patch) the attached.

Thanks for the fix, I have not been able to look at that.

> I notice in testing this that the "nosuper" business added by
> 6136e94dc is broken in more ways than what the buildfarm is
> complaining about: it leaves the role around at the end of the
> test.  That's a HUGE violation of project policy, for security
> reasons as well as the fact that it makes it impossible to run
> "make installcheck" twice without getting different results.

Roles left behind at the end of a test are annoying.  Here is an idea:
make pg_regress check if any roles prefixed by "regress_" are left
behind at the end of a test.  This will not work until test_pg_dump is
cleaned up, just a thought.
--
Michael

  6. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-21T03:17:20Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Dec 20, 2019 at 02:42:22PM -0500, Tom Lane wrote:
>> I notice in testing this that the "nosuper" business added by
>> 6136e94dc is broken in more ways than what the buildfarm is
>> complaining about: it leaves the role around at the end of the
>> test.

> Roles left behind at the end of a test are annoying.  Here is an idea:
> make pg_regress check if any roles prefixed by "regress_" are left
> behind at the end of a test.  This will not work until test_pg_dump is
> cleaned up, just a thought.

Yeah, it's sort of annoying that the buildfarm didn't notice this
aspect of things.  I'm not sure I want to spend cycles on checking
it in every test run, though.

Maybe we could have -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
enable a check for that aspect along with what it does now?

			regards, tom lane

  7. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Michael Paquier <michael@paquier.xyz> — 2019-12-25T02:25:54Z

    On Fri, Dec 20, 2019 at 10:17:20PM -0500, Tom Lane wrote:
> Yeah, it's sort of annoying that the buildfarm didn't notice this
> aspect of things.  I'm not sure I want to spend cycles on checking
> it in every test run, though.
> 
> Maybe we could have -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
> enable a check for that aspect along with what it does now?

Makes sense to restrict that under the flag.  Perhaps a catalog scan
of pg_authid at the end of pg_regress and isolationtester then?
--
Michael

  8. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-12-26T22:51:29Z

    On Wed, Dec 25, 2019 at 12:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Dec 20, 2019 at 10:17:20PM -0500, Tom Lane wrote:
> > Yeah, it's sort of annoying that the buildfarm didn't notice this
> > aspect of things.  I'm not sure I want to spend cycles on checking
> > it in every test run, though.
> >
> > Maybe we could have -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
> > enable a check for that aspect along with what it does now?
>
> Makes sense to restrict that under the flag.  Perhaps a catalog scan
> of pg_authid at the end of pg_regress and isolationtester then?


What's the preferred way to set that?

"configure CPPFLAGS=-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS"?

Maybe I should add that to the sample buildfarm config ...

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: pgsql: Superuser can permit passwordless connections on postgres_fdw

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-27T00:07:17Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> What's the preferred way to set that?
> "configure CPPFLAGS=-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS"?

longfin is doing it via config_env.  I have no opinion on whether
that's the "preferred" way.

			regards, tom lane