Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Daniel Gustafsson <daniel@yesql.se>

View thread
From: Daniel Gustafsson <daniel@yesql.se>
To: Jacob Champion <jacob.champion@enterprisedb.com>
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>, Andrew Dunstan <andrew@dunslane.net>
Date: 2024-03-28T22:34:02Z
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

Attachments

> On 22 Mar 2024, at 19:21, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> v21 is a quick rebase over HEAD, which has adopted a few pieces of
> v20. I've also fixed a race condition in the tests.

Thanks for the rebase, I have a few comments from working with it a bit:

In jsonapi.c, makeJsonLexContextCstringLen initialize a JsonLexContext with
palloc0 which would need to be ported over to use ALLOC for frontend code.  On
that note, the errorhandling in parse_oauth_json() for content-type checks
attempts to free the JsonLexContext even before it has been created.  Here we
can just return false.


-  echo 'libpq must not be calling any function which invokes exit'; exit 1; \
+  echo 'libpq must not be calling any function which invokes exit'; \
The offending codepath in libcurl was in the NTLM_WB module, a very old and
obscure form of NTLM support which was replaced (yet remained in the tree) a
long time ago by a full NTLM implementatin.  Based on the findings in this
thread it was deprecated with a removal date set to April 2024 [0].  A bug in
the 8.4.0 release however disconnected NTLM_WB from the build and given the
lack of complaints it was decided to leave as is, so we can base our libcurl
requirements on 8.4.0 while keeping the exit() check intact.


+  else if (strcasecmp(content_type, "application/json") != 0)
This needs to handle parameters as well since it will now fail if the charset
parameter is appended (which undoubtedly will be pretty common).  The easiest
way is probably to just verify the mediatype and skip the parameters since we
know it can only be charset?


+  /* TODO investigate using conn->Pfdebug and CURLOPT_DEBUGFUNCTION here */
+  CHECK_SETOPT(actx, CURLOPT_VERBOSE, 1L, return false);
+  CHECK_SETOPT(actx, CURLOPT_ERRORBUFFER, actx->curl_err, return false);
CURLOPT_ERRORBUFFER is the old and finicky way of extracting error messages, we
should absolutely move to using CURLOPT_DEBUGFUNCTION instead.


+  /* && response_code != 401 TODO */ )
Why is this marked with a TODO, do you remember?


+  print("# OAuth provider (PID $pid) is listening on port $port\n");
Code running under Test::More need to use diag() for printing non-test output
like this.


Another issue I have is the sheer size and the fact that so much code is
replaced by subsequent commits, so I took the liberty to squash some of this
down into something less daunting.  The attached v22 retains the 0001 and then
condenses the rest into two commits for frontent and backend parts.  I did drop
the Python pytest patch since I feel that it's unlikely to go in from this
thread (adding pytest seems worthy of its own thread and discussion), and the
weight of it makes this seem scarier than it is.  For using it, it can be
easily applied from the v21 patchset independently.  I did tweak the commit
message to match reality a bit better, but there is a lot of work left there.

The final patch contains fixes for all of the above review comments as well as
a some refactoring, smaller clean-ups and TODO fixing.  If these fixes are
accepted I'll incorporate them into the two commits.

Next I intend to work on writing documentation for this.

--
Daniel Gustafsson

[0] https://curl.se/dev/deprecate.html
[1] https://github.com/curl/curl/pull/12479