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

Hi, thanks for all the comments! Attached v5 with some fixes

(I'll answer comments for different emails on this since most of them is
fixed on this new v5 version)

> I think this fix may break the other usage in dblink_get_conn(), where
> rconn comes from a hash entry. Maybe dblink_connect() should instead
> put a PG_CATCH/pfree/PG_RE_THROW around the call to
> connect_pg_server(), to ensure that the rconn allocation in
> TopMemoryContext doesn't get leaked?
>
Fixed by wrapping on PG_CATCH/pfree/PG_RE_THROW. I didn't managed to
create a test that use this code path, so let me know if I'm still
missing something.

> Can a comment be added somewhere to state that the security of this
> check relies on scram_server_key and scram_client_key not coming from
> the environment or any mapping options? The fact that those two
> options are declared 1) without envvar names and 2) as debug options
> is doing a lot of heavy security lifting, but it's hard to see that
> from this part of the code.
>
I've added a code comment on dblink_connstr_has_required_scram_options
function which is called on "connstr check" and "security check". Please
let me know what you think.

> Alternatively, this check could also verify that
> scram_client_key/server_key are set in the connection string
> explicitly.
>
I've added this validation on dblink_connstr_has_required_scram_options.

> For the additions to dblink_connstr_check/security_check, I think the
> superuser checks should remain at the top. Superusers can still do
> what they want.
>
Fixed

> I don't think this check should be the same as the connstr check
> above, but I don't have a concrete example of what it should do
> instead. require_auth is taking care of the accidental trust config...
> Should there be an API that checks that the server and client key were
> used during the SCRAM exchange, similar to PQconnectionUsedPassword()?
> Would that even add anything?
>
I was also thinking about this, maybe we could add a new validation
(similar with PQconnectionUsedPassword, on fe-connect.c) that check if
the scram keys is set on PGconn (we only set these keys if we are
actually using the scram pass-through feature)

+int
+PQconnectionUsedScramKeys(const PGconn *conn)
+{
+       if (conn->scram_client_key && conn->scram_server_key)
+               return true;
+
+       return false;
+}

And then call on dblink_security_check
-       if (MyProcPort->has_scram_keys &&
dblink_connstr_has_required_scram_options(connstr))
+       if (MyProcPort->has_scram_keys
+               && dblink_connstr_has_required_scram_options(connstr)
+               && PQconnectionUsedScramKeys(conn))
                return;

(Note that I didn't implement this on this new patch version, I'm just
sharing some ideas that I had during development.)

> These should have spaces between them; i.e. "scram_client_key='%s' ".
>
Fixed

> > On Fri, Mar 7, 2025 at 8:22 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > > Right.  How about the attached?  It checks as an alternative to a
> > > password whether the SCRAM keys were provided.  That should get us back
> > > to the same level of checking?
> >
> > Yes, I think so. Attached is a set of tests to illustrate, mirroring
> > the dblink tests added upthread; they fail without this patch.
>
> In an offline discussion with Peter and Matheus, we figured out that
> this is still not enough. The latest patch checks that a password was
> used, but it doesn't ensure that the password material came from the
> SCRAM keys. Attached is an updated test to illustrate.
>
On this new patch version I also changed the "connstr check" and
"security check" to have a validation very similar to what Peter
implemented on 0001-WIP-postgres_fdw-Fix-SCRAM-pass-through-security
patch. I also reproduced this test case that you've created on this new
dblink patch version and we actually fail as expected (but with a
different error message) because here we are adding
require_auth=scram-sha-256.

So, I think that having something similar to what Peter implemented on
his patch and adding require_auth=scram-sha-256 may prevent this kind of
security issue?

--
Matheus Alcantara