Re: [PoC] Federated Authn/z with OAUTHBEARER

> On 16 Oct 2024, at 19:21, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:

> v31 folds in the remainder of the review patch (hooray!) and makes a
> change to the 401 handling. If the server doesn't tell the client why
> a token request failed, but we see that we got a 401 Unauthorized, the
> error message now suggests that the oauth_client_secret setting is
> unacceptable.

+1

The attached diff expands the documentation a little to chip away at the
non-trivial task of documenting this feature. I will keep working on this part
as well reviewing the rest of the patchset. A few more comments are added as
well but nothing groundbreaking. I've attached your v31 vanilla as well to
keep the CFBot happy.

A few small comments on things in the attached review 0005 comment:

- (errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg("client selected an invalid SASL authentication mechanism")));
+ errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("client selected an invalid SASL authentication mechanism"));
Might be a nitpick but in new code I think we should use the new style of
ereport() calls without extra parens around the aux functions.

In validate() it seems to me we should clear out ret->authn_id on failure to
pair belts with suspenders. Fixed by calling explicit_bzero on it in the error
path.

parse_hba_line() didn't enforce mandatory parameters, and one could configure a
usermap and skipping of the usermap at the same time which seems nonsensical.

src/test/modules/oauth_validator didn't check for PG_TEST_EXTRA which is should
since it opens a server. We should also not test capabilities with if-elsif
since they are really separate things.

A few smaller bits and pieces of cleanup is also included.

--
Daniel Gustafsson