Re: dblink: Add SCRAM pass-through authentication

On Wed, Feb 19, 2025 at 12:02 PM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> wrote:
> > Hardcoding to scram-sha-256 would also prohibit the use of GSS or
> > standard password auth, now that I think about it. The docs currently
> > have a note about being able to choose... Should we add the other
> > permitted authentication types, i.e. `password,md5,scram-sha-256,gss`?
> > Or should we prohibit the use of other auth types if you've set
> > use_scram_passthrough? Or maybe there's an easier way to enforce the
> > use of the SCRAM keys, that better matches the current logic in
> > dblink_security_check?
> >
> But would it be possible to use SCRAM pass-through feature using another auth
> method?

No, but if you want the same foreign server to be accessible by users
who log in with different authentication types, forcing a single
require_auth setting will defeat that. I don't have a strong opinion
about how important maintaining that functionality is, but the code
seems to allow it today.

--

Some thoughts on v2-0001:

I like the conceptual simplification of get_connect_string().

> + /* first time, allocate or get the custom wait event */
> + if (dblink_we_connect == 0)
> + dblink_we_connect = WaitEventExtensionNew("DblinkConnectPgServer");

Replacing two existing wait events with one new one is a user-facing
change (see the documented list of events at [1]). Maybe we want that,
but it hasn't been explained. I think that change should be made
independently of a refactoring patch (or else defended in the commit
message).

> + if (foreign_server)
> + {
> + serverid = foreign_server->serverid;
> + user_mapping = GetUserMapping(userid, serverid);
> +
> + connstr = get_connect_string(foreign_server, user_mapping);
> + }

Is there any other valid value for user_mapping that a caller can
choose? If not, I'd like to see the GetUserMapping call pushed back
down into get_connect_string(), unless there's another reason for
pulling it up that I'm missing.

> + static const PQconninfoOption *options = NULL;
> +
> + /*
> + * Get list of valid libpq options.
> + *
> + * To avoid unnecessary work, we get the list once and use it throughout
> + * the lifetime of this backend process. We don't need to care about
> + * memory context issues, because PQconndefaults allocates with malloc.
> + */
> + if (!options)
> + {
> + options = PQconndefaults();
> + if (!options) /* assume reason for failure is OOM */
> + ereport(ERROR,
> + (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
> + errmsg("out of memory"),
> + errdetail("Could not get libpq's default connection options.")));
> + }

It looks like `options` might be an unused variable in connect_pg_server()?

> - if (PQstatus(conn) == CONNECTION_BAD)
> + if (!conn || PQstatus(conn) != CONNECTION_OK)

I don't think this should be changed in a refactoring patch.
PQstatus() can handle a NULL conn pointer.

> - if (rconn)
> - pfree(rconn);

Looks like this code in dblink_connect() was dropped.

Thanks!
--Jacob

[1] https://www.postgresql.org/docs/current/dblink.html