Thread: Re: dblink: Add SCRAM pass-through authentication

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

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

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

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

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

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.

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

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

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

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

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

--Jacob

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

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?

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

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.

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