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 <pe...@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
v5-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
v5-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
