On Wed, Mar 19, 2025 at 4:21 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > Yeah, I think option (2) is enough for now. If someone wants to enable
> > the kinds of things you describe, they can always come back and
> > implement option (1) later.
>
> Sounds good to me.
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.
> Notes on v8:
>
> - The following documentation pieces need to be adjusted, now that
> we've decided that `use_scram_passthrough` will enforce
> `require_auth=scram-sha-256`:
>
> > + The remote server must request SCRAM authentication. (If desired,
> > + enforce this on the client side (FDW side) with the option
> > + <literal>require_auth</literal>.) If another authentication method is
> > + requested by the server, then that one will be used normally.
>
> and
>
> > + The user mapping password is not used. (It could be set to support other
> > + authentication methods, but that would arguably violate the point of this
> > + feature, which is to avoid storing plain-text passwords.)
>
> 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?
+ ... With SCRAM pass-through
+ authentication, <filename>dblink_fdw</filename> uses SCRAM-hashed secrets
+ instead of plain-text user passwords to connect to the remote server. This
+ avoids storing plain-text user passwords in PostgreSQL system catalogs.
> - In get_connect_string():
>
> > + /* first gather the server connstr options */
> > + Oid serverid = foreign_server->serverid;
>
> I think this comment belongs elsewhere (connect_pg_server) and should
> be deleted from this block.
Removed
> - Sorry for not realizing this before now, but I couldn't figure out
> why connect_pg_server() took the rconn pointer, and it turns out it
> just passes it along to dblink_security_check(), which pfree's it
> before throwing an error. So that will double-free with the current
> refactoring patch (and I'm not sure why assertions aren't catching
> that?).
>
> I thought for sure this inconsistency would be a problem on HEAD, but
> it turns out that rconn is set to NULL in the code path where it would
> be a bug... How confusing.
>
> 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.
> > + /* Verify the set of connection parameters. */
> > dblink_connstr_check(connstr);
> > ...
> > + /* Perform post-connection security checks. */
> > dblink_security_check(conn, rconn, connstr);
>
> - These comment additions probably belong in 0001 rather than 0002.
Fixed
> - As discussed offlist, 0002 needs pgperltidy'd rather than perltidy'd.
Fixed
> - I have attached some additional nitpicky comment edits and
> whitespace changes as a diff; pick and choose as desired.
I've squashed into this new version thanks!
--
Matheus Alcantara
