Re: dblink: Add SCRAM pass-through authentication

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