Re: dblink: Add SCRAM pass-through authentication

On Thu, Mar 13, 2025 at 2:38 PM Matheus Alcantara
<matheusssilv97(at)gmail(dot)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.

> > 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.

> > > -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.)

> > 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.

Thanks!
--Jacob

[1] https://postgr.es/m/CAFY6G8f%3DjRUAP5yiFRZkHmqstCiRkeeD5Zf2ixVf6HMmjBCgfg%40mail.gmail.com