Message-ID: <fa8b5b3a-9f09-b402-20cb-92c35fe24d9f@timescale.com>
Date: Tue, 20 Sep 2022 16:19:31 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
From: Jacob Champion <jchampion@timescale.com>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
To: mahendrakar s <mahendrakarforpg@gmail.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 "smilingsamay@gmail.com" <smilingsamay@gmail.com>,
 "andres@anarazel.de" <andres@anarazel.de>, anchudno@microsoft.com,
 mahendrakars@microsoft.com
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
 <e866a4a3-0ba6-c7ae-3abc-aaff0c033558@iki.fi>
 <2f06a4aef1d5defe4b7aaec01478572e5557d32a.camel@vmware.com>
 <8a5a35b31c3f25f6b047e77def0445a60399981d.camel@vmware.com>
 <CABkiuWrg7eiP=g9pqG9Y3w5SZuj4i=Kmsak6+ex-13xppHaBiA@mail.gmail.com>
Content-Language: en-US
In-Reply-To: 
 <CABkiuWrg7eiP=g9pqG9Y3w5SZuj4i=Kmsak6+ex-13xppHaBiA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/fa8b5b3a-9f09-b402-20cb-92c35fe24d9f%40timescale.com>
Precedence: bulk

Hi Mahendrakar, thanks for your interest and for the patch!

On Mon, Sep 19, 2022 at 10:03 PM mahendrakar s
<mahendrakarforpg@gmail.com> wrote:
> The changes for each component are summarized below.
>
> 1.     Provider-specific extension:
>         Each OAuth provider implements their own token validator as an
> extension. Extension registers an OAuth provider hook which is matched
> to a line in the HBA file.

How easy is it to write a Bearer validator using C? My limited
understanding was that most providers were publishing libraries in
higher-level languages.

Along those lines, sample validators will need to be provided, both to
help in review and to get the pytest suite green again. (And coverage
for the new code is important, too.)

> 2.     Add support to pass on the OAuth bearer token. In this
> obtaining the bearer token is left to 3rd party application or user.
>
>         ./psql -U <username> -d 'dbname=postgres
> oauth_client_id=<client_id> oauth_bearer_token=<token>

This hurts, but I think people are definitely going to ask for it, given
the frightening practice of copy-pasting these (incredibly sensitive
secret) tokens all over the place... Ideally I'd like to implement
sender constraints for the Bearer token, to *prevent* copy-pasting (or,
you know, outright theft). But I'm not sure that sender constraints are
well-implemented yet for the major providers.

> 3.     HBA: An additional param ‘provider’ is added for the oauth method.
>         Defining "oauth" as method + passing provider, issuer endpoint
> and expected audience
>
>         * * * * oauth   provider=<token validation extension>
> issuer=.... scope=....

Naming aside (this conflicts with Samay's previous proposal, I think), I
have concerns about the implementation. There's this code:

> +		if (oauth_provider && oauth_provider->name)
> +		{
> +			ereport(ERROR,
> +				(errmsg("OAuth provider \"%s\" is already loaded.",
> +					oauth_provider->name)));
> +		}

which appears to prevent loading more than one global provider. But
there's also code that deals with a provider list? (Again, it'd help to
have test code covering the new stuff.)

>            b)      libpq optionally compiled for the clients which
> explicitly need libpq to orchestrate OAuth communication with the
> issuer (it depends heavily on 3rd party library iddawc as Jacob
> already pointed out. The library seems to be supporting all the OAuth
> flows.)

Speaking of iddawc, I don't think it's a dependency we should choose to
rely on. For all the code that it has, it doesn't seem to provide
compatibility with several real-world providers.

Google, for one, chose not to follow the IETF spec it helped author, and
iddawc doesn't support its flavor of Device Authorization. At another
point, I think iddawc tried to decode Azure's Bearer tokens, which is
incorrect...

I haven't been able to check if those problems have been fixed in a
recent version, but if we're going to tie ourselves to a huge
dependency, I'd at least like to believe that said dependency is
battle-tested and solid, and personally I don't feel like iddawc is.

> -	auth_method = I_TOKEN_AUTH_METHOD_NONE;
> -	if (conn->oauth_client_secret && *conn->oauth_client_secret)
> -		auth_method = I_TOKEN_AUTH_METHOD_SECRET_BASIC;

This code got moved, but I'm not sure why? It doesn't appear to have
made a change to the logic.

> +	if (conn->oauth_client_secret && *conn->oauth_client_secret)
> +	{
> +		session_response_type = I_RESPONSE_TYPE_CLIENT_CREDENTIALS;
> +	}

Is this an Azure-specific requirement? Ideally a public client (which
psql is) shouldn't have to provide a secret to begin with, if I
understand that bit of the protocol correctly. I think Google also
required provider-specific changes in this part of the code, and
unfortunately I don't think they looked the same as yours.

We'll have to figure all that out... Standards are great; everyone has
one of their own. :)

Thanks,
--Jacob