Re: dblink: Add SCRAM pass-through authentication - Mailing list pgsql-hackers

On Wed, Feb 19, 2025 at 12:02 PM Matheus Alcantara
<matheusssilv97@gmail.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