On Mon, Mar 3, 2025 at 9:01 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I keep getting pulled away from my review of 0002
Here's a review of v3-0002:
> +dblink_connstr_check(const char *connstr, bool useScramPassthrough)
> {
> + if (useScramPassthrough)
> + {
> + if (dblink_connstr_has_scram_require_auth(connstr))
> + return;
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.
Alternatively, this check could also verify that
scram_client_key/server_key are set in the connection string
explicitly.
It is still strange to me that we don't fall through to check other
potential safe options (see comment on the dblink_security_check,
below).
> + ...
> + }
> +
> if (superuser())
> return;
>
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.
> +dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr, bool useScramPassthrough)
> {
> +
> + if (useScramPassthrough)
> + {
> + if (dblink_connstr_has_scram_require_auth(connstr))
> + return;
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?
This division between "connstr check" and "security check" is easier
to describe when we allow a variety of safe options, and check to see
if any of them have been used. use_scram_passthrough locks it down to
one possible option, making this division a little muddier.
> + appendStringInfo(buf, "scram_client_key='%s'", client_key);
> + appendStringInfo(buf, "scram_server_key='%s'", server_key);
> + appendStringInfo(buf, "require_auth='scram-sha-256'");
These should have spaces between them; i.e. "scram_client_key='%s' ".
Thanks,
--Jacob
