On 06.03.25 22:58, Jacob Champion wrote:
On Thu, Mar 6, 2025 at 12:33 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
AFAICT, in pgfdw_security_check(), if SCRAM has been used for the
outgoing server connection, then PQconnectionUsedPassword() is true, and
then this check should fail if no "password" parameter was given. That
check should be expanded to allow alternatively passing the SCRAM key
component parameters.
pgfdw_security_check() is currently not called if SCRAM passthrough is
in use, though:
/*
* 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);
Right. How about the attached? It checks as an alternative to a
password whether the SCRAM keys were provided. That should get us back
to the same level of checking?
From daa5ff65007ed1cef49020191b50abf226228d95 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri, 7 Mar 2025 17:20:33 +0100
Subject: [PATCH] WIP: postgres_fdw: Fix SCRAM pass-through security
---
contrib/postgres_fdw/connection.c | 41 ++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 8ef9702c05c..5a069b54078 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -447,12 +447,23 @@ pgfdw_security_check(const char **keywords, const char
**values, UserMapping *us
/* Connected via PW, with PW required true, and provided non-empty PW.
*/
if (PQconnectionUsedPassword(conn))
{
- /* ok if params contain a non-empty password */
+ bool has_scram_server_key = false;
+ bool has_scram_client_key = false;
+
+ /* ok if params contain a non-empty password or SCRAM keys */
for (int i = 0; keywords[i] != NULL; i++)
{
if (strcmp(keywords[i], "password") == 0 &&
values[i][0] != '\0')
return;
+
+ if (strcmp(keywords[i], "scram_client_key") == 0 &&
values[i][0] != '\0')
+ has_scram_client_key = true;
+ if (strcmp(keywords[i], "scram_server_key") == 0 &&
values[i][0] != '\0')
+ has_scram_server_key = true;
}
+
+ if (has_scram_client_key && has_scram_server_key &&
MyProcPort->has_scram_keys)
+ return;
}
ereport(ERROR,
@@ -586,12 +597,8 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
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 +616,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);
@@ -703,6 +706,8 @@ static void
check_conn_params(const char **keywords, const char **values, UserMapping
*user)
{
int i;
+ bool has_scram_server_key = false;
+ bool has_scram_client_key = false;
/* no check required if superuser */
if (superuser_arg(user->userid))
@@ -714,13 +719,21 @@ check_conn_params(const char **keywords, const char
**values, UserMapping *user)
return;
#endif
- /* ok if params contain a non-empty password */
+ /* ok if params contain a non-empty password or SCRAM keys */
for (i = 0; keywords[i] != NULL; i++)
{
if (strcmp(keywords[i], "password") == 0 && values[i][0] !=
'\0')
return;
+
+ if (strcmp(keywords[i], "scram_client_key") == 0 &&
values[i][0] != '\0')
+ has_scram_client_key = true;
+ if (strcmp(keywords[i], "scram_server_key") == 0 &&
values[i][0] != '\0')
+ has_scram_server_key = true;
}
+ if (has_scram_client_key && has_scram_server_key &&
MyProcPort->has_scram_keys)
+ return;
+
/* ok if the superuser explicitly said so at user mapping creation time
*/
if (!UserMappingPasswordRequired(user))
return;
--
2.48.1
