Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

Hi Jacob,

I had validated Github by skipping the discovery mechanism and letting
the provider extension pass on the endpoints. This is just for
validation purposes.
If it needs to be supported, then need a way to send the discovery
document from extension.


Thanks,
Mahendrakar.

On Thu, 24 Nov 2022 at 09:16, Andrey Chudnovsky <achudnovskij@gmail.com> wrote:
>
> > How does this differ from the previous proposal? The OAUTHBEARER SASL
> > mechanism already relies on OIDC for discovery. (I think that decision
> > is confusing from an architectural and naming standpoint, but I don't
> > think they really had an alternative...)
> Mostly terminology questions here. OAUTHBEARER SASL appears to be the
> spec about using OAUTH2 tokens for Authentication.
> While any OAUTH2 can generally work, we propose to specifically
> highlight that only OIDC providers can be supported, as we need the
> discovery document.
> And we won't be able to support Github under that requirement.
> Since the original patch used that too - no change on that, just
> confirmation that we need OIDC compliance.
>
> > 0) The original hook proposal upthread, I thought, was about allowing
> > libpq's flow implementation to be switched out by the application. I
> > don't see that approach taken here. It's fine if that turned out to be a
> > bad idea, of course, but this patch doesn't seem to match what we were
> > talking about.
> We still plan to allow the client to pass the token. Which is a
> generic way to implement its own OAUTH flows.
>
> > 1) I'm really concerned about the sudden explosion of flows. We went
> > from one flow (Device Authorization) to six. It's going to be hard
> > enough to validate that *one* flow is useful and can be securely
> > deployed by end users; I don't think we're going to be able to maintain
> > six, especially in combination with my statement that iddawc is not an
> > appropriate dependency for us.
>
> > I'd much rather give applications the ability to use their own OAuth
> > code, and then maintain within libpq only the flows that are broadly
> > useful. This ties back to (0) above.
> We consider the following set of flows to be minimum required:
> - Client Credentials - For Service to Service scenarios.
> - Authorization Code with PKCE - For rich clients,including pgAdmin.
> - Device code - for psql (and possibly other non-GUI clients).
> - Refresh code (separate discussion)
> Which is pretty much the list described here:
> https://oauth.net/2/grant-types/ and in OAUTH2 specs.
> Client Credentials is very simple, so does Refresh Code.
> If you prefer to pick one of the richer flows, Authorization code for
> GUI scenarios is probably much more widely used.
> Plus it's easier to implement too, as interaction goes through a
> series of callbacks. No polling required.
>
> > 2) Breaking the refresh token into its own pseudoflow is, I think,
> > passing the buck onto the user for something that's incredibly security
> > sensitive. The refresh token is powerful; I don't really want it to be
> > printed anywhere, let alone copy-pasted by the user. Imagine the
> > phishing opportunities.
>
> > If we want to support refresh tokens, I believe we should be developing
> > a plan to cache and secure them within the client. They should be used
> > as an accelerator for other flows, not as their own flow.
> It's considered a separate "grant_type" in the specs / APIs.
> https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokens
>
> For the clients, it would be storing the token and using it to authenticate.
> On the question of sensitivity, secure credentials stores are
> different for each platform, with a lot of cloud offerings for this.
> pgAdmin, for example, has its own way to secure credentials to avoid
> asking users for passwords every time the app is opened.
> I believe we should delegate the refresh token management to the clients.
>
> >3) I don't like the departure from the OAUTHBEARER mechanism that's
> > presented here. For one, since I can't see a sample plugin that makes
> > use of the "flow type" magic numbers that have been added, I don't
> > really understand why the extension to the mechanism is necessary.
> I don't think it's much of a departure, but rather a separation of
> responsibilities between libpq and upstream clients.
> As libpq can be used in different apps, the client would need
> different types of flows/grants.
> I.e. those need to be provided to libpq at connection initialization
> or some other point.
> We will change to "grant_type" though and use string to be closer to the spec.
> What do you think is the best way for the client to signal which OAUTH
> flow should be used?
>
> On Wed, Nov 23, 2022 at 12:05 PM Jacob Champion <jchampion@timescale.com> wrote:
> >
> > On 11/23/22 01:58, mahendrakar s wrote:
> > > We validated on  libpq handling OAuth natively with different flows
> > > with different OIDC certified providers.
> > >
> > > Flows: Device Code, Client Credentials and Refresh Token.
> > > Providers: Microsoft, Google and Okta.
> >
> > Great, thank you!
> >
> > > Also validated with OAuth provider Github.
> >
> > (How did you get discovery working? I tried this and had to give up
> > eventually.)
> >
> > > We propose using OpenID Connect (OIDC) as the protocol, instead of
> > > OAuth, as it is:
> > > - Discovery mechanism to bridge the differences and provide metadata.
> > > - Stricter protocol and certification process to reliably identify
> > > which providers can be supported.
> > > - OIDC is designed for authentication, while the main purpose of OAUTH is to
> > > authorize applications on behalf of the user.
> >
> > How does this differ from the previous proposal? The OAUTHBEARER SASL
> > mechanism already relies on OIDC for discovery. (I think that decision
> > is confusing from an architectural and naming standpoint, but I don't
> > think they really had an alternative...)
> >
> > > Github is not OIDC certified, so won’t be supported with this proposal.
> > > However, it may be supported in the future through the ability for the
> > > extension to provide custom discovery document content.
> >
> > Right.
> >
> > > OpenID configuration has a well-known discovery mechanism
> > > for the provider configuration URI which is
> > > defined in OpenID Connect. It allows libpq to fetch
> > > metadata about provider (i.e endpoints, supported grants, response types, etc).
> >
> > Sure, but this is already how the original PoC works. The test suite
> > implements an OIDC provider, for instance. Is there something different
> > to this that I'm missing?
> >
> > > In the attached patch (based on V2 patch in the thread and does not
> > > contain Samay's changes):
> > > - Provider can configure issuer url and scope through the options hook.)
> > > - Server passes on an open discovery url and scope to libpq.
> > > - Libpq handles OAuth flow based on the flow_type sent in the
> > > connection string [1].
> > > - Added callbacks to notify a structure to client tools if OAuth flow
> > > requires user interaction.
> > > - Pg backend uses hooks to validate bearer token.
> >
> > Thank you for the sample!
> >
> > > Note that authentication code flow with PKCE for GUI clients is not
> > > implemented yet.
> > >
> > > Proposed next steps:
> > > - Broaden discussion to reach agreement on the approach.
> >
> > High-level thoughts on this particular patch (I assume you're not
> > looking for low-level implementation comments yet):
> >
> > 0) The original hook proposal upthread, I thought, was about allowing
> > libpq's flow implementation to be switched out by the application. I
> > don't see that approach taken here. It's fine if that turned out to be a
> > bad idea, of course, but this patch doesn't seem to match what we were
> > talking about.
> >
> > 1) I'm really concerned about the sudden explosion of flows. We went
> > from one flow (Device Authorization) to six. It's going to be hard
> > enough to validate that *one* flow is useful and can be securely
> > deployed by end users; I don't think we're going to be able to maintain
> > six, especially in combination with my statement that iddawc is not an
> > appropriate dependency for us.
> >
> > I'd much rather give applications the ability to use their own OAuth
> > code, and then maintain within libpq only the flows that are broadly
> > useful. This ties back to (0) above.
> >
> > 2) Breaking the refresh token into its own pseudoflow is, I think,
> > passing the buck onto the user for something that's incredibly security
> > sensitive. The refresh token is powerful; I don't really want it to be
> > printed anywhere, let alone copy-pasted by the user. Imagine the
> > phishing opportunities.
> >
> > If we want to support refresh tokens, I believe we should be developing
> > a plan to cache and secure them within the client. They should be used
> > as an accelerator for other flows, not as their own flow.
> >
> > 3) I don't like the departure from the OAUTHBEARER mechanism that's
> > presented here. For one, since I can't see a sample plugin that makes
> > use of the "flow type" magic numbers that have been added, I don't
> > really understand why the extension to the mechanism is necessary.
> >
> > For two, if we think OAUTHBEARER is insufficient, the people who wrote
> > it would probably like to hear about it. Claiming support for a spec,
> > and then implementing an extension without review from the people who
> > wrote the spec, is not something I'm personally interested in doing.
> >
> > 4) The test suite is still broken, so it's difficult to see these things
> > in practice for review purposes.
> >
> > > - Implement libpq changes without iddawc
> >
> > This in particular will be much easier with a functioning test suite,
> > and with a smaller number of flows.
> >
> > > - Prototype GUI flow with pgAdmin
> >
> > Cool!
> >
> > Thanks,
> > --Jacob