Message-ID: <f40261a5-2efb-7851-fc63-7a5b6e00c146@timescale.com>
Date: Wed, 23 Nov 2022 12:05:37 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
To: mahendrakar s <mahendrakarforpg@gmail.com>,
 Andrey Chudnovsky <achudnovskij@gmail.com>
Cc: "hlinnaka@iki.fi" <hlinnaka@iki.fi>,
 "michael@paquier.xyz" <michael@paquier.xyz>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 "smilingsamay@gmail.com" <smilingsamay@gmail.com>
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
 <e866a4a3-0ba6-c7ae-3abc-aaff0c033558@iki.fi>
 <2f06a4aef1d5defe4b7aaec01478572e5557d32a.camel@vmware.com>
 <8a5a35b31c3f25f6b047e77def0445a60399981d.camel@vmware.com>
 <0e86fe7e3534fb05db02379fed404a2010eb1d62.camel@vmware.com>
 <4db7d3ea061f47a15fc6d865f53dd327ea2975fb.camel@vmware.com>
 <CAAWbhmiWudPQk2euOQQPPa=o14zCN9U_qLwU1pShKO4A-F9yeA@mail.gmail.com>
 <CACrwV540h9+qgz85aND7jpCcuxD_TOo+fJEg7xs1JKGLMf1N+A@mail.gmail.com>
 <CAAWbhmjiQ-X9w5CqLv__Wcfvg3oJ+hXd9807Lzi=zN9DMTpfFg@mail.gmail.com>
 <CACrwV54_euYe+v7bcLrxnje-JuM=KRX5azOcmmrXJ5qrffVZfg@mail.gmail.com>
 <CAAWbhmiR9D=OG=r9-bkiNw_Sp84qc7ti0PaWA_GWUM_t1PeS=g@mail.gmail.com>
 <CACrwV56g7ZZ9wfbQxcoX5YRdx6kx_HgJjxAM5PQoBV41jpCERw@mail.gmail.com>
 <CABkiuWqp1mog7GiUq+thfn0bqKye=QShe5aVBEVSUDPYyBWKvw@mail.gmail.com>
Content-Language: en-US
From: Jacob Champion <jchampion@timescale.com>
In-Reply-To: 
 <CABkiuWqp1mog7GiUq+thfn0bqKye=QShe5aVBEVSUDPYyBWKvw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/f40261a5-2efb-7851-fc63-7a5b6e00c146%40timescale.com>
Precedence: bulk

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