Re: dblink: Add SCRAM pass-through authentication - Mailing list pgsql-hackers

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