Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Zhihong Yu <zyu@yugabyte.com>

View thread
From: Zhihong Yu <zyu@yugabyte.com>
To: Jacob Champion <pchampion@vmware.com>
Cc: "hlinnaka@iki.fi" <hlinnaka@iki.fi>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "michael@paquier.xyz" <michael@paquier.xyz>
Date: 2021-08-25T23:24:19Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. meson: Fix install-quiet after clean

  2. oauth: Run Autoconf tests with correct compiler flags

  3. Link libpq with libdl if the platform needs that.

  4. Doc: correct spelling of meson switch.

  5. oauth: Correct SSL dependency for libpq-oauth.a

  6. oauth: Fix Autoconf build on macOS

  7. oauth: Move the builtin flow into a separate module

  8. Remove a stray "pgrminclude" annotation

  9. oauth: Simplify copy of PGoauthBearerRequest

  10. oauth: Improve validator docs on interruptibility

  11. oauth: Disallow synchronous DNS in libcurl

  12. oauth: Fix postcondition for set_timer on macOS

  13. oauth: Use IPv4-only issuer in oauth_validator tests

  14. Work around OAuth/EVFILT_TIMER quirk on NetBSD.

  15. oauth: Fix incorrect const markers in struct

  16. Add missing entry to oauth_validator test .gitignore

  17. cirrus: Temporarily fix libcurl link error

  18. Add support for OAUTHBEARER SASL mechanism

  19. libpq: Handle asynchronous actions during SASL

  20. require_auth: prepare for multiple SASL mechanisms

  21. Move PG_MAX_AUTH_TOKEN_LENGTH to libpq/auth.h

  22. Make SASL max message length configurable

  23. jsonapi: fully initialize dummy lexer

  24. common/jsonapi: support libpq as a client

  25. Remove fe_memutils from libpgcommon_shlib

  26. Revert ECPG's use of pnstrdup()

  27. Explicitly require password for SCRAM exchange

  28. Refactor SASL exchange to return tri-state status

On Wed, Aug 25, 2021 at 3:25 PM Zhihong Yu <zyu@yugabyte.com> wrote:

>
>
> On Wed, Aug 25, 2021 at 11:42 AM Jacob Champion <pchampion@vmware.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