Re: dblink: Add SCRAM pass-through authentication

On Mon, Mar 17, 2025 at 1:49 PM Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> 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.
>
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