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

Hi,

On 2022-03-18 15:23:21 -0400, Stephen Frost wrote:
> * 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.

It wasn't the goal of the patch to add oauth support. I think it's a good sign
that the patch allowed Jacob to move the server side code in an extension, but
it's a benefit not a requirement.

I think we might be able to build ontop of this to make things even more
extensible and it's worth having the discussion whether the proposal can be
made more extensible with a reasonable amount of complexity. But that's
different from faulting the patch for not achieving something it didn't set
out to do.


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

Since it wasn't the goal to add oauth...


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

It's called that way because auth.c calling it that. See

void
ClientAuthentication(Port *port)
...
    switch (port->hba->auth_method)

even though the different auth_methods use a smaller number of ways of
exchanges of secrets than we have "auth methods". So it doesn't at all seem
insane to name something allowing different authentication methods in the
sense of auth.c "custom authentication methods".



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

My point isn't that it's *just* the changing the value of rollpassword, but
that the authentication exchange with the client just uses the existing secret
exchanges, because they're sufficient for plenty use cases.


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

See above. Note also it was called "Support custom authentication methods" not
"Support custom authentication protocols".


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

Normally make extension APIs reasonably generic. If you want to argue against
that here, I think that's a debate worth having (as I explicitly said
upthread!), but we should be having it about that then.

It does seem good to me that the proposed API could implement stuff like peer,
bsd, PAM, etc without a problem.  And there's plenty other things that don't
require different authentication protocols on the pg protocol level - using os
specific extensions to safely get the remote user of tcp connections would be
cool for example.


> 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

s/methods/protocols/


>, 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 think that'd make it less useful, but still useful. E.g. doing different
external <-> internal username translation than what's built in.


> I'm still a -1 on this patch.

Fair enough, as long as you're doing it on what the patch actually tries to do
rather than something different.


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

How would you add external storage for secrets in a builtin way?


> 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

There's tradeoffs, for sure. But normally we don't prevent people from doing
stupid stuff with our extensibility (if anything we try to keep some debatable
stuff out of core by allowing it to be done in extensions).


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

That imo solves a separate problem.

Greetings,

Andres Freund