Re: Proposal: Support custom authentication methods using hooks - Mailing list pgsql-hackers

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > > I also don’t think that I agree that it’s acceptable to only have the
> > > > ability to extend the authentication on the server side as that implies a
> > > > whole bunch of client-side work that goes above and beyond just
> > > > implementing an authentication system if it’s not possible to leverage
> > > > libpq (which nearly all languages out there use..).  Not addressing that
> > > > side of it also makes me concerned that whatever we do here won’t be
> > > > right/enough/etc.
> > >
> > > You're skipping over my point of everything that can be made to use
> > > SASL-SCRAM-256 just working with the existing libpq? Again?
>
> > The plan is to make scram pluggable on the client side?  That isn’t what’s
> > been proposed here that I’ve seen.
>
> Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
> implementing an authentication method that wants to store secrets outside of
> postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
> don't need to make scram pluggable client-side. From the perspective of the
> client it'd look *exactly* like the normal auth flow with the server.

Then the only way to get support for something like, say, OAUTH, is to
modify the core code.

That strikes me as entirely defeating the point of having this be
extensible, since you still have to modify the core code to get support
for it.

> What's proposed here is not really about adding new authentication protocols
> between FE/BE (although some *limited* forms of that might be possible). It's
> primarily about using the existing FE/BE authentication exchanges
> (AUTH_REQ_{PASSWORD,SASL*,...}) to validate against something other than the
> builtin pg_authid.rolpassword.  Because drivers already know those
> authentication exchanges, it doesn't need to learn new tricks.

The OAUTH patch was specifically changed to try and use this and yet it
still had to modify the core code on the libpq side (at least, maybe
other things or maybe not depending on later changes to this patch).
I don't get why such an exercise would have been done if the goal is to
just allow what's in rolpassword to be pulled from somewhere else or why
we would be talking about different auth methods in the first place if
that's the goal here.  That the patch was also brought up in the context
of wanting to add support for another auth method also doesn't seem to
support the argument that this is just about changing where the value in
rolpassword comes from.

> As I said in
> https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
> when describing the above, that's not enough to implement every desireable
> authentication method - but there's real use-cases where using the existing
> SASL-SCRAM-256 is sufficient.

That this apparently isn't actually about adding new authentication
protocols or methods sure strikes me as odd when folks were adjusting
proposed patches specificially to use this for new authentication
methods and the subject is "Proposal: Support custom authentication
methods using hooks".  If what's happening is still actually SCRAM then
I also don't get why we would change pg_hba.conf to say 'custom' instead
of scram-sha-256 w/ some option to say how to go get the actual token,
or why there isn't anything done to deal with passwords being changed.
Basically, I don't agree with more-or-less anything that the patch is
doing if that's the goal.  If the goal is to actually change where the
token in rolpassword comes from for existing authentication methods and
specifically isn't to actually try to use this to support new
authenitcation methods, then I'd suggest laying it out that way and
having it be an option for scram-sha-256 or whatever other methods
(though that seems like the only one that should really be changed in
this way, if you want my 2c, as the rest that would work this way are
basically broken, that being md5) and then make sure to have a way to
handle password changes too.

> > I also wasn’t aware that we felt that it’s acceptable to just ignore other
> > committers.
>
> I'm not talking about going ahead and committing. Just not continuing
> discussing this topci and spending my time more fruitfully on something else.

I'm still a -1 on this patch.  Maybe there's a way to get a shared
storage for rolpassword and maybe that could even be extensible if we
want it to be (though I'd be more inclined to just build it in...) and
if that's actually the goal but this doesn't seem like the right
approach to be taking.  I also don't think that it's a good idea to have
a synchronous call-out during authentication as that can get really
painful and if you're cacheing it to deal with the risks of that, well,
seems like you'd be better off just using rolpassword (and perhaps with
Joshua's patches to allow more than one of those to exist at a time).

Thanks,

Stephen