Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Antonin Houska <ah@cybertec.at>

View thread
From: Antonin Houska <ah@cybertec.at>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2024-10-18T05:51:21Z
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

Antonin Houska <ah@cybertec.at> wrote:

> I'd like to play with the code a bit and provide some review before or during
> the next CF. That will probably generate some more questions.

This is the 1st round, based on reading the code. I'll continue paying
attention to the project and possibly post some more comments in the future.


* Information on the new method should be added to pg_hba.conf.sample.method.


* Is it important that fe_oauth_state.token also contains the "Bearer"
  keyword? I'd expect only the actual token value here. The keyword can be
  added to the authentication message w/o storing it.

  The same applies to the 'token' structure in fe-auth-oauth-curl.c.


* Does PQdefaultAuthDataHook() have to be declared extern and exported via
  libpq/exports.txt ? Even if the user was interested in it, he can use
  PQgetAuthDataHook() to get the pointer (unless he already installed his
  custom hook).


* I wonder if the hooks (PQauthDataHook) can be implemented in a separate
  diff. Couldn't the first version of the feature be commitable without these
  hooks?


* Instead of allocating an instance of PQoauthBearerRequest, assigning it to
  fe_oauth_state.async_ctx, and eventually having to all its cleanup()
  function, wouldn't it be simpler to embed PQoauthBearerRequest as a member
  in fe_oauth_state ?


* oauth_validator_library is defined as PGC_SIGHUP - is that intentional?

  And regardless, the library appears to be loaded by every backend during
  authentication. Why isn't it loaded by postmaster like libraries listed in
  shared_preload_libraries? fork() would then ensure that the backends do have
  the library in their address space.


* pg_fe_run_oauth_flow()

  When first time here
			case OAUTH_STEP_TOKEN_REQUEST:
				if (!handle_token_response(actx, &state->token))
					goto error_return;

  the user hasn't been prompted yet so ISTM that the first token request must
  always fail. It seems more logical if the prompt is set to the user before
  sending the token request to the server. (Although the user probably won't
  be that fast to make the first request succeed, so consider this just a
  hint.)


* As long as I understand, the following comment would make sense:

diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index f943a31cc08..97259fb5654 100644
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -518,6 +518,7 @@ oauth_exchange(void *opaq, bool final,
        switch (state->state)
        {
                case FE_OAUTH_INIT:
+                       /* Initial Client Response */
                        Assert(inputlen == -1);
 
                        if (!derive_discovery_uri(conn))

  Or, doesn't the FE_OAUTH_INIT branch of the switch statement actually fit
  better into oauth_init()? A side-effect of that might be (I only judge from
  reading the code, haven't tried to implement this suggestion) that
  oauth_exchange() would no longer return the SASL_ASYNC status. Furthermore,
  I'm not sure if pg_SASL_continue() can receive the SASL_ASYNC at all. So I
  wonder if moving that part from oauth_exchange() to oauth_init() would make
  the SASL_ASYNC state unnecessary.


* Finally, the user documentation is almost missing. I say that just for the
  sake of completeness, you obviously know it. (On the other hand, I think
  that the lack of user information might discourage some people from running
  the code and testing it.)

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com