Re: [PoC] Federated Authn/z with OAUTHBEARER

Peter Eisentraut <peter@eisentraut.org>

From: Peter Eisentraut <peter@eisentraut.org>

To: Jacob Champion <jacob.champion@enterprisedb.com>

Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Daniel Gustafsson <daniel@yesql.se>

Date: 2024-08-30T09:49:43Z

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 →

meson: Fix install-quiet after clean
- a9ffb35274fb 18.0 landed
- 4ae03be54734 19 (unreleased) landed
oauth: Run Autoconf tests with correct compiler flags
- 3d23f68c5529 18.0 landed
- 990571a08b66 19 (unreleased) landed
Link libpq with libdl if the platform needs that.
- 4df477153a6b 19 (unreleased) landed
- 7bd752c1fb8e 18.0 landed
Doc: correct spelling of meson switch.
- 3faac9d14063 16.9 landed
- 766d2e673342 17.5 landed
- ac557793d478 18.0 landed
oauth: Correct SSL dependency for libpq-oauth.a
- 3db68212a393 18.0 landed
oauth: Fix Autoconf build on macOS
- 4ea1254f35b2 18.0 cited
oauth: Move the builtin flow into a separate module
- b0635bfda053 18.0 landed
Remove a stray "pgrminclude" annotation
- 764d501d24ba 18.0 cited
oauth: Simplify copy of PGoauthBearerRequest
- 1cf4c56480f8 18.0 landed
oauth: Improve validator docs on interruptibility
- 873c0fd67872 18.0 landed
oauth: Disallow synchronous DNS in libcurl
- d7e40845f923 18.0 landed
oauth: Fix postcondition for set_timer on macOS
- 434dbf6907ec 18.0 landed
oauth: Use IPv4-only issuer in oauth_validator tests
- 8d9d5843b55f 18.0 landed
Work around OAuth/EVFILT_TIMER quirk on NetBSD.
- c301a0a74a8a 18.0 landed
oauth: Fix incorrect const markers in struct
- 03366b61dfe5 18.0 landed
Add missing entry to oauth_validator test .gitignore
- 2c53dec7f440 18.0 landed
cirrus: Temporarily fix libcurl link error
- 9d9a71002a1c 18.0 landed
Add support for OAUTHBEARER SASL mechanism
- b3f0be788afc 18.0 landed
libpq: Handle asynchronous actions during SASL
- a99a32e43ed7 18.0 landed
require_auth: prepare for multiple SASL mechanisms
- f8d8581ed882 18.0 landed
Move PG_MAX_AUTH_TOKEN_LENGTH to libpq/auth.h
- e21d6f297158 18.0 landed
Make SASL max message length configurable
- 6d16f9debae0 18.0 landed
jsonapi: fully initialize dummy lexer
- 41b023946dfd 18.0 landed
common/jsonapi: support libpq as a client
- 0785d1b8b2fa 18.0 landed
Remove fe_memutils from libpgcommon_shlib
- f1976df5eaf2 18.0 landed
Revert ECPG's use of pnstrdup()
- f0096ef13be2 13.17 landed
- 3557185538fe 14.14 landed
- 2de129b356bf 15.9 landed
- ee2997c678d8 16.5 landed
- e9e05c655069 17.0 landed
- 5388216f6adc 18.0 landed
Explicitly require password for SCRAM exchange
- adcdb2c8dda4 17.0 landed
Refactor SASL exchange to return tri-state status
- 24178e235ea5 17.0 landed

On 28.08.24 18:31, Jacob Champion wrote:
> On Mon, Aug 26, 2024 at 4:23 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> I was having trouble reasoning about the palloc-that-isn't-palloc code
>> during the first few drafts, so I will try a round with the jsonapi_
>> prefix.
> 
> v27 takes a stab at that. I have kept the ALLOC/FREE naming to match
> the strategy in other src/common source files.

This looks pretty good to me.  Maybe on the naming side, this seems like 
a gratuitous divergence:

+#define jsonapi_createStringInfo           makeStringInfo

> The name of the variable JSONAPI_USE_PQEXPBUFFER leads to sections of
> code that look like this:
> 
> +#ifdef JSONAPI_USE_PQEXPBUFFER
> +    if (!new_prediction || !new_fnames || !new_fnull)
> +        return false;
> +#endif
> 
> To me it wouldn't be immediately obvious why "using PQExpBuffer" has
> anything to do with this code; the key idea is that we expect any
> allocations to be able to fail. Maybe a name like JSONAPI_ALLOW_OOM or
> JSONAPI_SHLIB_ALLOCATIONS or...?

Seems ok to me as is.  I think the purpose of JSONAPI_USE_PQEXPBUFFER is 
adequately explained by this comment

+/*
+ * By default, we will use palloc/pfree along with StringInfo.  In libpq,
+ * use malloc and PQExpBuffer, and return JSON_OUT_OF_MEMORY on 
out-of-memory.
+ */
+#ifdef JSONAPI_USE_PQEXPBUFFER

For some of the other proposed names, I'd be afraid that someone might 
think you are free to mix and match APIs, OOM behavior, and compilation 
options.

Some comments on src/include/common/jsonapi.h:

-#include "lib/stringinfo.h"

I suspect this will fail headerscheck?  Probably needs an exception 
added there.

+#ifdef JSONAPI_USE_PQEXPBUFFER
+#define StrValType PQExpBufferData
+#else
+#define StrValType StringInfoData
+#endif

Maybe use jsonapi_StrValType here.

+typedef struct StrValType StrValType;

I don't think that is needed.  It would just duplicate typedefs that 
already exist elsewhere, depending on what StrValType is set to.

+       bool            parse_strval;
+       StrValType *strval;                     /* only used if 
parse_strval == true */

The parse_strval field could use a better explanation.

I actually don't understand the need for this field.  AFAICT, this is
just used to record whether strval is valid.  But in the cases where
it's not valid, why do we need to record that?  Couldn't you just return
failed_oom in those cases?