Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iiOAQ-0007Ke-40 for pgsql-hackers@arkaria.postgresql.org; Fri, 20 Dec 2019 19:42:30 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iiOAO-0002BM-Cj for pgsql-hackers@arkaria.postgresql.org; Fri, 20 Dec 2019 19:42:28 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iiOAN-0002B7-S2 for pgsql-hackers@lists.postgresql.org; Fri, 20 Dec 2019 19:42:28 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1iiOAK-0001IC-QD for pgsql-hackers@lists.postgresql.org; Fri, 20 Dec 2019 19:42:26 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id xBKJgMr4032151; Fri, 20 Dec 2019 14:42:22 -0500 From: Tom Lane To: Andrew Dunstan cc: Michael Paquier , pgsql-hackers@lists.postgresql.org Subject: Re: pgsql: Superuser can permit passwordless connections on postgres_fdw In-reply-to: <11297.1576868677@sss.pgh.pa.us> References: <20191220120208.GA4258@paquier.xyz> <11297.1576868677@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Fri, 20 Dec 2019 14:04:37 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <32124.1576870916.0@sss.pgh.pa.us> Date: Fri, 20 Dec 2019 14:42:22 -0500 Message-ID: <32150.1576870942@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <32124.1576870916.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="accept-libpq-gss-parameters-unconditionally.patch"; charset="us-ascii" Content-ID: <32124.1576870916.2@sss.pgh.pa.us> Content-Description: accept-libpq-gss-parameters-unconditionally.patch Content-Transfer-Encoding: quoted-printable diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/post= gres_fdw/expected/postgres_fdw.out index 3d6e4ee..ed52ddd 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -132,7 +132,7 @@ CREATE FOREIGN TABLE ft6 ( -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D -- tests for validator -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- requiressl, krbsrvname and gsslib are omitted because they depend on +-- requiressl is omitted because valid values depend on -- configure options ALTER SERVER testserver1 OPTIONS ( use_remote_estimate 'false', @@ -158,10 +158,10 @@ ALTER SERVER testserver1 OPTIONS ( sslcert 'value', sslkey 'value', sslrootcert 'value', - sslcrl 'value' + sslcrl 'value', --requirepeer 'value', - -- krbsrvname 'value', - -- gsslib 'value', + krbsrvname 'value', + gsslib 'value' --replication 'value' ); -- Error, invalid list syntax @@ -8855,7 +8855,7 @@ DO $d$ END; $d$; ERROR: invalid option "password" -HINT: Valid options in this context are: service, passfile, channel_bind= ing, connect_timeout, dbname, host, hostaddr, port, options, application_n= ame, keepalives, keepalives_idle, keepalives_interval, keepalives_count, t= cp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, ss= lcrl, requirepeer, gssencmode, krbsrvname, target_session_attrs, use_remot= e_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch= _size +HINT: Valid options in this context are: service, passfile, channel_bind= ing, connect_timeout, dbname, host, hostaddr, port, options, application_n= ame, keepalives, keepalives_idle, keepalives_interval, keepalives_count, t= cp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, ss= lcrl, requirepeer, gssencmode, krbsrvname, gsslib, target_session_attrs, u= se_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatabl= e, fetch_size CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password= 'dummypw')" PL/pgSQL function inline_code_block line 3 at EXECUTE -- If we add a password for our user mapping instead, we should get a dif= ferent diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_= fdw/sql/postgres_fdw.sql index 33307d6..e0f4c72 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -145,7 +145,7 @@ CREATE FOREIGN TABLE ft6 ( -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D -- tests for validator -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- requiressl, krbsrvname and gsslib are omitted because they depend on +-- requiressl is omitted because valid values depend on -- configure options ALTER SERVER testserver1 OPTIONS ( use_remote_estimate 'false', @@ -171,10 +171,10 @@ ALTER SERVER testserver1 OPTIONS ( sslcert 'value', sslkey 'value', sslrootcert 'value', - sslcrl 'value' + sslcrl 'value', --requirepeer 'value', - -- krbsrvname 'value', - -- gsslib 'value', + krbsrvname 'value', + gsslib 'value' --replication 'value' ); = diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 4325946..66b09da 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1747,8 +1747,10 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname gsslib - GSS library to use for GSSAPI authentication. Only used on Window= s. - Set to gssapi to force libpq to use the GSSAPI + GSS library to use for GSSAPI authentication. + Currently this is disregarded except on Windows builds that inclu= de + both GSSAPI and SSPI support. In that case, set + this to gssapi to cause libpq to use the GSSAP= I library for authentication instead of the default SSPI. diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-c= onnect.c index 3ca7e05..cb3c431 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -304,6 +304,10 @@ static const internalPQconninfoOption PQconninfoOptio= ns[] =3D { "SSL-Client-Key", "", 64, offsetof(struct pg_conn, sslkey)}, = + {"sslpassword", NULL, NULL, NULL, + "SSL-Client-Key-Password", "*", 20, + offsetof(struct pg_conn, sslpassword)}, + {"sslrootcert", "PGSSLROOTCERT", NULL, NULL, "SSL-Root-Certificate", "", 64, offsetof(struct pg_conn, sslrootcert)}, @@ -317,30 +321,21 @@ static const internalPQconninfoOption PQconninfoOpti= ons[] =3D { offsetof(struct pg_conn, requirepeer)}, = /* - * Expose gssencmode similarly to sslmode - we can still handle "disable= " - * and "prefer". + * As with SSL, all GSS options are exposed even in builds that don't ha= ve + * support. */ {"gssencmode", "PGGSSENCMODE", DefaultGSSMode, NULL, "GSSENC-Mode", "", 7, /* sizeof("disable") =3D=3D 7 */ offsetof(struct pg_conn, gssencmode)}, = -#if defined(ENABLE_GSS) || defined(ENABLE_SSPI) /* Kerberos and GSSAPI authentication support specifying the service nam= e */ {"krbsrvname", "PGKRBSRVNAME", PG_KRB_SRVNAM, NULL, "Kerberos-service-name", "", 20, offsetof(struct pg_conn, krbsrvname)}, -#endif - -#if defined(ENABLE_GSS) && defined(ENABLE_SSPI) = - /* - * GSSAPI and SSPI both enabled, give a way to override which is used by - * default - */ {"gsslib", "PGGSSLIB", NULL, NULL, "GSS-library", "", 7, /* sizeof("gssapi") =3D 7 */ offsetof(struct pg_conn, gsslib)}, -#endif = {"replication", NULL, NULL, NULL, "Replication", "D", 5, @@ -351,10 +346,6 @@ static const internalPQconninfoOption PQconninfoOptio= ns[] =3D { "Target-Session-Attrs", "", 11, /* sizeof("read-write") =3D 11 */ offsetof(struct pg_conn, target_session_attrs)}, = - {"sslpassword", NULL, NULL, NULL, - "SSL-Client-Key-Password", "*", 20, - offsetof(struct pg_conn, sslpassword)}, - /* Terminating entry --- MUST BE LAST */ {NULL, NULL, NULL, NULL, NULL, NULL, 0} @@ -3983,6 +3974,8 @@ freePGconn(PGconn *conn) free(conn->sslcert); if (conn->sslkey) free(conn->sslkey); + if (conn->sslpassword) + free(conn->sslpassword); if (conn->sslrootcert) free(conn->sslrootcert); if (conn->sslcrl) @@ -3991,14 +3984,14 @@ freePGconn(PGconn *conn) free(conn->sslcompression); if (conn->requirepeer) free(conn->requirepeer); - if (conn->connip) - free(conn->connip); if (conn->gssencmode) free(conn->gssencmode); -#if defined(ENABLE_GSS) || defined(ENABLE_SSPI) if (conn->krbsrvname) free(conn->krbsrvname); -#endif + if (conn->gsslib) + free(conn->gsslib); + if (conn->connip) + free(conn->connip); #ifdef ENABLE_GSS if (conn->gcred !=3D GSS_C_NO_CREDENTIAL) { @@ -4015,10 +4008,6 @@ freePGconn(PGconn *conn) conn->gctx =3D NULL; } #endif -#if defined(ENABLE_GSS) && defined(ENABLE_SSPI) - if (conn->gsslib) - free(conn->gsslib); -#endif /* Note that conn->Pfdebug is not ours to close or free */ if (conn->last_query) free(conn->last_query); @@ -4034,8 +4023,6 @@ freePGconn(PGconn *conn) free(conn->target_session_attrs); termPQExpBuffer(&conn->errorMessage); termPQExpBuffer(&conn->workBuffer); - if (conn->sslpassword) - free(conn->sslpassword); = free(conn); = diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq= -int.h index 7f5be7d..e99cbf3 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -359,13 +359,14 @@ struct pg_conn char *sslcompression; /* SSL compression (0 or 1) */ char *sslkey; /* client key filename */ char *sslcert; /* client certificate filename */ + char *sslpassword; /* client key file password */ char *sslrootcert; /* root certificate filename */ char *sslcrl; /* certificate revocation list filename */ char *requirepeer; /* required peer credentials for local sockets */ - -#if defined(ENABLE_GSS) || defined(ENABLE_SSPI) + char *gssencmode; /* GSS mode (require,prefer,disable) */ char *krbsrvname; /* Kerberos service name */ -#endif + char *gsslib; /* What GSS library to use ("gssapi" or + * "sspi") */ = /* Type of connection to make. Possible values: any, read-write. */ char *target_session_attrs; @@ -484,7 +485,6 @@ struct pg_conn #endif /* USE_OPENSSL */ #endif /* USE_SSL */ = - char *gssencmode; /* GSS mode (require,prefer,disable) */ #ifdef ENABLE_GSS gss_ctx_id_t gctx; /* GSS context */ gss_name_t gtarg_nam; /* GSS target name */ @@ -496,10 +496,6 @@ struct pg_conn #endif = #ifdef ENABLE_SSPI -#ifdef ENABLE_GSS - char *gsslib; /* What GSS library to use ("gssapi" or - * "sspi") */ -#endif CredHandle *sspicred; /* SSPI credentials handle */ CtxtHandle *sspictx; /* SSPI context */ char *sspitarget; /* SSPI target name */ @@ -512,8 +508,6 @@ struct pg_conn = /* Buffer for receiving various parts of messages */ PQExpBufferData workBuffer; /* expansible string */ - - char *sslpassword; /* client key file password */ }; = /* PGcancel stores all data necessary to cancel a connection. A copy of t= his ------- =_aaaaaaaaaa0--