Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Jacob Champion <jacob.champion@enterprisedb.com>

View thread
From: Jacob Champion <jacob.champion@enterprisedb.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Shlok Kyal <shlok.kyal.oss@gmail.com>, mahendrakar s <mahendrakarforpg@gmail.com>, Andrey Chudnovsky <achudnovskij@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, "hlinnaka@iki.fi" <hlinnaka@iki.fi>, "michael@paquier.xyz" <michael@paquier.xyz>, "smilingsamay@gmail.com" <smilingsamay@gmail.com>, Stephen Frost <sfrost@snowman.net>
Date: 2024-03-01T00:04:05Z
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, Feb 28, 2024 at 9:40 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> +#define ALLOC(size) malloc(size)
> I wonder if we should use pg_malloc_extended(size, MCXT_ALLOC_NO_OOM) instead
> to self document the code.  We clearly don't want feature-parity with server-
> side palloc here.  I know we use malloc in similar ALLOC macros so it's not
> unique in that regard, but maybe?

I have a vague recollection that linking fe_memutils into libpq
tripped the exit() checks, but I can try again and see.

> +#ifdef FRONTEND
> +               destroyPQExpBuffer(lex->errormsg);
> +#else
> +               pfree(lex->errormsg->data);
> +               pfree(lex->errormsg);
> +#endif
> Wouldn't it be nicer if we abstracted this into a destroyStrVal function to a)
> avoid the ifdefs and b) make it more like the rest of the new API?  While it's
> only used in two places (close to each other) it's a shame to let the
> underlying API bleed through the abstraction.

Good idea. I'll fold this from your patch into the next set (and do
the same for the ones I've marked +1 below).

> +   CURLM      *curlm;      /* top-level multi handle for cURL operations */
> Nitpick, but curl is not capitalized cURL anymore (for some value of "anymore"
> since it changed in 2016 [0]).  I do wonder if we should consistently write
> "libcurl" as well since we don't use curl but libcurl.

Huh, I missed that memo. Thanks -- that makes it much easier to type!

> +   PQExpBufferData     work_data;  /* scratch buffer for general use (remember
> +                                      to clear out prior contents first!) */
> This seems like asking for subtle bugs due to uncleared buffers bleeding into
> another operation (especially since we are writing this data across the wire).
> How about having an array the size of OAuthStep of unallocated buffers where
> each step use it's own?  Storing the content of each step could also be useful
> for debugging.  Looking at the statemachine here it's not an obvious change but
> also not impossible.

I like that idea; I'll give it a look.

> + * TODO: This disables DNS resolution timeouts unless libcurl has been
> + * compiled against alternative resolution support. We should check that.
> curl_version_info() can be used to check for c-ares support.

+1

> + * so you don't have to write out the error handling every time. They assume
> + * that they're embedded in a function returning bool, however.
> It feels a bit iffy to encode the returntype in the macro, we can use the same
> trick that DISABLE_SIGPIPE employs where a failaction is passed in.

+1

> +  if (!strcmp(name, field->name))
> Project style is to test for (strcmp(x,y) == 0) rather than (!strcmp()) to
> improve readability.

+1

> +  libpq_append_conn_error(conn, "out of memory");
> While not introduced in this patch, it's not an ideal pattern to report "out of
> memory" errors via a function which may allocate memory.

Does trying (and failing) to allocate more memory cause any harm? Best
case, we still have enough room in the errorMessage to fit the whole
error; worst case, we mark the errorMessage broken and then
PQerrorMessage() can handle it correctly.

> +  appendPQExpBufferStr(&conn->errorMessage,
> +           libpq_gettext("server's error message contained an embedded NULL"));
> We should maybe add ", discarding" or something similar after this string to
> indicate that there was an actual error which has been thrown away, the error
> wasn't that the server passed an embedded NULL.

+1

> +#ifdef USE_OAUTH
> +       else if (strcmp(mechanism_buf.data, OAUTHBEARER_NAME) == 0 &&
> +               !selected_mechanism)
> I wonder if we instead should move the guards inside the statement and error
> out with "not built with OAuth support" or something similar like how we do
> with TLS and other optional components?

This one seems like a step backwards. IIUC, the client can currently
handle a situation where the server returns multiple mechanisms
(though the server doesn't support that yet), and I'd really like to
make use of that property without making users upgrade libpq.

That said, it'd be good to have a more specific error message in the
case where we don't have a match...

> +   errdetail("Comma expected, but found character %s.",
> +             sanitize_char(*p))));
> The %s formatter should be wrapped like '%s' to indicate that the message part
> is the character in question (and we can then reuse the translation since the
> error message already exist for SCRAM).

+1

> +       temp = curl_slist_append(temp, "authorization_code");
> +       if (!temp)
> +           oom = true;
> +
> +       temp = curl_slist_append(temp, "implicit");
> While not a bug per se, it reads a bit odd to call another operation that can
> allocate memory when the oom flag has been set.  I think we can move some
> things around a little to make it clearer.

I'm not a huge fan of nested happy paths/pyramids of doom, but in this
case it's small enough that I'm not opposed. :D

> The attached diff contains some (most?) of the above as a patch on top of your
> v17, but as a .txt to keep the CFBot from munging on it.

Thanks very much! I plan to apply all but the USE_OAUTH guard change
(but let me know if you feel strongly about it).

--Jacob