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-26T16:20:17Z
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 Thu, Aug 26, 2021 at 9:13 AM Jacob Champion <pchampion@vmware.com> wrote:

> On Wed, 2021-08-25 at 15:25 -0700, Zhihong Yu wrote:
> >
> > 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 ?
>
> Do you mean heap-based? i.e. destroyJsonLexContext() does an
> unnecessary copy before free? Yeah, in that case it's not super useful,
> but I think I'd want some evidence that the performance hit matters
> before optimizing it.
>
> Are there any other internal APIs that take a boolean parameter like
> that? If not, I think we'd probably just want to remove the copy
> entirely if it's a problem.
>
> > +#ifdef FRONTEND
> > +       /* make sure initialization succeeded */
> > +       if (lex->strval == NULL)
> > +           return JSON_OUT_OF_MEMORY;
> >
> > Should PQExpBufferBroken(lex->strval) be used for the check ?
>
> It should be okay to continue if the strval is broken but non-NULL,
> since it's about to be reset. That has the fringe benefit of allowing
> the function to go as far as possible without failing, though that's
> probably a pretty weak justification.
>
> In practice, do you think that the probability of success is low enough
> that we should just short-circuit and be done with it?
>
> On Wed, 2021-08-25 at 16:24 -0700, Zhihong Yu wrote:
> >
> > 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.
>
> Yeah, I think that makes sense. FYI, this is probably one of the
> functions that will be rewritten completely once iddawc is removed.
>
> > +       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 ?
>
> We'd have to update our code; that would be a breaking change to the
> Device Authorization spec. Here's what it says today [1]:
>
>    The "authorization_pending" and "slow_down" error codes define
>    particularly unique behavior, as they indicate that the OAuth client
>    should continue to poll the token endpoint by repeating the token
>    request (implementing the precise behavior defined above).  If the
>    client receives an error response with any other error code, it MUST
>    stop polling and SHOULD react accordingly, for example, by displaying
>    an error to the user.
>
> > 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.
>
> That's true today, but implementations have a habit of changing. I
> personally prefer not to introduce too many exit points from a function
> that's already using goto. In my experience, that makes future
> maintenance harder.
>
> Thanks for the reviews! Have you been able to give the patchset a try
> with an OAuth deployment?
>
> --Jacob
>
> [1] https://datatracker.ietf.org/doc/html/rfc8628#section-3.5

Hi,
bq. destroyJsonLexContext() does an unnecessary copy before free? Yeah, in
that case it's not super useful,
but I think I'd want some evidence that the performance hit matters before
optimizing it.

Yes I agree.

bq. In practice, do you think that the probability of success is low enough
that we should just short-circuit and be done with it?

Haven't had a chance to try your patches out yet.
I will leave this to people who are more familiar with OAuth
implementation(s).

bq.  I personally prefer not to introduce too many exit points from a
function that's already using goto.

Fair enough.

Cheers