On Thu, Mar 20, 2025 at 12:54 PM Matheus Alcantara
<matheusssil...@gmail.com> wrote:
> Since the security checks are defined I'm attaching 0003 which include
> the fix of security checks for postgres_fdw. It implements the
> validations very similar to what are being implemented on dblink.
Comments on 0003:
> + keywords[n] = "require_auth";
> + values[n] = "scram-sha-256";
> + n++;
The keywords and values arrays need to be lengthened for this.
> host all all $hostaddr/32
> scram-sha-256
> - });
> + }
> + );
Accidental diff?
A few whitespace and comment tweaks are attached as well.
--
> > I think they should just be reduced to "The remote server must request
> > SCRAM authentication." and "The user mapping password is not used."
>
> I've removed the "user mapping password" <listitem> because we already
> mentioned above that the password is not used and having just "The user
> mapping password is not used." again seems redundant, what do you think?
Personally, I think it's still useful to call out that the password in
the user mapping is explicitly ignored. The other text motivates the
feature, but it doesn't explain how it interacts with existing user
mappings (most of which will have passwords).
> > Now that we handle the pfree() in PG_CATCH instead, that lower-level
> > pfree should be removed, and then connect_pg_server() doesn't need to
> > take an rconn pointer at all. For extra credit you could maybe move
> > the allocation of rconn down below the call to connect_pg_server(),
> > and get rid of the try/catch?
>
> Good catch, thanks, it's much better now! With this change we can also
> remove the second if (connname) condition. All fixed on attached.
I like that; the patch is a lot easier to follow now.
Thanks,
--Jacob
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index b8b30284086..b33895b4f56 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -184,7 +184,6 @@ 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);
/*
@@ -458,7 +457,7 @@ pgfdw_security_check(const char **keywords, const char
**values, UserMapping *us
}
/*
- * Ok if SCRAM pass-through is being used and all required scram options
+ * 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.
@@ -567,7 +566,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
values[n] = GetDatabaseEncodingName();
n++;
- /* Add required SCRAM pass-through connection options if it's
enable. */
+ /* Add required SCRAM pass-through connection options if it's
enabled. */
if (MyProcPort->has_scram_keys && UseScramPassthrough(server,
user))
{
int len;
@@ -2509,14 +2508,14 @@ pgfdw_conn_checkable(void)
#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.
- */
+/*
+ * 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)
{
@@ -2560,6 +2559,5 @@ pgfdw_has_required_scram_options(const char **keywords,
const char **values)
has_scram_keys = has_scram_client_key && has_scram_server_key &&
MyProcPort->has_scram_keys;
-
return (has_scram_keys && has_require_auth);
}
