On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <peter@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
