On Wed, Mar 19, 2025 at 4:21 PM Jacob Champion <jacob.champ...@enterprisedb.com> wrote: > > On Tue, Mar 18, 2025 at 12:32 PM Peter Eisentraut <pe...@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
v9-0003-postgres_fdw-improve-security-checks.patch
Description: Binary data
v9-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
v9-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
