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

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> 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 disagree that it's actually at all useful when considering this
change, specifically because it doesn't move the actual goalposts at
all when it comes to adding support for new authentication methods for
PG overall as the core code still has to be modified through the regular
process.  I get that the idea of this patch isn't to add oauth, but I
don't think it's sensible to claim that the changes to the oauth patch
to use these hooks on just the server side while still having a bunch of
code in libpq for oauth makes this set of hooks make sense.  I
definitely don't think we should ever even consider adding support for
something on the libpq side which require a 'custom' auth module on the
server side that isn't included in core.  Putting it into contrib would
just ultimately make it really painful for users to deal with without
any benefit that I can see either as we'd still have to maintain it and
update it through the regular PG process.  Trying hard to give the
benefit of the doubt here, maybe you could argue that by having the
module in contrib and therefore not loaded unless requested that the
system might be overall 'more secure', but given that we already require
admins to specify what the auth method is with pg_hba and that hasn't
been a source of a lot of CVEs around somehow getting the wrong code in
there, I just don't see it as a sensible justfication.

> 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 it wasn't clear just what the idea of this patch was strikes me as
another point against it, really.  The thread talked about custom
authentication methods and the modification to pg_hba clearly made it
out to be a top-level alternative to SCRAM and the other methods even
though it sounds like that wasn't the intent, or maybe was but was only
part of it (but then, what's the other part..?  That's still unclear to
me even after reading these latest emails..).

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

But.. a patch was specifically proposed on this thread to use this to
add oauth, with encouragment from the folks working on this patch, and
that was then used, even just above, as a reason why this was a useful
change to make, but I don't see what about it makes it such.

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

The thing is though that the ones that do are the older ones that
generally aren't all that secure..  SCRAM is alright, RADIUS isn't
great unless it's a OTP kind of thing (and even then it's not great),
but md5, password, pam (tho it doesn't even actually use sendAuthRequest
and recv_password_packet), bsd, ldap, etc aren't.  The more secure
methods are the ones like gssapi, sspi, certificate and the layered ones
on top of those, which aren't that simple.  This is also the direction
that things are going in and why I dislike the idea of trying to extend
it in this way with the 'simple' exchange.  I'd also say that if you
really want a 'simple' exchange with a centralized service, you could
already just use RADIUS.

Also ... even md5 and SCRAM aren't actually just a simple exchange
as there's computation and back-and-forth between the client and the
server that's specific to those.  That there's a need for client-side
code that's specific to the auth method which goes to my point above
that this isn't just a server-side thing.

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

I'm really not following what this is saying.  If the client-side is
doing SCRAM but the server-side is doing 'custom', how is that going to
actually work unless what's actually happening is... just SCRAM?  If
only the server-side is 'custom', while the client thinks it's doing
SCRAM, then the client is going to:

Send to server (at least): username + random nonce

Get ... something back from the server, but needs to be a nonce, a salt,
and an iteration count

Calculate the SaltedPW from the password + iteration from server

Calculate Auth as client-first msg +,+ server-first msg +,+ client-final

Calculate the ClientProof using ClientKey XOR HMAC(H(ClientKey), Auth)

Send to server client-final and ClientProof

Get back ... something? from the server, but it had better match up with
what the client calculates as the right answer

Calculate the ServerSignature based on what the server sent and compare

I don't really follow how that would actually work if the whole thing
isn't just SCRAM, and if that's what it is, then why do the whole
'custom' thing at all instead of just providing a way for someone to
centralize rolpassword?

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

I don't get what is being implied here as the distinction between the
two.  I see how maybe you could say that RADIUS and password are
"methods" and not "protocols" because they're simple and just "send what
the user provided to the server" and "let the server tell you if you're
authorized or not" but that's the kind of thing we should really be
moving away from because they're just not secure.

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

But, even with one of your examples, peer, there has to be client-side
code that knows that's what is going on and checking things to make sure
it's a secure connection.  Both sides should be be calling getpeereid(),
as we support with requirepeer on the client-side and pg_ident map on
the server-side, so that the client and the server are verifying each
other.  That PAM doesn't support a way to do that is really a strike
against it, not something that we should be modeling or building on top
of.

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

Responded to this above ..

> >, 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 don't generally have an issue with the idea of having an external
username translation but I don't think this is the way to go about
having that.

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

I still have doubts about what exactly the patch is trying to do.  When
I thought that the idea was to have a centralized rolpassword, suddenly
things made sense, but then it was argued that it was trying to do more
than that and I'm left unclear as to what that 'more' was if it's only
to be something that's done on the server side.

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

We have code to reach out to an external LDAP server already, we could
certainly add an option to the scram auth method that says "go get the
value for this from this LDAP server", or we could add support for
reaching out to another PG server and running a query to get the value,
or add support for some other protocol that makes sense to query such
information out of, like a KMS.  None of those seem hard, and we have
support for LDAP and libpq already too.

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

We continue to have too much stuff in extensions imv, but that's a whole
other discussion.

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

Not sure which is being referred to here but also not sure that it's
terribly germane to this particular thread or patch.

Thanks,

Stephen