From e4667b7e5520bbe7380ea684ebb815522a44fe82 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara Date: Wed, 19 Mar 2025 14:16:18 -0300 Subject: [PATCH v10 3/3] postgres_fdw: improve security checks On adding scram pass-through for dblink [1] thread, it was discussed that we should not by-pass fdw security check as it was implemented for postgres_fdw on 761c79508e7. This commit improve the security check by adding new scram pass-through checks to ensure that the required scram connection options are not overwritten by the user mapping or foreign server options. [1] https://www.postgresql.org/message-id/flat/CAFY6G8ercA1KES%3DE_0__R9QCTR805TTyYr1No8qF8ZxmMg8z2Q%40mail.gmail.com --- contrib/postgres_fdw/connection.c | 103 ++++++++++++++++++++--- contrib/postgres_fdw/t/001_auth_scram.pl | 41 +++++++++ doc/src/sgml/postgres-fdw.sgml | 11 +-- 3 files changed, 133 insertions(+), 22 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 8ef9702c05c..5a6e3e9825a 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -184,6 +184,7 @@ static void postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo, enum pgfdwVersion api_version); static int pgfdw_conn_check(PGconn *conn); static bool pgfdw_conn_checkable(void); +static bool pgfdw_has_required_scram_options(const char **keywords, const char **values); /* * Get a PGconn which can be used to execute queries on the remote PostgreSQL @@ -455,6 +456,15 @@ pgfdw_security_check(const char **keywords, const char **values, UserMapping *us } } + /* + * Ok if SCRAM pass-through is being used and all required SCRAM options + * are set correctly. If pgfdw_has_required_scram_options returns true we + * assume that UseScramPassthrough is also true since SCRAM options are + * only set when UseScramPassthrough is enabled. + */ + if (MyProcPort->has_scram_keys && pgfdw_has_required_scram_options(keywords, values)) + return; + ereport(ERROR, (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED), errmsg("password or GSSAPI delegated credentials required"), @@ -485,9 +495,10 @@ connect_pg_server(ForeignServer *server, UserMapping *user) * and UserMapping. (Some of them might not be libpq options, in * which case we'll just waste a few array slots.) Add 4 extra slots * for application_name, fallback_application_name, client_encoding, - * end marker. + * end marker, and 3 extra slots for scram keys and required scram + * pass-through options. */ - n = list_length(server->options) + list_length(user->options) + 4 + 2; + n = list_length(server->options) + list_length(user->options) + 4 + 3; keywords = (const char **) palloc(n * sizeof(char *)); values = (const char **) palloc(n * sizeof(char *)); @@ -556,6 +567,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user) values[n] = GetDatabaseEncodingName(); n++; + /* Add required SCRAM pass-through connection options if it's enabled. */ if (MyProcPort->has_scram_keys && UseScramPassthrough(server, user)) { int len; @@ -582,16 +594,20 @@ connect_pg_server(ForeignServer *server, UserMapping *user) if (encoded_len < 0) elog(ERROR, "could not encode SCRAM server key"); n++; + + /* + * Require scram-sha-256 to ensure that no other auth method is + * used when connecting with foreign server. + */ + keywords[n] = "require_auth"; + values[n] = "scram-sha-256"; + n++; } keywords[n] = values[n] = NULL; - /* - * Verify the set of connection parameters only if scram pass-through - * is not being used because the password is not necessary. - */ - if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) - check_conn_params(keywords, values, user); + /* Verify the set of connection parameters. */ + check_conn_params(keywords, values, user); /* first time, allocate or get the custom wait event */ if (pgfdw_we_connect == 0) @@ -609,12 +625,8 @@ connect_pg_server(ForeignServer *server, UserMapping *user) server->servername), errdetail_internal("%s", pchomp(PQerrorMessage(conn))))); - /* - * Perform post-connection security checks only if scram pass-through - * is not being used because the password is not necessary. - */ - if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) - pgfdw_security_check(keywords, values, user, conn); + /* Perform post-connection security checks. */ + pgfdw_security_check(keywords, values, user, conn); /* Prepare new session for use */ configure_remote_session(conn); @@ -725,6 +737,15 @@ check_conn_params(const char **keywords, const char **values, UserMapping *user) if (!UserMappingPasswordRequired(user)) return; + /* + * Ok if SCRAM pass-through is being used and all required scram options + * are set correctly. If pgfdw_has_required_scram_options returns true we + * assume that UseScramPassthrough is also true since SCRAM options are + * only set when UseScramPassthrough is enabled. + */ + if (MyProcPort->has_scram_keys && pgfdw_has_required_scram_options(keywords, values)) + return; + ereport(ERROR, (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED), errmsg("password or GSSAPI delegated credentials required"), @@ -2487,3 +2508,57 @@ pgfdw_conn_checkable(void) return false; #endif } + +/* + * Ensure that require_auth and SCRAM keys are correctly set on values. SCRAM + * keys used to pass-through are coming from the initial connection from the + * client with the server. + * + * All required SCRAM options are set by postgres_fdw, so we just need to + * ensure that these options are not overwritten by the user. + */ +static bool +pgfdw_has_required_scram_options(const char **keywords, const char **values) +{ + int i; + bool has_scram_server_key = false; + bool has_scram_client_key = false; + bool has_require_auth = false; + bool has_scram_keys = false; + + /* + * Continue iterating even if we found the keys that we need to validate + * to make sure that there is no other declaration of these keys that can + * overwrite the first. + */ + for (i = 0; keywords[i] != NULL; i++) + { + if (strcmp(keywords[i], "scram_client_key") == 0) + { + if (values[i] != NULL && values[i][0] != '\0') + has_scram_client_key = true; + else + has_scram_client_key = false; + } + + if (strcmp(keywords[i], "scram_server_key") == 0) + { + if (values[i] != NULL && values[i][0] != '\0') + has_scram_server_key = true; + else + has_scram_server_key = false; + } + + if (strcmp(keywords[i], "require_auth") == 0) + { + if (values[i] != NULL && strcmp(values[i], "scram-sha-256") == 0) + has_require_auth = true; + else + has_require_auth = false; + } + } + + has_scram_keys = has_scram_client_key && has_scram_server_key && MyProcPort->has_scram_keys; + + return (has_scram_keys && has_require_auth); +} diff --git a/contrib/postgres_fdw/t/001_auth_scram.pl b/contrib/postgres_fdw/t/001_auth_scram.pl index 047840cc914..2cce21b0fdb 100644 --- a/contrib/postgres_fdw/t/001_auth_scram.pl +++ b/contrib/postgres_fdw/t/001_auth_scram.pl @@ -68,6 +68,47 @@ test_fdw_auth($node1, $db0, "t2", $fdw_server2, test_auth($node2, $db2, "t2", "SCRAM auth directly on foreign server should still succeed"); +# Ensure that trust connections fail without superuser opt-in. +unlink($node1->data_dir . '/pg_hba.conf'); +unlink($node2->data_dir . '/pg_hba.conf'); + +$node1->append_conf( + 'pg_hba.conf', qq{ +local db0 all scram-sha-256 +local db1 all trust +} +); +$node2->append_conf( + 'pg_hba.conf', qq{ +local all all password +} +); + +$node1->restart; +$node2->restart; + +my ($ret, $stdout, $stderr) = $node1->psql( + $db0, + qq'select count(1) from t', + connstr => $node1->connstr($db0) . " user=$user"); + +is($ret, 3, 'loopback trust fails on the same cluster'); +like( + $stderr, + qr/failed: authentication method requirement "scram-sha-256"/, + 'expected error from loopback trust (same cluster)'); + +($ret, $stdout, $stderr) = $node1->psql( + $db0, + qq'select count(1) from t2', + connstr => $node1->connstr($db0) . " user=$user"); + +is($ret, 3, 'loopback password fails on a different cluster'); +like( + $stderr, + qr/failed: authentication method requirement "scram-sha-256"/, + 'expected error from loopback password (different cluster)'); + # Helper functions sub test_auth diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index a7f2f5ca182..65e36f1f3e4 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -788,10 +788,8 @@ OPTIONS (ADD password_required 'false'); - The remote server must request SCRAM authentication. (If desired, - enforce this on the client side (FDW side) with the option - require_auth.) If another authentication method - is requested by the server, then that one will be used normally. + The remote server must request the scram-sha-256 + authentication method; otherwise, the connection will fail. @@ -805,10 +803,7 @@ OPTIONS (ADD password_required 'false'); - The user mapping password is not used. (It could be set to support - other authentication methods, but that would arguably violate the - point of this feature, which is to avoid storing plain-text - passwords.) + The user mapping password is not used. -- 2.39.5 (Apple Git-154)