Re: dblink: Add SCRAM pass-through authentication

On Thu, Mar 13, 2025 at 4:54 PM Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Thu, Mar 13, 2025 at 6:59 AM Matheus Alcantara
> <matheusssilv97(at)gmail(dot)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