Re: [PoC] Federated Authn/z with OAUTHBEARER

On Wed, Aug 25, 2021 at 3:25 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:

>
>
> On Wed, Aug 25, 2021 at 11:42 AM Jacob Champion <pchampion(at)vmware(dot)com>
> wrote:
>
>> On Tue, 2021-06-22 at 23:22 +0000, Jacob Champion wrote:
>> > On Fri, 2021-06-18 at 11:31 +0300, Heikki Linnakangas wrote:
>> > >
>> > > 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().
>> >
>> > Thanks, I'll fix that in the next re-roll.
>> >
>> > > 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'".
>> >
>> > I'll fix this too. Thanks!
>>
>> v2, attached, incorporates Heikki's suggested fixes and also rebases on
>> top of latest HEAD, which had the SASL refactoring changes committed
>> last month.
>>
>> The biggest change from the last patchset is 0001, an attempt at
>> enabling jsonapi in the frontend without the use of palloc(), based on
>> suggestions by Michael and Tom from last commitfest. I've also made
>> some improvements to the pytest suite. No major changes to the OAuth
>> implementation yet.
>>
>> --Jacob
>>
> Hi,
> For v2-0001-common-jsonapi-support-FRONTEND-clients.patch :
>
> + /* Clean up. */
> + termJsonLexContext(&lex);
>
> At the end of termJsonLexContext(), empty is copied to lex. For stack
> based JsonLexContext, the copy seems unnecessary.
> Maybe introduce a boolean parameter for termJsonLexContext() to signal
> that the copy can be omitted ?
>
> +#ifdef FRONTEND
> + /* make sure initialization succeeded */
> + if (lex->strval == NULL)
> + return JSON_OUT_OF_MEMORY;
>
> Should PQExpBufferBroken(lex->strval) be used for the check ?
>
> Thanks
>
Hi,
For v2-0002-libpq-add-OAUTHBEARER-SASL-mechanism.patch :

+ i_init_session(&session);
+
+ if (!conn->oauth_client_id)
+ {
+ /* We can't talk to a server without a client identifier. */
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("no oauth_client_id is set for
the connection"));
+ goto cleanup;

Can conn->oauth_client_id check be performed ahead of i_init_session() ?
That way, ```goto cleanup``` can be replaced with return.

+ if (!error_code || (strcmp(error_code, "authorization_pending")
+ && strcmp(error_code, "slow_down")))

What if, in the future, there is error code different from the above two
which doesn't represent "OAuth token retrieval failed" scenario ?

For client_initial_response(),

+ token_buf = createPQExpBuffer();
+ if (!token_buf)
+ goto cleanup;

If token_buf is NULL, there doesn't seem to be anything to free. We can
return directly.

Cheers