Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
To: Jacob Champion <pchampion@vmware.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <e866a4a3-0ba6-c7ae-3abc-aaff0c033558@iki.fi>
Date: Fri, 18 Jun 2021 11:31:00 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/e866a4a3-0ba6-c7ae-3abc-aaff0c033558%40iki.fi>
Precedence: bulk

On 08/06/2021 19:37, Jacob Champion wrote:
> We've been working on ways to expand the list of third-party auth
> methods that Postgres provides. Some example use cases might be "I want
> to let anyone with a Google account read this table" or "let anyone who
> belongs to this GitHub organization connect as a superuser".

Cool!

> The iddawc dependency for client-side OAuth was extremely helpful to
> develop this proof of concept quickly, but I don't think it would be an
> appropriate component to build a real feature on. It's extremely
> heavyweight -- it incorporates a huge stack of dependencies, including
> a logging framework and a web server, to implement features we would
> probably never use -- and it's fairly difficult to debug in practice.
> If a device authorization flow were the only thing that libpq needed to
> support natively, I think we should just depend on a widely used HTTP
> client, like libcurl or neon, and implement the minimum spec directly
> against the existing test suite.

You could punt and let the application implement that stuff. I'm 
imagining that the application code would look something like this:

conn = PQconnectStartParams(...);
for (;;)
{
     status = PQconnectPoll(conn)
     switch (status)
     {
         case CONNECTION_SASL_TOKEN_REQUIRED:
             /* open a browser for the user, get token */
             token = open_browser()
             PQauthResponse(token);
             break;
         ...
     }
}

It would be nice to have a simple default implementation, though, for 
psql and all the other client applications that come with PostgreSQL itself.

> If you've read this far, thank you for your interest, and I hope you
> enjoy playing with it!

A few small things caught my eye in the backend oauth_exchange function:

> +       /* Handle the client's initial message. */
> +       p = strdup(input);

this strdup() should be pstrdup().

In the same function, there are a bunch of reports like this:

>                    ereport(ERROR,
> +                          (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                           errmsg("malformed OAUTHBEARER message"),
> +                           errdetail("Comma expected, but found character \"%s\".",
> +                                     sanitize_char(*p))));

I don't think the double quotes are needed here, because sanitize_char 
will return quotes if it's a single character. So it would end up 
looking like this: ... found character "'x'".

- Heikki