Message-ID: <705e7eb8-29ee-a707-5a67-d2acfb2f3fad@timescale.com>
Date: Thu, 8 Dec 2022 16:41:21 -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
Content-Language: en-US
To: Andrey Chudnovsky <achudnovskij@gmail.com>
Cc: mahendrakar s <mahendrakarforpg@gmail.com>,
 "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>
 <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>
 <f40261a5-2efb-7851-fc63-7a5b6e00c146@timescale.com>
 <CACrwV54pf-VcL_XmDSWOHtLhiprm9nxORW5LJXocD=yCVq=knQ@mail.gmail.com>
 <CABkiuWo-7KPMZ92Jii0FLz=VmQ=2UbnsgWUCqtfuLXGzubB=tg@mail.gmail.com>
 <a19dc65a-ccea-fd1b-5e0c-71b7aa8502f1@timescale.com>
 <CACrwV56MTdboeBKVU-b-M2oHLttRxoVm4tJmwVOK2cydL+Spdg@mail.gmail.com>
 <CAAWbhmgsFanAWst7xMW7hgzAcGEqsSjgP0vcFt=cUYGxJBWndQ@mail.gmail.com>
 <CACrwV57+4MT7Fru-YbqEOYWp9EiLgfasUtPhntPmsNd0uNS6oA@mail.gmail.com>
 <CACrwV57DcigMiEvuy=RJu2HH+euhyzgpeUuVAMu_rigY6R1P4w@mail.gmail.com>
From: Jacob Champion <jchampion@timescale.com>
In-Reply-To: 
 <CACrwV57DcigMiEvuy=RJu2HH+euhyzgpeUuVAMu_rigY6R1P4w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/705e7eb8-29ee-a707-5a67-d2acfb2f3fad%40timescale.com>
Precedence: bulk

On Wed, Dec 7, 2022 at 3:22 PM Andrey Chudnovsky
<achudnovskij@gmail.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