Re: dblink: Add SCRAM pass-through authentication

Hi, thanks for reviewing this patch!

Em seg., 10 de fev. de 2025 às 20:19, Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> escreveu:
>
> On Wed, Jan 22, 2025 at 6:10 AM Matheus Alcantara
> <matheusssilv97(at)gmail(dot)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