Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Andrey Chudnovsky <achudnovskij(at)gmail(dot)com>
To: Jacob Champion <jchampion(at)timescale(dot)com>
Cc: mahendrakar s <mahendrakarforpg(at)gmail(dot)com>, "hlinnaka(at)iki(dot)fi" <hlinnaka(at)iki(dot)fi>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "smilingsamay(at)gmail(dot)com" <smilingsamay(at)gmail(dot)com>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Date: 2022-12-13 05:06:20
Message-ID: CACrwV55OMyHw7wu_-NyyHe1ysMK5_b7kYKBfkwxXhk0YB8-VPQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> The server hook is the right place to check the scopes, yes, but I think
> the DBA should be able to specify what those scopes are to begin with.
> The provider of the extension shouldn't be expected by the architecture
> to hardcode those decisions, even if Azure AD chooses to short-circuit
> that choice and provide magic instead.

Hardcode is definitely not expected, but customization for identity
provider specific, I think, should be allowed.
I can provide a couple of advanced use cases which happen in the cloud
deployments world, and require per-role management:
- Multi-tenant deployments, when root provider URL would be different
for different roles, based on which tenant they come from.
- Federation to multiple providers. Solutions like Amazon Cognito
which offer a layer of abstraction with several providers
transparently supported.

If your concern is extension not honoring the DBA configured values:
Would a server-side logic to prefer HBA value over extension-provided
resolve this concern?
We are definitely biased towards the cloud deployment scenarios, where
direct access to .hba files is usually not offered at all.
Let's find the middle ground here.

A separate reason for creating this pre-authentication hook is further
extensibility to support more metadata.
Specifically when we add support for OAUTH flows to libpq, server-side
extensions can help bridge the gap between the identity provider
implementation and OAUTH/OIDC specs.
For example, that could allow the Github extension to provide an OIDC
discovery document.

I definitely see identity providers as institutional actors here which
can be given some power through the extension hooks to customize the
behavior within the framework.

> I maintain that the hook doesn't need to hand back artifacts to the
> client for a second PQconnect call. It can just use those artifacts to
> obtain the access token and hand that right back to libpq. (I think any
> requirement that clients be rewritten to call PQconnect twice will
> probably be a sticking point for adoption of an OAuth patch.)

Obtaining a token is an asynchronous process with a human in the loop.
Not sure if expecting a hook function to return a token synchronously
is the best option here.
Can that be an optional return value of the hook in cases when a token
can be obtained synchronously?

On Thu, Dec 8, 2022 at 4:41 PM Jacob Champion <jchampion(at)timescale(dot)com> wrote:
>
> On Wed, Dec 7, 2022 at 3:22 PM Andrey Chudnovsky
> <achudnovskij(at)gmail(dot)com> wrote:
> >
> >> I think it's okay to have the extension and HBA collaborate to
> >> provide discovery information. Your proposal goes further than
> >> that, though, and makes the server aware of the chosen client flow.
> >> That appears to be an architectural violation: why does an OAuth
> >> resource server need to know the client flow at all?
> >
> > Ok. It may have left there from intermediate iterations. We did
> > consider making extension drive the flow for specific grant_type,
> > but decided against that idea. For the same reason you point to. Is
> > it correct that your main concern about use of grant_type was that
> > it's propagated to the server? Then yes, we will remove sending it
> > to the server.
>
> Okay. Yes, that was my primary concern.
>
> >> Ideally, yes, but that only works if all identity providers
> >> implement the same flows in compatible ways. We're already seeing
> >> instances where that's not the case and we'll necessarily have to
> >> deal with that up front.
> >
> > Yes, based on our analysis OIDC spec is detailed enough, that
> > providers implementing that one, can be supported with generic code
> > in libpq / client. Github specifically won't fit there though.
> > Microsoft Azure AD, Google, Okta (including Auth0) will.
> > Theoretically discovery documents can be returned from the extension
> > (server-side) which is provider specific. Though we didn't plan to
> > prioritize that.
>
> As another example, Google's device authorization grant is incompatible
> with the spec (which they co-authored). I want to say I had problems
> with Azure AD not following that spec either, but I don't remember
> exactly what they were. I wouldn't be surprised to find more tiny
> departures once we get deeper into implementation.
>
> >> That seems to be restating the goal of OAuth and OIDC. Can you
> >> explain how the incompatible change allows you to accomplish this
> >> better than standard implementations?
> >
> > Do you refer to passing grant_type to the server? Which we will get
> > rid of in the next iteration. Or other incompatible changes as well?
>
> Just the grant type, yeah.
>
> >> Why? I claim that standard OAUTHBEARER can handle all of that.
> >> What does your proposed architecture (the third diagram) enable
> >> that my proposed hook (the second diagram) doesn't?
> >
> > The hook proposed on the 2nd diagram effectively delegates all Oauth
> > flows implementations to the client. We propose libpq takes care of
> > pulling OpenId discovery and coordination. Which is effectively
> > Diagram 1 + more flows + server hook providing root url/audience.
> >
> > Created the diagrams with all components for 3 flows: [snip]
>
> (I'll skip ahead to your later mail on this.)
>
> >> (For a future conversation: they need to set up authorization,
> >> too, with custom scopes or some other magic. It's not enough to
> >> check who the token belongs to; even if Postgres is just using the
> >> verified email from OpenID as an authenticator, you have to also
> >> know that the user authorized the token -- and therefore the client
> >> -- to access Postgres on their behalf.)
> >
> > My understanding is that metadata in the tokens is provider
> > specific, so server side hook would be the right place to handle
> > that. Plus I can envision for some providers it can make sense to
> > make a remote call to pull some information.
>
> The server hook is the right place to check the scopes, yes, but I think
> the DBA should be able to specify what those scopes are to begin with.
> The provider of the extension shouldn't be expected by the architecture
> to hardcode those decisions, even if Azure AD chooses to short-circuit
> that choice and provide magic instead.
>
> On 12/7/22 20:25, Andrey Chudnovsky wrote:
> > That being said, the Diagram 2 would look like this with our proposal:
> > [snip]
> >
> > With the application taking care of all Token acquisition logic. While
> > the server-side hook is participating in the pre-authentication reply.
> >
> > That is definitely a required scenario for the long term and the
> > easiest to implement in the client core.> And if we can do at least that flow in PG16 it will be a strong
> > foundation to provide more support for specific grants in libpq going
> > forward.
>
> Agreed.
> > Does the diagram above look good to you? We can then start cleaning up
> > the patch to get that in first.
>
> I maintain that the hook doesn't need to hand back artifacts to the
> client for a second PQconnect call. It can just use those artifacts to
> obtain the access token and hand that right back to libpq. (I think any
> requirement that clients be rewritten to call PQconnect twice will
> probably be a sticking point for adoption of an OAuth patch.)
>
> That said, now that your proposal is also compatible with OAUTHBEARER, I
> can pony up some code to hopefully prove my point. (I don't know if I'll
> be able to do that by the holidays though.)
>
> Thanks!
> --Jacob

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2022-12-13 05:45:00 Re: Force streaming every change in logical decoding
Previous Message Michael Paquier 2022-12-13 04:57:22 Remove SHA256_HMAC_B from scram-common.h