Skip to content

Commit 3d03b24

Browse files
sfrostsfrost
committed
Revert "Add support for Kerberos credential delegation"
This reverts commit 3d4fa22. Per discussion and buildfarm, this depends on APIs that seem to not be available on at least one platform (NetBSD). Should be certainly possible to rework to be optional on that platform if necessary but bit late for that at this point. Discussion: https://postgr.es/m/3286097.1680922218@sss.pgh.pa.us
1 parent db4f21e commit 3d03b24

36 files changed

+136
-755
lines changed

‎contrib/dblink/dblink.c

Lines changed: 45 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@
4848
#include "funcapi.h"
4949
#include "lib/stringinfo.h"
5050
#include "libpq-fe.h"
51-
#include "libpq/libpq-be.h"
5251
#include "libpq/libpq-be-fe-helpers.h"
5352
#include "mb/pg_wchar.h"
5453
#include "miscadmin.h"
@@ -112,8 +111,7 @@ static HeapTuple get_tuple_of_interest(Relation rel, int *pkattnums, int pknumat
112111
static Relation get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode);
113112
static char *generate_relation_name(Relation rel);
114113
static void dblink_connstr_check(const char *connstr);
115-
static bool dblink_connstr_has_pw(const char *connstr);
116-
static void dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr);
114+
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
117115
static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
118116
bool fail, const char *fmt,...) pg_attribute_printf(5, 6);
119117
static char *get_connect_string(const char *servername);
@@ -215,7 +213,7 @@ dblink_get_conn(char *conname_or_str,
215213
errmsg("could not establish connection"),
216214
errdetail_internal("%s", msg)));
217215
}
218-
dblink_security_check(conn, rconn, connstr);
216+
dblink_security_check(conn, rconn);
219217
if (PQclientEncoding(conn) != GetDatabaseEncoding())
220218
PQsetClientEncoding(conn, GetDatabaseEncodingName());
221219
freeconn = true;
@@ -309,7 +307,7 @@ dblink_connect(PG_FUNCTION_ARGS)
309307
}
310308

311309
/* check password actually used if not superuser */
312-
dblink_security_check(conn, rconn, connstr);
310+
dblink_security_check(conn, rconn);
313311

314312
/* attempt to set client encoding to match server encoding, if needed */
315313
if (PQclientEncoding(conn) != GetDatabaseEncoding())
@@ -2586,99 +2584,64 @@ deleteConnection(const char *name)
25862584
errmsg("undefined connection name")));
25872585
}
25882586

2589-
/*
2590-
* We need to make sure that the connection made used credentials
2591-
* which were provided by the user, so check what credentials were
2592-
* used to connect and then make sure that they came from the user.
2593-
*/
25942587
static void
2595-
dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
2588+
dblink_security_check(PGconn *conn, remoteConn *rconn)
25962589
{
2597-
/* Superuser bypasses security check */
2598-
if (superuser())
2599-
return;
2600-
2601-
/* If password was used to connect, make sure it was one provided */
2602-
if (PQconnectionUsedPassword(conn) && dblink_connstr_has_pw(connstr))
2603-
return;
2604-
2605-
#ifdef ENABLE_GSS
2606-
/* If GSSAPI creds used to connect, make sure it was one delegated */
2607-
if (PQconnectionUsedGSSAPI(conn) && be_gssapi_get_deleg(MyProcPort))
2608-
return;
2609-
#endif
2610-
2611-
/* Otherwise, fail out */
2612-
libpqsrv_disconnect(conn);
2613-
if (rconn)
2614-
pfree(rconn);
2590+
if (!superuser())
2591+
{
2592+
if (!PQconnectionUsedPassword(conn))
2593+
{
2594+
libpqsrv_disconnect(conn);
2595+
if (rconn)
2596+
pfree(rconn);
26152597

2616-
ereport(ERROR,
2617-
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
2618-
errmsg("password or GSSAPI delegated credentials required"),
2619-
errdetail("Non-superusers may only connect using credentials they provide, eg: password in connection string or delegated GSSAPI credentials"),
2620-
errhint("Ensure provided credentials match target server's authentication method.")));
2598+
ereport(ERROR,
2599+
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
2600+
errmsg("password is required"),
2601+
errdetail("Non-superuser cannot connect if the server does not request a password."),
2602+
errhint("Target server's authentication method must be changed.")));
2603+
}
2604+
}
26212605
}
26222606

26232607
/*
2624-
* Function to check if the connection string includes an explicit
2625-
* password, needed to ensure that non-superuser password-based auth
2626-
* is using a provided password and not one picked up from the
2627-
* environment.
2608+
* For non-superusers, insist that the connstr specify a password. This
2609+
* prevents a password from being picked up from .pgpass, a service file,
2610+
* the environment, etc. We don't want the postgres user's passwords
2611+
* to be accessible to non-superusers.
26282612
*/
2629-
static bool
2630-
dblink_connstr_has_pw(const char *connstr)
2613+
static void
2614+
dblink_connstr_check(const char *connstr)
26312615
{
2632-
PQconninfoOption *options;
2633-
PQconninfoOption *option;
2634-
bool connstr_gives_password = false;
2635-
2636-
options = PQconninfoParse(connstr, NULL);
2637-
if (options)
2616+
if (!superuser())
26382617
{
2639-
for (option = options; option->keyword != NULL; option++)
2618+
PQconninfoOption *options;
2619+
PQconninfoOption *option;
2620+
bool connstr_gives_password = false;
2621+
2622+
options = PQconninfoParse(connstr, NULL);
2623+
if (options)
26402624
{
2641-
if (strcmp(option->keyword, "password") == 0)
2625+
for (option = options; option->keyword != NULL; option++)
26422626
{
2643-
if (option->val != NULL && option->val[0] != '\0')
2627+
if (strcmp(option->keyword, "password") == 0)
26442628
{
2645-
connstr_gives_password = true;
2646-
break;
2629+
if (option->val != NULL && option->val[0] != '\0')
2630+
{
2631+
connstr_gives_password = true;
2632+
break;
2633+
}
26472634
}
26482635
}
2636+
PQconninfoFree(options);
26492637
}
2650-
PQconninfoFree(options);
2651-
}
2652-
2653-
return connstr_gives_password;
2654-
}
26552638

2656-
/*
2657-
* For non-superusers, insist that the connstr specify a password, except
2658-
* if GSSAPI credentials have been delegated (and we check that they are used
2659-
* for the connection in dblink_security_check later). This prevents a
2660-
* password or GSSAPI credentials from being picked up from .pgpass, a
2661-
* service file, the environment, etc. We don't want the postgres user's
2662-
* passwords or Kerberos credentials to be accessible to non-superusers.
2663-
*/
2664-
static void
2665-
dblink_connstr_check(const char *connstr)
2666-
{
2667-
if (superuser())
2668-
return;
2669-
2670-
if (dblink_connstr_has_pw(connstr))
2671-
return;
2672-
2673-
#ifdef ENABLE_GSS
2674-
if (be_gssapi_get_deleg(MyProcPort))
2675-
return;
2676-
#endif
2677-
2678-
ereport(ERROR,
2679-
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
2680-
errmsg("password or GSSAPI delegated credentials required"),
2681-
errdetail("Non-superusers must provide a password in the connection string or send delegated GSSAPI credentials.")));
2639+
if (!connstr_gives_password)
2640+
ereport(ERROR,
2641+
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
2642+
errmsg("password is required"),
2643+
errdetail("Non-superusers must provide a password in the connection string.")));
2644+
}
26822645
}
26832646

26842647
/*

‎contrib/dblink/expected/dblink.out

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -903,8 +903,8 @@ GRANT EXECUTE ON FUNCTION dblink_connect_u(text, text) TO regress_dblink_user;
903903
SET SESSION AUTHORIZATION regress_dblink_user;
904904
-- should fail
905905
SELECT dblink_connect('myconn', 'fdtest');
906-
ERROR: password or GSSAPI delegated credentials required
907-
DETAIL: Non-superusers must provide a password in the connection string or send delegated GSSAPI credentials.
906+
ERROR: password is required
907+
DETAIL: Non-superusers must provide a password in the connection string.
908908
-- should succeed
909909
SELECT dblink_connect_u('myconn', 'fdtest');
910910
dblink_connect_u

‎contrib/postgres_fdw/connection.c

Lines changed: 16 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
#include "catalog/pg_user_mapping.h"
1818
#include "commands/defrem.h"
1919
#include "funcapi.h"
20-
#include "libpq/libpq-be.h"
2120
#include "libpq/libpq-be-fe-helpers.h"
2221
#include "mb/pg_wchar.h"
2322
#include "miscadmin.h"
@@ -150,8 +149,6 @@ static void pgfdw_finish_pre_subcommit_cleanup(List *pending_entries,
150149
static void pgfdw_finish_abort_cleanup(List *pending_entries,
151150
List *cancel_requested,
152151
bool toplevel);
153-
static void pgfdw_security_check(const char **keywords, const char **values,
154-
UserMapping *user, PGconn *conn);
155152
static bool UserMappingPasswordRequired(UserMapping *user);
156153
static bool disconnect_cached_connections(Oid serverid);
157154

@@ -387,47 +384,6 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
387384
entry->conn, server->servername, user->umid, user->userid);
388385
}
389386

390-
/*
391-
* Check that non-superuser has used password or delegated credentials
392-
* to establish connection; otherwise, he's piggybacking on the
393-
* postgres server's user identity. See also dblink_security_check()
394-
* in contrib/dblink and check_conn_params.
395-
*/
396-
static void
397-
pgfdw_security_check(const char **keywords, const char **values, UserMapping *user, PGconn *conn)
398-
{
399-
/* Superusers bypass the check */
400-
if (superuser_arg(user->userid))
401-
return;
402-
403-
#ifdef ENABLE_GSS
404-
/* Connected via GSSAPI with delegated credentials- all good. */
405-
if (PQconnectionUsedGSSAPI(conn) && be_gssapi_get_deleg(MyProcPort))
406-
return;
407-
#endif
408-
409-
/* Ok if superuser set PW required false. */
410-
if (!UserMappingPasswordRequired(user))
411-
return;
412-
413-
/* Connected via PW, with PW required true, and provided non-empty PW. */
414-
if (PQconnectionUsedPassword(conn))
415-
{
416-
/* ok if params contain a non-empty password */
417-
for (int i = 0; keywords[i] != NULL; i++)
418-
{
419-
if (strcmp(keywords[i], "password") == 0 && values[i][0] != '\0')
420-
return;
421-
}
422-
}
423-
424-
ereport(ERROR,
425-
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
426-
errmsg("password or GSSAPI delegated credentials required"),
427-
errdetail("Non-superuser cannot connect if the server does not request a password or use GSSAPI with delegated credentials."),
428-
errhint("Target server's authentication method must be changed or password_required=false set in the user mapping attributes.")));
429-
}
430-
431387
/*
432388
* Connect to remote server using specified server and user mapping properties.
433389
*/
@@ -539,8 +495,19 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
539495
server->servername),
540496
errdetail_internal("%s", pchomp(PQerrorMessage(conn)))));
541497

542-
/* Perform post-connection security checks */
543-
pgfdw_security_check(keywords, values, user, conn);
498+
/*
499+
* Check that non-superuser has used password to establish connection;
500+
* otherwise, he's piggybacking on the postgres server's user
501+
* identity. See also dblink_security_check() in contrib/dblink and
502+
* check_conn_params.
503+
*/
504+
if (!superuser_arg(user->userid) && UserMappingPasswordRequired(user) &&
505+
!PQconnectionUsedPassword(conn))
506+
ereport(ERROR,
507+
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
508+
errmsg("password is required"),
509+
errdetail("Non-superuser cannot connect if the server does not request a password."),
510+
errhint("Target server's authentication method must be changed or password_required=false set in the user mapping attributes.")));
544511

545512
/* Prepare new session for use */
546513
configure_remote_session(conn);
@@ -594,8 +561,7 @@ UserMappingPasswordRequired(UserMapping *user)
594561
}
595562

596563
/*
597-
* For non-superusers, insist that the connstr specify a password or that the
598-
* user provided their own GSSAPI delegated credentials. This
564+
* For non-superusers, insist that the connstr specify a password. This
599565
* prevents a password from being picked up from .pgpass, a service file, the
600566
* environment, etc. We don't want the postgres user's passwords,
601567
* certificates, etc to be accessible to non-superusers. (See also
@@ -610,12 +576,6 @@ check_conn_params(const char **keywords, const char **values, UserMapping *user)
610576
if (superuser_arg(user->userid))
611577
return;
612578

613-
#ifdef ENABLE_GSS
614-
/* ok if the user provided their own delegated credentials */
615-
if (be_gssapi_get_deleg(MyProcPort))
616-
return;
617-
#endif
618-
619579
/* ok if params contain a non-empty password */
620580
for (i = 0; keywords[i] != NULL; i++)
621581
{
@@ -629,8 +589,8 @@ check_conn_params(const char **keywords, const char **values, UserMapping *user)
629589

630590
ereport(ERROR,
631591
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
632-
errmsg("password or GSSAPI delegated credentials required"),
633-
errdetail("Non-superusers must delegate GSSAPI credentials or provide a password in the user mapping.")));
592+
errmsg("password is required"),
593+
errdetail("Non-superusers must provide a password in the user mapping.")));
634594
}
635595

636596
/*

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,7 @@ ALTER SERVER testserver1 OPTIONS (
171171
sslcrl 'value',
172172
--requirepeer 'value',
173173
krbsrvname 'value',
174-
gsslib 'value',
175-
gssdeleg 'value'
174+
gsslib 'value'
176175
--replication 'value'
177176
);
178177
-- Error, invalid list syntax
@@ -9841,8 +9840,8 @@ CREATE FOREIGN TABLE pg_temp.ft1_nopw (
98419840
c8 user_enum
98429841
) SERVER loopback_nopw OPTIONS (schema_name 'public', table_name 'ft1');
98439842
SELECT 1 FROM ft1_nopw LIMIT 1;
9844-
ERROR: password or GSSAPI delegated credentials required
9845-
DETAIL: Non-superusers must delegate GSSAPI credentials or provide a password in the user mapping.
9843+
ERROR: password is required
9844+
DETAIL: Non-superusers must provide a password in the user mapping.
98469845
-- If we add a password to the connstr it'll fail, because we don't allow passwords
98479846
-- in connstrs only in user mappings.
98489847
ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
@@ -9854,16 +9853,16 @@ HINT: Perhaps you meant the option "passfile".
98549853
-- This won't work with installcheck, but neither will most of the FDW checks.
98559854
ALTER USER MAPPING FOR CURRENT_USER SERVER loopback_nopw OPTIONS (ADD password 'dummypw');
98569855
SELECT 1 FROM ft1_nopw LIMIT 1;
9857-
ERROR: password or GSSAPI delegated credentials required
9858-
DETAIL: Non-superuser cannot connect if the server does not request a password or use GSSAPI with delegated credentials.
9856+
ERROR: password is required
9857+
DETAIL: Non-superuser cannot connect if the server does not request a password.
98599858
HINT: Target server's authentication method must be changed or password_required=false set in the user mapping attributes.
98609859
-- Unpriv user cannot make the mapping passwordless
98619860
ALTER USER MAPPING FOR CURRENT_USER SERVER loopback_nopw OPTIONS (ADD password_required 'false');
98629861
ERROR: password_required=false is superuser-only
98639862
HINT: User mappings with the password_required option set to false may only be created or modified by the superuser.
98649863
SELECT 1 FROM ft1_nopw LIMIT 1;
9865-
ERROR: password or GSSAPI delegated credentials required
9866-
DETAIL: Non-superuser cannot connect if the server does not request a password or use GSSAPI with delegated credentials.
9864+
ERROR: password is required
9865+
DETAIL: Non-superuser cannot connect if the server does not request a password.
98679866
HINT: Target server's authentication method must be changed or password_required=false set in the user mapping attributes.
98689867
RESET ROLE;
98699868
-- But the superuser can
@@ -9891,8 +9890,8 @@ DROP USER MAPPING FOR CURRENT_USER SERVER loopback_nopw;
98919890
-- This will fail again as it'll resolve the user mapping for public, which
98929891
-- lacks password_required=false
98939892
SELECT 1 FROM ft1_nopw LIMIT 1;
9894-
ERROR: password or GSSAPI delegated credentials required
9895-
DETAIL: Non-superusers must delegate GSSAPI credentials or provide a password in the user mapping.
9893+
ERROR: password is required
9894+
DETAIL: Non-superusers must provide a password in the user mapping.
98969895
RESET ROLE;
98979896
-- The user mapping for public is passwordless and lacks the password_required=false
98989897
-- mapping option, but will work because the current user is a superuser.

‎contrib/postgres_fdw/option.c

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -288,12 +288,6 @@ InitPgFdwOptions(void)
288288
{"sslcert", UserMappingRelationId, true},
289289
{"sslkey", UserMappingRelationId, true},
290290

291-
/*
292-
* gssdeleg is also a libpq option but should be allowed in a user
293-
* mapping context too
294-
*/
295-
{"gssdeleg", UserMappingRelationId, true},
296-
297291
{NULL, InvalidOid, false}
298292
};
299293

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,8 +185,7 @@ ALTER SERVER testserver1 OPTIONS (
185185
sslcrl 'value',
186186
--requirepeer 'value',
187187
krbsrvname 'value',
188-
gsslib 'value',
189-
gssdeleg 'value'
188+
gsslib 'value'
190189
--replication 'value'
191190
);
192191

0 commit comments

Comments
 (0)