dblink: Add SCRAM pass-through authentication

On Wed, Jan 22, 2025 at 6:10 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

The attached patch enables SCRAM authentication for dblink connections when
using dblink_fdw without requiring a plain-text password on user mapping
properties. The implementation is very similar to what was implemented on
postgres_fdw [0].

Not a full review, but I wanted to highlight one aspect of the patch:

-   dblink_connstr_check(connstr);
+   /*
+    * Verify the set of connection parameters only if scram pass-through is
+    * not being used because the password is not necessary.
+    */
+   if (!useScramPassthrough)
+       dblink_connstr_check(connstr);

and

-   dblink_security_check(conn, rconn, connstr);
+   /*
+    * Perform post-connection security checks only if scram pass-through is
+    * not being used because the password is not necessary.
+    */
+   if (!useScramPassthrough)
+       dblink_security_check(conn, rconn, connstr);

These don't seem right to me. SCRAM passthrough should be considered
as _part_ of the connstr/security checks, but I think it should not
_bypass_ those checks. We have to enforce the use of the SCRAM
credentials on the remote for safety, similarly to GSS delegation.
(This might be a good place for `require_auth=scram-sha-256`?)

I've attached a failing test to better illustrate what I mean.

It looks like the postgres_fdw patch that landed also performs a
bypass -- I think that may need an open item to fix? CC'd Peter.

Thanks!
--Jacob

Hi, thanks for reviewing this patch!

Em seg., 10 de fev. de 2025 às 20:19, Jacob Champion
<jacob.champion@enterprisedb.com> escreveu:

On Wed, Jan 22, 2025 at 6:10 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

The attached patch enables SCRAM authentication for dblink connections when
using dblink_fdw without requiring a plain-text password on user mapping
properties. The implementation is very similar to what was implemented on
postgres_fdw [0].

Not a full review, but I wanted to highlight one aspect of the patch:

-   dblink_connstr_check(connstr);
+   /*
+    * Verify the set of connection parameters only if scram pass-through is
+    * not being used because the password is not necessary.
+    */
+   if (!useScramPassthrough)
+       dblink_connstr_check(connstr);

and

-   dblink_security_check(conn, rconn, connstr);
+   /*
+    * Perform post-connection security checks only if scram pass-through is
+    * not being used because the password is not necessary.
+    */
+   if (!useScramPassthrough)
+       dblink_security_check(conn, rconn, connstr);

These don't seem right to me. SCRAM passthrough should be considered
as _part_ of the connstr/security checks, but I think it should not
_bypass_ those checks. We have to enforce the use of the SCRAM
credentials on the remote for safety, similarly to GSS delegation.
(This might be a good place for `require_auth=scram-sha-256`?)

Currently dblink_connstr_check and dblink_security_check only check if the
password is present on connection options, in case of not superuser. I added
this logic because the password is not required for SCRAM but I agree with you
that it sounds strange. Maybe these functions could check if the SCRAM is
being used and then skip the password validation if needed internally?

I also agree that we should enforce the use of the SCRAM on the remote for
safety. To do this I think that we could set require_auth=scram-sha-256 on
connection options if SCRAM pass-through is being used, with this we will get a
connection error. WYT?

I've attached a failing test to better illustrate what I mean.

Thanks for this! I'm attaching a v2 patch that includes the
require_auth=scram-sha-256 option and also your test case, which I've just
changed to the expected error message.

It looks like the postgres_fdw patch that landed also performs a
bypass -- I think that may need an open item to fix? CC'd Peter.

I can create a new patch to fix this on postgres_fdw too once we define the
approach to this here on dblink.

--
Matheus Alcantara

On Wed, Feb 12, 2025 at 11:54 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

Currently dblink_connstr_check and dblink_security_check only check if the
password is present on connection options, in case of not superuser.

They also check for GSS delegation. I think SCRAM passthrough should
ideally be considered a second form of credentials delegation, from
the perspective of those functions.

I added
this logic because the password is not required for SCRAM but I agree with you
that it sounds strange. Maybe these functions could check if the SCRAM is
being used and then skip the password validation if needed internally?

As long as the end result is to enforce that the credentials must come
from the end user, I think that would be fine in theory.

I also agree that we should enforce the use of the SCRAM on the remote for
safety. To do this I think that we could set require_auth=scram-sha-256 on
connection options if SCRAM pass-through is being used, with this we will get a
connection error. WYT?

We would need to verify that the user mapping can't overwrite that
with its own (less trusted) `require_auth` setting. (I think that
should be true already, but I'm not 100% sure.)

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?

I can create a new patch to fix this on postgres_fdw too once we define the
approach to this here on dblink.

Sounds good to me.

Thanks,
--Jacob

On Thu, Feb 13, 2025 at 8:25 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

I also agree that we should enforce the use of the SCRAM on the remote for
safety. To do this I think that we could set require_auth=scram-sha-256 on
connection options if SCRAM pass-through is being used, with this we will get a
connection error. WYT?

We would need to verify that the user mapping can't overwrite that
with its own (less trusted) `require_auth` setting. (I think that
should be true already, but I'm not 100% sure.)

Yeah, this is true. The user mapping and the fdw options can overwrite this.
I'll work on a fix for this.

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? We need the scram keys (that is saved on MyProcPort during
scram_exchange) to perform the pass-through which I think that we would not
have with another auth type? That being said I think that we could prohibit the
usage of other auth types when use_scram_passthrough is set, what do you think?

--
Matheus Alcantara

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]https://www.postgresql.org/docs/current/dblink.html). 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

Hi, thanks for all the reviews. Attached v3 with some fixes.

They also check for GSS delegation. I think SCRAM passthrough should
ideally be considered a second form of credentials delegation, from
the perspective of those functions.

Changed dblink_connstr_check and dblink_security_check to receive information
if scram passthrough is being used and then perform the validations. I just
don't know if the order that I put the checks on these functions is the better
or not, any input is welcome.

We would need to verify that the user mapping can't overwrite that
with its own (less trusted) `require_auth` setting. (I think that
should be true already, but I'm not 100% sure.)

When adding the require_auth on user mapping options we get an error `invalid
option "require_auth"`, but it was possible to add the require_auth on foreign
data wrapper server options. This new patch also add a validation on
dblink_connstr_check and dblink_security_check to ensure that require_auth is
is present on connection properties with scram-sha-256 as a value (we check only
the connstr so I think that perform the validation only on the first security
check function dblink_connstr_check would be enough, since I don't think that
the connstr will be changed after this check, but I added on both functions
just for sanity).

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.

The hard coded require_auth=scram-sha-256 will only be added if
use_scram_passthrough is set and also if the client that connect into the
server also uses the scram auth type, so that we can have the scram keys. So in
case the user try to authenticate using a different auth type on the same
foreign server that has use_scram_passthrough the require_auth=scram-sha-256
will not be added because MyProcPort->has_scram_keys is false.

--

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).

You're right, I've fixed it by changing the connect_pg_server to
receive the wait
event info.

+   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.

I agree, I've just declared outside of get_connect_string because on 0002 we
also need the user mapping for UseScramPassthrough function, so I think that it
would make the review more easier.

+ useScramPassthrough = MyProcPort->has_scram_keys &&
UseScramPassthrough(foreign_server, user_mapping);

+   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()?

It's right. Fixed.

-       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.

Fixed

- if (rconn)
- pfree(rconn);

Looks like this code in dblink_connect() was dropped.

Oops, fixed on connect_pg_server since this logic was moved to this function.

## Some other changes

I also added a new TAP test case to ensure that we return an error if
require_auth is overwritten with another value.

## Questions:

- The new dblink_connstr_has_scam_require_auth function is very similar with
dblink_connstr_has_pw, we may create a common function for these or let it
duplicated?

--
Matheus Alcantara

On Fri, Feb 21, 2025 at 6:48 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

Hi, thanks for all the reviews. Attached v3 with some fixes.

Thanks! I keep getting pulled away from my review of 0002, so I'll
just comment on 0001 to get things moving again; sorry for the delay.

I agree, I've just declared outside of get_connect_string because on 0002 we
also need the user mapping for UseScramPassthrough function, so I think that it
would make the review more easier.

Ah, I missed that part of 0002. Works for me.

- if (rconn)
- pfree(rconn);

Looks like this code in dblink_connect() was dropped.

Oops, fixed on connect_pg_server since this logic was moved to this function.

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?

## Questions:

- The new dblink_connstr_has_scam_require_auth function is very similar with
dblink_connstr_has_pw, we may create a common function for these or let it
duplicated?

My preference would be to wait for a third [1]https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming), but at the very least
I think the new function should go right next to the old one, to
highlight the similarity.

I have attached some stylistic suggestions, plus pgindent/pgperltidy
tweaks, as fixup commits 0002 and 0004.

Thanks,
--Jacob

[1]: https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

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

On 11.02.25 00:19, Jacob Champion wrote:

These don't seem right to me. SCRAM passthrough should be considered
as_part_ of the connstr/security checks, but I think it should not
_bypass_ those checks. We have to enforce the use of the SCRAM
credentials on the remote for safety, similarly to GSS delegation.
(This might be a good place for `require_auth=scram-sha-256`?)

I've attached a failing test to better illustrate what I mean.

It looks like the postgres_fdw patch that landed also performs a
bypass -- I think that may need an open item to fix? CC'd Peter.

AFAICT, in pgfdw_security_check(), if SCRAM has been used for the
outgoing server connection, then PQconnectionUsedPassword() is true, and
then this check should fail if no "password" parameter was given. That
check should be expanded to allow alternatively passing the SCRAM key
component parameters.

But that would mean the check is too restrictive, while you are
apparently claiming that the check is not restrictive enough?

(Also, this would appear to mean the current SCRAM pass-through code in
postgres_fdw should mostly not work, but the tests work, so I'm confused.)

On Thu, Mar 6, 2025 at 12:33 PM Peter Eisentraut <peter@eisentraut.org> wrote:

AFAICT, in pgfdw_security_check(), if SCRAM has been used for the
outgoing server connection, then PQconnectionUsedPassword() is true, and
then this check should fail if no "password" parameter was given. That
check should be expanded to allow alternatively passing the SCRAM key
component parameters.

pgfdw_security_check() is currently not called if SCRAM passthrough is
in use, though:

/*
* Perform post-connection security checks only if scram pass-through
* is not being used because the password is not necessary.
*/
if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user)))
pgfdw_security_check(keywords, values, user, conn);

--Jacob

On 06.03.25 22:58, Jacob Champion wrote:

On Thu, Mar 6, 2025 at 12:33 PM Peter Eisentraut <peter@eisentraut.org> wrote:

AFAICT, in pgfdw_security_check(), if SCRAM has been used for the
outgoing server connection, then PQconnectionUsedPassword() is true, and
then this check should fail if no "password" parameter was given. That
check should be expanded to allow alternatively passing the SCRAM key
component parameters.

pgfdw_security_check() is currently not called if SCRAM passthrough is
in use, though:

/*
* Perform post-connection security checks only if scram pass-through
* is not being used because the password is not necessary.
*/
if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user)))
pgfdw_security_check(keywords, values, user, conn);

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?

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.

I like that this solution addresses some of the concerns from my dblink review.

--

Not part of this patchset, but I think the errmsg in
pgfdw_security_check() is confusing:

ERROR: password or GSSAPI delegated credentials required
DETAIL: Non-superuser cannot connect if the server does not
request a password or...
HINT: Target server's authentication method must be changed or...

For the user to have gotten past check_conn_params, they *have*
provided a password/credentials. But the server didn't ask for it (or
at least, not the right one). The detail and hint messages are correct
here, but I'd argue the error message itself is not.

Thanks!
--Jacob

On Mon, Mar 10, 2025 at 11:25 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

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.

Thanks,
--Jacob

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

On Thu, Mar 13, 2025 at 6:59 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

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.

Thanks! Looks like the regression suite has one test that takes that
path (found by adding an Assert(false) to the PG_CATCH branch):

SET SESSION AUTHORIZATION regress_dblink_user;
-- should fail
SELECT dblink_connect('myconn', 'fdtest');

PG_CATCH();
{
if (rconn)
pfree(rconn);

A comment in this branch might be nice, to draw attention to the fact
that rconn is allocated in the TopMemoryContext and we can't leak it.

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.

That comment does not seem to match the code now:

+  * All required scram options is set by ourself, so we just need to ensure
+  * that these options are not overwritten by the user.

But later, there's no provision to detect if the keys have been overwritten:

+                  if (strcmp(option->keyword, "scram_client_key") == 0 && option->val[0] != '\0')
+                          has_scram_client_key = true;
+                  if (strcmp(option->keyword, "scram_server_key") == 0 && option->val[0] != '\0')
+                          has_scram_server_key = true;

This needs to match the handling directly above it, if we want to
claim that we'll detect duplicates.

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;
+}

If we implement this, it needs to check that the keys were actually
sent during scram_exchange(). Having them set on the PGconn doesn't
mean that we used them for authentication.

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?

Right. I think it'll come down to how Peter feels about putting that
into the solution, vs. PQconnectionUsedScramKeys() or some third
option.

--

Miscellaneous patch review:

-dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
+dblink_security_check(PGconn *conn, remoteConn *rconn,
+                                    const char *connstr)

nit: this whitespace change is not necessary now that
useScramPassthrough is no longer in the signature.

Speaking of which, does get_connect_string() still need to take
user_mapping as an argument?

+  if (has_scram_keys && has_require_auth)
+          return true;
+
+  return false;

nit: this is equivalent to `return (has_scram_keys && has_require_auth);`

+ my ($ret2, $stdout2, $stderr2) = $node->psql(

Declaring a second set of return values is probably unnecessary; the
previous ones can be reused.

+  is( $stderr,
+      "psql:<stdin>:1: ERROR:  invalid option \"scram_client_key\"",
+      'user mapping creation fails when using scram_client_key');

I think the two new tests like this should be using like() rather than
is() so that they can match only the important part of the error. I
don't think we want to pin the "psql:<stdin>" stuff in this test.

+($ret, $stdout, $stderr) = $node1->psql(
+  $db0,
+  "SELECT * FROM dblink('$fdw_server', 'SELECT * FROM t') AS t(a int, b int)",
+  connstr => $node1->connstr($db0) . " user=$user");
+
+is($ret, 3, 'loopback trust fails on the same cluster');
+like(
+  $stderr,
+  qr/failed: authentication method requirement "scram-sha-256" failed: server did not complete authentication/,
+  'expected error from loopback trust (same cluster)');

Is this the same as the previous loopback-trust test? If so I think it
can be removed (or the two sections merged completely).

Thanks!
--Jacob

On Thu, Mar 13, 2025 at 4:54 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Thu, Mar 13, 2025 at 6:59 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

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.

Thanks! Looks like the regression suite has one test that takes that
path (found by adding an Assert(false) to the PG_CATCH branch):

SET SESSION AUTHORIZATION regress_dblink_user;
-- should fail
SELECT dblink_connect('myconn', 'fdtest');

PG_CATCH();
{
if (rconn)
pfree(rconn);

A comment in this branch might be nice, to draw attention to the fact
that rconn is allocated in the TopMemoryContext and we can't leak it.

Fixed

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.

That comment does not seem to match the code now:

+  * All required scram options is set by ourself, so we just need to ensure
+  * that these options are not overwritten by the user.

But later, there's no provision to detect if the keys have been overwritten:

+                  if (strcmp(option->keyword, "scram_client_key") == 0 && option->val[0] != '\0')
+                          has_scram_client_key = true;
+                  if (strcmp(option->keyword, "scram_server_key") == 0 && option->val[0] != '\0')
+                          has_scram_server_key = true;

This needs to match the handling directly above it, if we want to
claim that we'll detect duplicates.

I thought about this; The problem is that at this point, the scram keys
on connection options are base64 encoded (see appendSCRAMKeysInfo), so
we can't compare with the values stored on MyProcPort. I don't know if
decoding the option or encoding the keys on MyProcPort from/to base64 is
a way to go, what do you think?

I've implemented this check in this way because we don't allow adding
the scram keys on user mapping or foreign server options, so the user
can't actually overwrite the scram keys, unless there is the possibility
of filling in these scram keys options in other places besides user
mapping and foreign server options that I am missing?

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;
+}

If we implement this, it needs to check that the keys were actually
sent during scram_exchange(). Having them set on the PGconn doesn't
mean that we used them for authentication.

We use the client key and server key on calculate_client_proof and
verify_server_signature respective during memcpy, it would be too hack
to add new fields on pg_conn like scram_client_key_in_use and
scram_server_key_in_use, set them to true on these functions and then
validate that both are true on PQconnectionUsedScramKeys?

--

Miscellaneous patch review:

-dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
+dblink_security_check(PGconn *conn, remoteConn *rconn,
+                                    const char *connstr)

nit: this whitespace change is not necessary now that
useScramPassthrough is no longer in the signature.

Fixed

Speaking of which, does get_connect_string() still need to take
user_mapping as an argument?

Yes, because we need to check if the use_scram_passthrough option is set
on foreign server or user mapping options:
if (MyProcPort->has_scram_keys && UseScramPassthrough(foreign_server,
user_mapping))
appendSCRAMKeysInfo(&buf);

+  if (has_scram_keys && has_require_auth)
+          return true;
+
+  return false;

nit: this is equivalent to `return (has_scram_keys && has_require_auth);`

Fixed

+ my ($ret2, $stdout2, $stderr2) = $node->psql(

Declaring a second set of return values is probably unnecessary; the
previous ones can be reused.

Fixed

+  is( $stderr,
+      "psql:<stdin>:1: ERROR:  invalid option \"scram_client_key\"",
+      'user mapping creation fails when using scram_client_key');

I think the two new tests like this should be using like() rather than
is() so that they can match only the important part of the error. I
don't think we want to pin the "psql:<stdin>" stuff in this test.

Yes, having "psq:<stdin>" is weird, fixed.

+($ret, $stdout, $stderr) = $node1->psql(
+  $db0,
+  "SELECT * FROM dblink('$fdw_server', 'SELECT * FROM t') AS t(a int, b int)",
+  connstr => $node1->connstr($db0) . " user=$user");
+
+is($ret, 3, 'loopback trust fails on the same cluster');
+like(
+  $stderr,
+  qr/failed: authentication method requirement "scram-sha-256" failed: server did not complete authentication/,
+  'expected error from loopback trust (same cluster)');

Is this the same as the previous loopback-trust test? If so I think it
can be removed (or the two sections merged completely).

The only difference is using "trust" vs "password" on $db2 pg_hba.conf,
but the expectation of the test is the same. I've just removed the test
using "trust". Good catch, I've made a small confusion on these tests.

Thanks for all the comments!

--
Matheus Alcantara

On Thu, Mar 13, 2025 at 2:38 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

I thought about this; The problem is that at this point, the scram keys
on connection options are base64 encoded (see appendSCRAMKeysInfo), so
we can't compare with the values stored on MyProcPort.

I don't think that's necessary -- the important part is to check
whether they've been unset (empty string).

I've implemented this check in this way because we don't allow adding
the scram keys on user mapping or foreign server options, so the user
can't actually overwrite the scram keys, unless there is the possibility
of filling in these scram keys options in other places besides user
mapping and foreign server options that I am missing?

Understood, but there are two separate comments that claim the code
does something that it doesn't:

+  * All required scram options is set by ourself, so we just need to ensure
+  * that these options are not overwritten by the user.

and

+    * First append hardcoded options needed for SCRAM pass-through, so if the
+    * user overwrite these options we can ereport on dblink_connstr_check and
+    * dblink_security_check.

If the check functions aren't going to check those because it's
unnecessary, then that's fine, but then the comments should be
adjusted.

If we implement this, it needs to check that the keys were actually
sent during scram_exchange(). Having them set on the PGconn doesn't
mean that we used them for authentication.

We use the client key and server key on calculate_client_proof and
verify_server_signature respective during memcpy, it would be too hack
to add new fields on pg_conn like scram_client_key_in_use and
scram_server_key_in_use, set them to true on these functions and then
validate that both are true on PQconnectionUsedScramKeys?

I think that's probably a question for Peter: whether or not that
additional API is something we want to support.

-dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
+dblink_security_check(PGconn *conn, remoteConn *rconn,
+                                    const char *connstr)

nit: this whitespace change is not necessary now that
useScramPassthrough is no longer in the signature.

Fixed

(This diff is still present in v6-0002.)

Speaking of which, does get_connect_string() still need to take
user_mapping as an argument?

Yes, because we need to check if the use_scram_passthrough option is set
on foreign server or user mapping options:
if (MyProcPort->has_scram_keys && UseScramPassthrough(foreign_server,
user_mapping))
appendSCRAMKeysInfo(&buf);

I was referring to the discussion upthread [1]/messages/by-id/CAFY6G8f=jRUAP5yiFRZkHmqstCiRkeeD5Zf2ixVf6HMmjBCgfg@mail.gmail.com; you'd mentioned that
the only reason that get_connect_string() didn't call GetUserMapping()
itself was because we needed that mapping later on for
UseScramPassthrough(). But that's no longer true for this patch,
because the later call to UseScramPassthrough() has been removed. So I
think we can move GetUserMapping() back down, and remove that part of
the refactoring from patch 0001.

Thanks!
--Jacob

[1]: /messages/by-id/CAFY6G8f=jRUAP5yiFRZkHmqstCiRkeeD5Zf2ixVf6HMmjBCgfg@mail.gmail.com

On Mon, Mar 17, 2025 at 1:49 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

On Thu, Mar 13, 2025 at 2:38 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

I thought about this; The problem is that at this point, the scram keys
on connection options are base64 encoded (see appendSCRAMKeysInfo), so
we can't compare with the values stored on MyProcPort.

I don't think that's necessary -- the important part is to check
whether they've been unset (empty string).

I've implemented this check in this way because we don't allow adding
the scram keys on user mapping or foreign server options, so the user
can't actually overwrite the scram keys, unless there is the possibility
of filling in these scram keys options in other places besides user
mapping and foreign server options that I am missing?

Understood, but there are two separate comments that claim the code
does something that it doesn't:

+  * All required scram options is set by ourself, so we just need to ensure
+  * that these options are not overwritten by the user.

and

+    * First append hardcoded options needed for SCRAM pass-through, so if the
+    * user overwrite these options we can ereport on dblink_connstr_check and
+    * dblink_security_check.

If the check functions aren't going to check those because it's
unnecessary, then that's fine, but then the comments should be
adjusted.

Ok, now I get all of your points. I've misinterpreted your comments,
sorry about that. I've changed on v7 to validate the scram keys using
the same approach implemented for require_auth, so that now we correctly
check for overwritten scram keys on connection options. I think that the
code comments should be correct now?

If we implement this, it needs to check that the keys were actually
sent during scram_exchange(). Having them set on the PGconn doesn't
mean that we used them for authentication.

We use the client key and server key on calculate_client_proof and
verify_server_signature respective during memcpy, it would be too hack
to add new fields on pg_conn like scram_client_key_in_use and
scram_server_key_in_use, set them to true on these functions and then
validate that both are true on PQconnectionUsedScramKeys?

I think that's probably a question for Peter: whether or not that
additional API is something we want to support.

Ok

-dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
+dblink_security_check(PGconn *conn, remoteConn *rconn,
+                                    const char *connstr)

nit: this whitespace change is not necessary now that
useScramPassthrough is no longer in the signature.

Fixed

(This diff is still present in v6-0002.)

Sorry, I think that now is fixed.

Speaking of which, does get_connect_string() still need to take
user_mapping as an argument?

Yes, because we need to check if the use_scram_passthrough option is set
on foreign server or user mapping options:
if (MyProcPort->has_scram_keys && UseScramPassthrough(foreign_server,
user_mapping))
appendSCRAMKeysInfo(&buf);

I was referring to the discussion upthread [1]; you'd mentioned that
the only reason that get_connect_string() didn't call GetUserMapping()
itself was because we needed that mapping later on for
UseScramPassthrough(). But that's no longer true for this patch,
because the later call to UseScramPassthrough() has been removed. So I
think we can move GetUserMapping() back down, and remove that part of
the refactoring from patch 0001.

Ok, it totally makes sense. Fixed on v7.

--
Matheus Alcantara

On Mon, Mar 17, 2025 at 11:54 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

If the check functions aren't going to check those because it's
unnecessary, then that's fine, but then the comments should be
adjusted.

Ok, now I get all of your points. I've misinterpreted your comments,
sorry about that. I've changed on v7 to validate the scram keys using
the same approach implemented for require_auth, so that now we correctly
check for overwritten scram keys on connection options. I think that the
code comments should be correct now?

Looks good.

I was referring to the discussion upthread [1]; you'd mentioned that
the only reason that get_connect_string() didn't call GetUserMapping()
itself was because we needed that mapping later on for
UseScramPassthrough(). But that's no longer true for this patch,
because the later call to UseScramPassthrough() has been removed. So I
think we can move GetUserMapping() back down, and remove that part of
the refactoring from patch 0001.

Ok, it totally makes sense. Fixed on v7.

The fix is in, but it needs to be part of 0001 rather than 0002;
otherwise 0001 doesn't compile.

--

A pgperltidy run is needed for some of the more recent test changes.
But I'm rapidly running out of feedback, so I think this is very
close.

Thanks!
--Jacob