Re: [PoC] Federated Authn/z with OAUTHBEARER

On Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Assorted review comments from me:

Thank you! I will cherry-pick some responses here and plan to address
the rest in a future patchset.

> trust_validator_authz: Personally, I'm not a fan of the "authz" and
> "authn" abbreviations. I know this is security jargon. But are
> regular users going to understand this? Can we just spell it out?

Yes. That name's a holdover from the very first draft, actually.

Is "trust_validator_authorization" a great name in the first place?
The key concept is that user mapping is being delegated to the OAuth
system itself, so you'd better make sure that the validator has been
built to do that. (Anyone have any suggestions?)

> I find the way the installation options are structured a bit odd. I
> would have expected --with-libcurl and -Dlibcurl (or --with-curl and
> -Dcurl). These build options usually just say, use this library.

It's patterned directly off of -Dssl/--with-ssl (which I liberally
borrowed from) because the builtin client implementation used to have
multiple options for the library in use. I can change it if needed,
but I thought it'd be helpful for future devs if I didn't undo the
generalization.

> Maybe oauth_issuer should be oauth_issuer_url? Otherwise one might
> expect to just write "google" here or something. Or there might be
> other ways to contact an issuer in the future? Just a thought.

More specifically this is an "issuer identifier", as defined by the
OAuth/OpenID discovery specs. It's a subset of a URL, and I want to
make sure users know how to differentiate between an "issuer" they
trust and the "discovery URI" that's in use for that issuer. They may
want to set one or the other -- a discovery URI is associated with
exactly one issuer, but unfortunately an issuer may have multiple
discovery URIs, which I'm actively working on. (There is also some
relation to the multiple-issuers problem mentioned below.)

> I'm confused by the use of PG_MAX_AUTH_TOKEN_LENGTH in the
> pg_be_oauth_mech definition. What does that mean?

Just that Bearer tokens can be pretty long, so we don't want to limit
them to 1k like SCRAM does. 64k is probably overkill, but I've seen
anecdotal reports of tens of KBs and it seemed reasonable to match
what we're doing for GSS tokens.

> Also, shouldn't [oauth_validator_library] be an hba option instead? What if you want to
> use different validators for different connections?

Yes. This is again the multiple-issuers problem; I will split that off
into its own email since this one's getting long. It has security
implications.

> The CURL_IGNORE_DEPRECATION thing needs clarification. Is that in
> progress?

Thanks for the nudge, I've started a thread:

https://curl.se/mail/lib-2024-11/0028.html

> This is an anonymous union, which requires C11. Strangely, I cannot
> get clang to warn about this with -Wc11-extensions. Probably better
> to fix anyway. (The trailing supported MSVC versions don't support
> C11 yet.)

Oh, that's not going to be fun.

> This is only used once, in append_urlencoded(), and there are other
> ways to communicate errors, for example returning a bool.

I'd rather not introduce two parallel error indicators for the caller
to have to check for that particular part. But I can change over to
using the (identical!) termPQExpBuffer. I felt like the other API
signaled the intent a little better, though.

> On Cirrus CI Windows task, this test reports SKIP. Can't tell why,
> because the log is not kept. I suppose you expect this to work on
> Windows (but see my comment below)

No, builtin client support does not exist on Windows. If/when it's
added, the 001_server tests will need to be ported.

> +my $issuer = "https://127.0.0.1:54321";
>
> Use PostgreSQL::Test::Cluster::get_free_port() instead of hardcoding
> port numbers.
>
> Or is this a real port? I don't see it used anywhere else.

It's not real; 002_client.pl doesn't start an authorization server at
all. I can make that more explicit.

> src/test/perl/PostgreSQL/Test/OAuthServer.pm: Return value of flagged
> function ignored - read at line 39, column 2.

So perlcritic recognizes "or" but not the "//" operator... Lovely.

Thanks!
--Jacob