Horton

Thread

Commits

  1. dblink: SCRAM authentication pass-through

  2. postgres_fdw: improve security checks

  1. dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-01-22T14:10:38Z

    Hi,

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

To make it more closer to what was implemented on postgres_fdw a refactor was
needed on get connection routines. A connect_pg_server function was created to
centralize all the logic to actually open the connection with the foreign
server and then replace the duplicated code on dblink_get_conn and
dblink_connect to just call this new function. The main reason for this
refactor was to centralize the SCRAM logic in a single function, instead of
checking if SCRAM is enabled on both functions.

A new is_valid_dblink_fdw_option function was also created to check for valid
dblink fdw options when creating a server with CREATE SERVER command. The
is_valid_dblink_option function only checks for valid libpq options, and the
use_scram_passthrough option is only valid on CREATE SERVER options.

The documentation was also changed to include a new Foreign Data Wrapper
section to describe the SCRAM pass-through feature.

Thoughts?

[0] https://www.postgresql.org/message-id/27b29a35-9b96-46a9-bc1a-914140869dac@gmail.com

-- 
Matheus Alcantara

  2. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-02-10T23:19:46Z

    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

  3. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-02-12T19:53:46Z

    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

  4. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-02-13T23:25:46Z

    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

  5. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-02-19T20:01:58Z

    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

  6. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-02-19T23:19:10Z

    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

  7. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-02-21T14:47:39Z

    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

  8. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-03T17:01:37Z

    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], 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)

  9. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-06T19:24:25Z

    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

  10. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-06T20:33:16Z

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

  11. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-06T21:58:55Z

    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

  12. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-07T16:22:12Z

    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?

  13. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-10T18:25:51Z

    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

  14. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-12T15:17:55Z

    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

  15. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-13T13:59:17Z

    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

  16. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-13T19:54:46Z

    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

  17. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-13T21:38:26Z

    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

  18. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-17T16:49:17Z

    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]; 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] https://postgr.es/m/CAFY6G8f%3DjRUAP5yiFRZkHmqstCiRkeeD5Zf2ixVf6HMmjBCgfg%40mail.gmail.com

  19. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-17T18:54:08Z

    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

  20. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-17T23:26:40Z

    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

  21. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-18T13:10:03Z

    On Mon, Mar 17, 2025 at 8:26 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> > > 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.
>
Fixed. 0001 and 0002 are compiling now.

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

Thanks very much for all the reviews on this patch!

-- 
Matheus Alcantara

  22. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-18T16:35:46Z

    On 17.03.25 17:49, Jacob Champion wrote:
>>> 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.

So the way I understand this is that the options are:

(1) We add a libpq function like PQconnectionUsedScramKeys() in the 
style of PQconnectionUsedPassword() and call that function during the 
checks.

(2) We make use_scram_passthrough=true imply require_auth=scram-sha-256. 
  This is essentially a way to get the info from (1) out of libpq using 
existing facilities.  But it would preempt certain setups that might 
otherwise work.  (Which ones?  Are they important?)

Why can't we use PQconnectionUsedPassword()?  What problems would that 
leave?  The example test case that Jacob showed earlier involved the 
remote server using "trust".  We don't want that, of course.  But what 
we want to make sure is that some kind of authentication happened 
between postgres_fdw and the remote server.  PQconnectionUsedPassword() 
does indicate that.

(Or could we just stick in require_auth=!none to solve this problem once 
and for all?)

  23. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-18T16:53:51Z

    On Tue, Mar 18, 2025 at 9:35 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> So the way I understand this is that the options are:
>
> (1) We add a libpq function like PQconnectionUsedScramKeys() in the
> style of PQconnectionUsedPassword() and call that function during the
> checks.
>
> (2) We make use_scram_passthrough=true imply require_auth=scram-sha-256.
>   This is essentially a way to get the info from (1) out of libpq using
> existing facilities.

Right.

> But it would preempt certain setups that might
> otherwise work.  (Which ones?  Are they important?)

If the backend HBA later changes, to require delegated GSS or a
different type of password authentication, the user will have to unset
use_scram_passthrough (or ask the owner of the foreign server to unset
it). Whereas before they could just add a password to their user
mapping or enable delegation to move forward immediately.

I think this is probably not a serious limitation, in practice.

> Why can't we use PQconnectionUsedPassword()?  What problems would that
> leave?  The example test case that Jacob showed earlier involved the
> remote server using "trust". We don't want that, of course.  But what
> we want to make sure is that some kind of authentication happened
> between postgres_fdw and the remote server.  PQconnectionUsedPassword()
> does indicate that.

That test also added a "password" HBA case, where the correct password
got pulled from the environment instead of the connection string.
Making sure authentication happens is only one part -- we have to
ensure authentication takes place using the end user's credentials and
not the server's.

So since PQconnectionUsedPassword() can't differentiate between "I
used your SCRAM key" and "I used a password I found lying around on
disk", it's not strong enough for this check.

> (Or could we just stick in require_auth=!none to solve this problem once
> and for all?)

It solves the trust case nicely (and we should maybe consider it for a
future simplification?), but not the "wrong credentials were used"
case.

Thanks!
--Jacob

  24. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-18T19:32:35Z

    On 18.03.25 17:53, Jacob Champion wrote:
> On Tue, Mar 18, 2025 at 9:35 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> So the way I understand this is that the options are:
>>
>> (1) We add a libpq function like PQconnectionUsedScramKeys() in the
>> style of PQconnectionUsedPassword() and call that function during the
>> checks.
>>
>> (2) We make use_scram_passthrough=true imply require_auth=scram-sha-256.
>>    This is essentially a way to get the info from (1) out of libpq using
>> existing facilities.
> 
> Right.
> 
>> But it would preempt certain setups that might
>> otherwise work.  (Which ones?  Are they important?)
> 
> If the backend HBA later changes, to require delegated GSS or a
> different type of password authentication, the user will have to unset
> use_scram_passthrough (or ask the owner of the foreign server to unset
> it). Whereas before they could just add a password to their user
> mapping or enable delegation to move forward immediately.
> 
> I think this is probably not a serious limitation, in practice.

Yeah, I think option (2) is enough for now.  If someone wants to enable 
the kinds of things you describe, they can always come back and 
implement option (1) later.

  25. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-19T19:21:02Z

    On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> Yeah, I think option (2) is enough for now.  If someone wants to enable
> the kinds of things you describe, they can always come back and
> implement option (1) later.

Sounds good to me.

--

Notes on v8:

- The following documentation pieces need to be adjusted, now that
we've decided that `use_scram_passthrough` will enforce
`require_auth=scram-sha-256`:

> +       The remote server must request SCRAM authentication.  (If desired,
> +       enforce this on the client side (FDW side) with the option
> +       <literal>require_auth</literal>.)  If another authentication method is
> +       requested by the server, then that one will be used normally.

and

> +      The user mapping password is not used.  (It could be set to support other
> +      authentication methods, but that would arguably violate the point of this
> +      feature, which is to avoid storing plain-text passwords.)

I think they should just be reduced to "The remote server must request
SCRAM authentication." and "The user mapping password is not used."

- In get_connect_string():

> +   /* first gather the server connstr options */
> +   Oid         serverid = foreign_server->serverid;

I think this comment belongs elsewhere (connect_pg_server) and should
be deleted from this block.

- Sorry for not realizing this before now, but I couldn't figure out
why connect_pg_server() took the rconn pointer, and it turns out it
just passes it along to dblink_security_check(), which pfree's it
before throwing an error. So that will double-free with the current
refactoring patch (and I'm not sure why assertions aren't catching
that?).

I thought for sure this inconsistency would be a problem on HEAD, but
it turns out that rconn is set to NULL in the code path where it would
be a bug... How confusing.

Now that we handle the pfree() in PG_CATCH instead, that lower-level
pfree should be removed, and then connect_pg_server() doesn't need to
take an rconn pointer at all. For extra credit you could maybe move
the allocation of rconn down below the call to connect_pg_server(),
and get rid of the try/catch?

> +   /* Verify the set of connection parameters. */
>     dblink_connstr_check(connstr);
> ...
> +   /* Perform post-connection security checks. */
>     dblink_security_check(conn, rconn, connstr);

- These comment additions probably belong in 0001 rather than 0002.

- As discussed offlist, 0002 needs pgperltidy'd rather than perltidy'd.

- I have attached some additional nitpicky comment edits and
whitespace changes as a diff; pick and choose as desired.

Thanks!
--Jacob

  26. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-20T19:54:08Z

    On Wed, Mar 19, 2025 at 4:21 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > Yeah, I think option (2) is enough for now.  If someone wants to enable
> > the kinds of things you describe, they can always come back and
> > implement option (1) later.
>
> Sounds good to me.

Since the security checks are defined I'm attaching 0003 which include
the fix of security checks for postgres_fdw. It implements the
validations very similar to what are being implemented on dblink.

> Notes on v8:
>
> - The following documentation pieces need to be adjusted, now that
> we've decided that `use_scram_passthrough` will enforce
> `require_auth=scram-sha-256`:
>
> > +       The remote server must request SCRAM authentication.  (If desired,
> > +       enforce this on the client side (FDW side) with the option
> > +       <literal>require_auth</literal>.)  If another authentication method is
> > +       requested by the server, then that one will be used normally.
>
> and
>
> > +      The user mapping password is not used.  (It could be set to support other
> > +      authentication methods, but that would arguably violate the point of this
> > +      feature, which is to avoid storing plain-text passwords.)
>
> I think they should just be reduced to "The remote server must request
> SCRAM authentication." and "The user mapping password is not used."

I've removed the "user mapping password" <listitem> because we already
mentioned above that the password is not used and having just "The user
mapping password is not used." again seems redundant, what do you think?

+    ... With SCRAM pass-through
+    authentication, <filename>dblink_fdw</filename> uses SCRAM-hashed secrets
+    instead of plain-text user passwords to connect to the remote server. This
+    avoids storing plain-text user passwords in PostgreSQL system catalogs.


> - In get_connect_string():
>
> > +   /* first gather the server connstr options */
> > +   Oid         serverid = foreign_server->serverid;
>
> I think this comment belongs elsewhere (connect_pg_server) and should
> be deleted from this block.

Removed

> - Sorry for not realizing this before now, but I couldn't figure out
> why connect_pg_server() took the rconn pointer, and it turns out it
> just passes it along to dblink_security_check(), which pfree's it
> before throwing an error. So that will double-free with the current
> refactoring patch (and I'm not sure why assertions aren't catching
> that?).
>
> I thought for sure this inconsistency would be a problem on HEAD, but
> it turns out that rconn is set to NULL in the code path where it would
> be a bug... How confusing.
>
> Now that we handle the pfree() in PG_CATCH instead, that lower-level
> pfree should be removed, and then connect_pg_server() doesn't need to
> take an rconn pointer at all. For extra credit you could maybe move
> the allocation of rconn down below the call to connect_pg_server(),
> and get rid of the try/catch?

Good catch, thanks, it's much better now! With this change we can also
remove the second if (connname) condition. All fixed on attached.

> > +   /* Verify the set of connection parameters. */
> >     dblink_connstr_check(connstr);
> > ...
> > +   /* Perform post-connection security checks. */
> >     dblink_security_check(conn, rconn, connstr);
>
> - These comment additions probably belong in 0001 rather than 0002.

Fixed

> - As discussed offlist, 0002 needs pgperltidy'd rather than perltidy'd.

Fixed

> - I have attached some additional nitpicky comment edits and
> whitespace changes as a diff; pick and choose as desired.

I've squashed into this new version thanks!

-- 
Matheus Alcantara

  27. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-21T00:02:36Z

    On Thu, Mar 20, 2025 at 12:54 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
> Since the security checks are defined I'm attaching 0003 which include
> the fix of security checks for postgres_fdw. It implements the
> validations very similar to what are being implemented on dblink.

Comments on 0003:

> +           keywords[n] = "require_auth";
> +           values[n] = "scram-sha-256";
> +           n++;

The keywords and values arrays need to be lengthened for this.

>     host    all             all             $hostaddr/32            scram-sha-256
> -   });
> +   }
> +   );

Accidental diff?

A few whitespace and comment tweaks are attached as well.

--

> > I think they should just be reduced to "The remote server must request
> > SCRAM authentication." and "The user mapping password is not used."
>
> I've removed the "user mapping password" <listitem> because we already
> mentioned above that the password is not used and having just "The user
> mapping password is not used." again seems redundant, what do you think?

Personally, I think it's still useful to call out that the password in
the user mapping is explicitly ignored. The other text motivates the
feature, but it doesn't explain how it interacts with existing user
mappings (most of which will have passwords).

> > Now that we handle the pfree() in PG_CATCH instead, that lower-level
> > pfree should be removed, and then connect_pg_server() doesn't need to
> > take an rconn pointer at all. For extra credit you could maybe move
> > the allocation of rconn down below the call to connect_pg_server(),
> > and get rid of the try/catch?
>
> Good catch, thanks, it's much better now! With this change we can also
> remove the second if (connname) condition. All fixed on attached.

I like that; the patch is a lot easier to follow now.

Thanks,
--Jacob

  28. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-21T15:32:24Z

    On Thu, Mar 20, 2025 at 9:02 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Thu, Mar 20, 2025 at 12:54 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > Since the security checks are defined I'm attaching 0003 which include
> > the fix of security checks for postgres_fdw. It implements the
> > validations very similar to what are being implemented on dblink.
>
> Comments on 0003:
>
> > +           keywords[n] = "require_auth";
> > +           values[n] = "scram-sha-256";
> > +           n++;
>
> The keywords and values arrays need to be lengthened for this.

Fixed. I've also changed the code comment to mention the scram keys and
required options.


> >     host    all             all             $hostaddr/32            scram-sha-256
> > -   });
> > +   }
> > +   );
>
> Accidental diff?

Yep, sorry, I made some confusion with dblink formatting. Removed

> A few whitespace and comment tweaks are attached as well.

Squashed

> --
>
> > > I think they should just be reduced to "The remote server must request
> > > SCRAM authentication." and "The user mapping password is not used."
> >
> > I've removed the "user mapping password" <listitem> because we already
> > mentioned above that the password is not used and having just "The user
> > mapping password is not used." again seems redundant, what do you think?
>
> Personally, I think it's still useful to call out that the password in
> the user mapping is explicitly ignored. The other text motivates the
> feature, but it doesn't explain how it interacts with existing user
> mappings (most of which will have passwords).

Fair point. I've changed it to just "The user mapping password is not
used".

-- 
Matheus Alcantara

  29. Re: dblink: Add SCRAM pass-through authentication

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-21T16:28:12Z

    Great, thank you! Looking over v10, I think I've run out of feedback
at this point. Marked Ready for Committer.

--Jacob

  30. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-21T18:24:25Z

    On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> Great, thank you! Looking over v10, I think I've run out of feedback
> at this point. Marked Ready for Committer.

Thanks for all the effort reviewing this patch!

-- 
Matheus Alcantara

  31. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-24T16:16:40Z

    On 21.03.25 19:24, Matheus Alcantara wrote:
> On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>>
>> Great, thank you! Looking over v10, I think I've run out of feedback
>> at this point. Marked Ready for Committer.
> 
> Thanks for all the effort reviewing this patch!

I have committed the 0003 patch (the postgres_fdw bug fix).

The dblink feature patch (0002) looks good to me.

I'm a bit confused about the refactoring patch 0001.  There are some 
details there that don't seem right.  For example, you write that the 
pfree(rconn) calls are no longer necessary, but AFAICT, it would still 
be needed in dblink_get_conn().  Also, there appear to be some possible 
behavior changes, or at least it's not fully explained, like 
connect_pg_server() doing foreign-server name resolution, which 
dblink_get_conn() did not do before.

But it's actually not clear to me how the refactoring in 0001 
contributes to making the patch 0002 better, since patch 0002 barely 
touches the code touched by 0001.

How would patch 0002 look without 0001 before it?  Which code would need 
to be duplicated or what other awkward changes are you trying to avoid?

  32. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-24T20:33:26Z

    On Mon, Mar 24, 2025 at 1:16 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 21.03.25 19:24, Matheus Alcantara wrote:
> > On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
> > <jacob.champion@enterprisedb.com> wrote:
> >>
> >> Great, thank you! Looking over v10, I think I've run out of feedback
> >> at this point. Marked Ready for Committer.
> >
> > Thanks for all the effort reviewing this patch!
>
> I have committed the 0003 patch (the postgres_fdw bug fix).

Thanks!

> I'm a bit confused about the refactoring patch 0001.  There are some
> details there that don't seem right.  For example, you write that the
> pfree(rconn) calls are no longer necessary, but AFAICT, it would still
> be needed in dblink_get_conn().  Also, there appear to be some possible
> behavior changes, or at least it's not fully explained, like
> connect_pg_server() doing foreign-server name resolution, which
> dblink_get_conn() did not do before.
>
> But it's actually not clear to me how the refactoring in 0001
> contributes to making the patch 0002 better, since patch 0002 barely
> touches the code touched by 0001.
>
> How would patch 0002 look without 0001 before it?  Which code would need
> to be duplicated or what other awkward changes are you trying to avoid?

You are right, I think that the refactor was needed on the initial
versions of the patch because it was referencing the UseScramPassthrough
function in multiple places, so the refactor was needed to accomplish the
parameters of the function.

Since we now assume that the UseScramPassthrough is already checked on
some parts of the code I agree that this refactor is not required
anymore. Attached v11 without the refactor patch.

Thanks!

-- 
Matheus Alcantara

  33. Re: dblink: Add SCRAM pass-through authentication

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-26T10:41:18Z

    On 24.03.25 21:33, Matheus Alcantara wrote:
>> I'm a bit confused about the refactoring patch 0001.  There are some
>> details there that don't seem right.  For example, you write that the
>> pfree(rconn) calls are no longer necessary, but AFAICT, it would still
>> be needed in dblink_get_conn().  Also, there appear to be some possible
>> behavior changes, or at least it's not fully explained, like
>> connect_pg_server() doing foreign-server name resolution, which
>> dblink_get_conn() did not do before.
>>
>> But it's actually not clear to me how the refactoring in 0001
>> contributes to making the patch 0002 better, since patch 0002 barely
>> touches the code touched by 0001.
>>
>> How would patch 0002 look without 0001 before it?  Which code would need
>> to be duplicated or what other awkward changes are you trying to avoid?
> You are right, I think that the refactor was needed on the initial
> versions of the patch because it was referencing the UseScramPassthrough
> function in multiple places, so the refactor was needed to accomplish the
> parameters of the function.
> 
> Since we now assume that the UseScramPassthrough is already checked on
> some parts of the code I agree that this refactor is not required
> anymore. Attached v11 without the refactor patch.

Committed.

I cut down the documentation a bit and instead linked to postgres_fdw 
for some of the details.  I think that's better than having to maintain 
that text in two different places.

  34. Re: dblink: Add SCRAM pass-through authentication

    Matheus Alcantara <matheusssilv97@gmail.com> — 2025-03-26T11:39:05Z

    On Wed, Mar 26, 2025 at 7:41 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 24.03.25 21:33, Matheus Alcantara wrote:
> >> I'm a bit confused about the refactoring patch 0001.  There are some
> >> details there that don't seem right.  For example, you write that the
> >> pfree(rconn) calls are no longer necessary, but AFAICT, it would still
> >> be needed in dblink_get_conn().  Also, there appear to be some possible
> >> behavior changes, or at least it's not fully explained, like
> >> connect_pg_server() doing foreign-server name resolution, which
> >> dblink_get_conn() did not do before.
> >>
> >> But it's actually not clear to me how the refactoring in 0001
> >> contributes to making the patch 0002 better, since patch 0002 barely
> >> touches the code touched by 0001.
> >>
> >> How would patch 0002 look without 0001 before it?  Which code would need
> >> to be duplicated or what other awkward changes are you trying to avoid?
> > You are right, I think that the refactor was needed on the initial
> > versions of the patch because it was referencing the UseScramPassthrough
> > function in multiple places, so the refactor was needed to accomplish the
> > parameters of the function.
> >
> > Since we now assume that the UseScramPassthrough is already checked on
> > some parts of the code I agree that this refactor is not required
> > anymore. Attached v11 without the refactor patch.
>
> Committed.
>
> I cut down the documentation a bit and instead linked to postgres_fdw
> for some of the details.  I think that's better than having to maintain
> that text in two different places.

Thanks!

-- 
Matheus Alcantara