On Mon, Mar 17, 2025 at 1:49 PM Jacob Champion <jacob.champ...@enterprisedb.com> wrote: > > On Thu, Mar 13, 2025 at 2:38 PM Matheus Alcantara > <matheusssil...@gmail.com> wrote: > > I thought about this; The problem is that at this point, the scram keys > > on connection options are base64 encoded (see appendSCRAMKeysInfo), so > > we can't compare with the values stored on MyProcPort. > > I don't think that's necessary -- the important part is to check > whether they've been unset (empty string). > > > I've implemented this check in this way because we don't allow adding > > the scram keys on user mapping or foreign server options, so the user > > can't actually overwrite the scram keys, unless there is the possibility > > of filling in these scram keys options in other places besides user > > mapping and foreign server options that I am missing? > > Understood, but there are two separate comments that claim the code > does something that it doesn't: > > + * All required scram options is set by ourself, so we just need to ensure > + * that these options are not overwritten by the user. > > and > > + * First append hardcoded options needed for SCRAM pass-through, so if the > + * user overwrite these options we can ereport on dblink_connstr_check and > + * dblink_security_check. > > If the check functions aren't going to check those because it's > unnecessary, then that's fine, but then the comments should be > adjusted. > Ok, now I get all of your points. I've misinterpreted your comments, sorry about that. I've changed on v7 to validate the scram keys using the same approach implemented for require_auth, so that now we correctly check for overwritten scram keys on connection options. I think that the code comments should be correct now?
> > > If we implement this, it needs to check that the keys were actually > > > sent during scram_exchange(). Having them set on the PGconn doesn't > > > mean that we used them for authentication. > > > > > We use the client key and server key on calculate_client_proof and > > verify_server_signature respective during memcpy, it would be too hack > > to add new fields on pg_conn like scram_client_key_in_use and > > scram_server_key_in_use, set them to true on these functions and then > > validate that both are true on PQconnectionUsedScramKeys? > > I think that's probably a question for Peter: whether or not that > additional API is something we want to support. > Ok > > > > -dblink_security_check(PGconn *conn, remoteConn *rconn, const char > > > > *connstr) > > > > +dblink_security_check(PGconn *conn, remoteConn *rconn, > > > > + const char *connstr) > > > > > > nit: this whitespace change is not necessary now that > > > useScramPassthrough is no longer in the signature. > > > > > Fixed > > (This diff is still present in v6-0002.) > Sorry, I think that now is fixed. > > > Speaking of which, does get_connect_string() still need to take > > > user_mapping as an argument? > > > > > Yes, because we need to check if the use_scram_passthrough option is set > > on foreign server or user mapping options: > > if (MyProcPort->has_scram_keys && UseScramPassthrough(foreign_server, > > user_mapping)) > > appendSCRAMKeysInfo(&buf); > > I was referring to the discussion upthread [1]; you'd mentioned that > the only reason that get_connect_string() didn't call GetUserMapping() > itself was because we needed that mapping later on for > UseScramPassthrough(). But that's no longer true for this patch, > because the later call to UseScramPassthrough() has been removed. So I > think we can move GetUserMapping() back down, and remove that part of > the refactoring from patch 0001. > Ok, it totally makes sense. Fixed on v7. -- Matheus Alcantara
v7-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
v7-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
