Re: [PoC] Federated Authn/z with OAUTHBEARER

> On 22 Mar 2024, at 19:21, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)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