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
