Hi, thanks for reviewing this patch! Em seg., 10 de fev. de 2025 às 20:19, Jacob Champion <jacob.champ...@enterprisedb.com> escreveu: > > On Wed, Jan 22, 2025 at 6:10 AM Matheus Alcantara > <matheusssil...@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
v2-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
v2-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
