On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
> Yeah, I think option (2) is enough for now. If someone wants to enable
> the kinds of things you describe, they can always come back and
> implement option (1) later.
Sounds good to me.
--
Notes on v8:
- The following documentation pieces need to be adjusted, now that
we've decided that `use_scram_passthrough` will enforce
`require_auth=scram-sha-256`:
> + The remote server must request SCRAM authentication. (If desired,
> + enforce this on the client side (FDW side) with the option
> + <literal>require_auth</literal>.) If another authentication method is
> + requested by the server, then that one will be used normally.
and
> + 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.)
I think they should just be reduced to "The remote server must request
SCRAM authentication." and "The user mapping password is not used."
- In get_connect_string():
> + /* first gather the server connstr options */
> + Oid serverid = foreign_server->serverid;
I think this comment belongs elsewhere (connect_pg_server) and should
be deleted from this block.
- Sorry for not realizing this before now, but I couldn't figure out
why connect_pg_server() took the rconn pointer, and it turns out it
just passes it along to dblink_security_check(), which pfree's it
before throwing an error. So that will double-free with the current
refactoring patch (and I'm not sure why assertions aren't catching
that?).
I thought for sure this inconsistency would be a problem on HEAD, but
it turns out that rconn is set to NULL in the code path where it would
be a bug... How confusing.
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?
> + /* Verify the set of connection parameters. */
> dblink_connstr_check(connstr);
> ...
> + /* Perform post-connection security checks. */
> dblink_security_check(conn, rconn, connstr);
- These comment additions probably belong in 0001 rather than 0002.
- As discussed offlist, 0002 needs pgperltidy'd rather than perltidy'd.
- I have attached some additional nitpicky comment edits and
whitespace changes as a diff; pick and choose as desired.
Thanks!
--Jacob
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 50959c4f5bb..d333f1c5429 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2567,14 +2567,14 @@ deleteConnection(const char *name)
}
/*
- * Ensure that require_auth and scram keys are correctly set on connstr.
- * SCRAM keys used to pass-through is coming from the initial connection from
- * the client with the server.
+ * Ensure that require_auth and SCRAM keys are correctly set on connstr.
+ * SCRAM keys used to pass-through are coming from the initial connection
+ * from the client with the server.
*
- * All required scram options is set by ourself, so we just need to ensure
+ * All required SCRAM options are set by dblink, so we just need to ensure
* that these options are not overwritten by the user.
*
- * See appendSCRAMKeysInfo and it's usage for more.
+ * See appendSCRAMKeysInfo and its usage for more.
*/
bool
dblink_connstr_has_required_scram_options(const char *connstr)
@@ -2645,13 +2645,13 @@ dblink_security_check(PGconn *conn, remoteConn *rconn,
const char *connstr)
return;
/*
- * Password was not used to connect, check if scram pass-through is in
+ * Password was not used to connect, check if SCRAM pass-through is in
* use.
*
* If dblink_connstr_has_required_scram_options is true we assume that
- * UseScramPassthrough is also true because the required scram keys is
+ * UseScramPassthrough is also true because the required SCRAM keys are
* only added if UseScramPassthrough is set, and the user is not allowed
- * to add the scram keys on fdw and user mapping options.
+ * to add the SCRAM keys on fdw and user mapping options.
*/
if (MyProcPort->has_scram_keys &&
dblink_connstr_has_required_scram_options(connstr))
return;
@@ -2710,12 +2710,12 @@ dblink_connstr_has_pw(const char *connstr)
/*
* For non-superusers, insist that the connstr specify a password, except if
* GSSAPI credentials have been delegated (and we check that they are used for
- * the connection in dblink_security_check later) or if scram pass-through is
+ * the connection in dblink_security_check later) or if SCRAM pass-through is
* being used. This prevents a password or GSSAPI credentials from being
* picked up from .pgpass, a service file, the environment, etc. We don't want
* the postgres user's passwords or Kerberos credentials to be accessible to
- * non-superusers. In case of scram pass-through insist that the connstr
- * has the required scram pass-through options.
+ * non-superusers. In case of SCRAM pass-through insist that the connstr
+ * has the required SCRAM pass-through options.
*/
static void
dblink_connstr_check(const char *connstr)
@@ -2729,7 +2729,6 @@ dblink_connstr_check(const char *connstr)
if (MyProcPort->has_scram_keys &&
dblink_connstr_has_required_scram_options(connstr))
return;
-
#ifdef ENABLE_GSS
if (be_gssapi_get_delegation(MyProcPort))
return;
@@ -2876,8 +2875,8 @@ get_connect_string(ForeignServer *foreign_server)
/*
* First append hardcoded options needed for SCRAM pass-through, so if
the
- * user overwrite these options we can ereport on dblink_connstr_check
and
- * dblink_security_check.
+ * user overwrites these options we can ereport on dblink_connstr_check
+ * and dblink_security_check.
*/
if (MyProcPort->has_scram_keys && UseScramPassthrough(foreign_server,
user_mapping))
appendSCRAMKeysInfo(&buf);
@@ -3063,7 +3062,6 @@ is_valid_dblink_option(const PQconninfoOption *options,
const char *option,
return true;
}
-
/*
* Same as is_valid_dblink_option but also check for only dblink_fdw specific
* options.
@@ -3078,7 +3076,6 @@ is_valid_dblink_fdw_option(const PQconninfoOption
*options, const char *option,
return is_valid_dblink_option(options, option, context);
}
-
/*
* Copy the remote session's values of GUCs that affect datatype I/O
* and apply them locally in a new GUC nesting level. Returns the new
@@ -3161,7 +3158,6 @@ appendSCRAMKeysInfo(StringInfo buf)
char *client_key;
char *server_key;
-
len = pg_b64_enc_len(sizeof(MyProcPort->scram_ClientKey));
/* don't forget the zero-terminator */
client_key = palloc0(len + 1);
diff --git a/doc/src/sgml/dblink.sgml b/doc/src/sgml/dblink.sgml
index e3b4129ae26..9cbcf645e78 100644
--- a/doc/src/sgml/dblink.sgml
+++ b/doc/src/sgml/dblink.sgml
@@ -136,7 +136,7 @@ dblink_connect(text connname, text connstr) returns text
</para>
</refsect1>
-<refsect1>
+ <refsect1>
<title>Foreign Data Wrapper</title>
<para>
@@ -208,8 +208,6 @@ dblink_connect(text connname, text connstr) returns text
</listitem>
</itemizedlist>
</para>
-
-
</refsect1>
<refsect1>
@@ -257,7 +255,7 @@ SELECT dblink_connect('myconn', 'dbname=postgres
options=-csearch_path=');
(1 row)
-- FOREIGN DATA WRAPPER functionality
--- Note: local connection that don't use SCRAM pass-through require password
+-- Note: local connections that don't use SCRAM pass-through require password
-- authentication for this to work properly. Otherwise, you will receive
-- the following error from dblink_connect():
-- ERROR: password is required
