postgres_fdw: improve security checks

SCRAM pass-through should not bypass the FDW security check as it was
implemented for postgres_fdw in commit 761c79508e7.

This commit improves 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.  This is meant to match the security requirements for a
password-using connection.

Since libpq has no SCRAM-specific equivalent of
PQconnectionUsedPassword(), we enforce this instead by making the
use_scram_passthrough option of postgres_fdw imply
require_auth=scram-sha-256.  This means that if use_scram_passthrough
is set, some situations that might otherwise have worked are
preempted, for example GSSAPI with delegated credentials.  This could
be enhanced in the future if there is desire for more flexibility.

Reported-by: Jacob Champion <jacob.champion@enterprisedb.com>
Author: Matheus Alcantara <mths.dev@pm.me>
Co-authored-by: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://www.postgresql.org/message-id/flat/CAFY6G8ercA1KES%3DE_0__R9QCTR805TTyYr1No8qF8ZxmMg8z2Q%40mail.gmail.com

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,15 +594,19 @@ 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. */
-		 * 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);
 
 		/* first time, allocate or get the custom wait event */
@@ -609,11 +625,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 							server->servername),
 					 errdetail_internal("%s", pchomp(PQerrorMessage(conn)))));
 
-		/*
+		/* Perform post-connection security checks. */
-		 * 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);
 
 		/* Prepare new session for use */
@@ -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,56 @@ 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)
+{
+	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 (int 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);
+}

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

doc/src/sgml/postgres-fdw.sgml

@@ -788,10 +788,8 @@ OPTIONS (ADD password_required 'false');
         <itemizedlist>
          <listitem>
           <para>
-           The remote server must request SCRAM authentication.  (If desired,
+           The remote server must request the <literal>scram-sha-256</literal>
-           enforce this on the client side (FDW side) with the option
+           authentication method; otherwise, the connection will fail.
-           <literal>require_auth</literal>.)  If another authentication method
-           is requested by the server, then that one will be used normally.
           </para>
          </listitem>
 
@@ -805,10 +803,7 @@ OPTIONS (ADD password_required 'false');
 
          <listitem>
           <para>
-           The user mapping password is not used.  (It could be set to support
+           The user mapping password is not used.
-           other authentication methods, but that would arguably violate the
-           point of this feature, which is to avoid storing plain-text
-           passwords.)
           </para>
          </listitem>