Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Peter Eisentraut <peter@eisentraut.org>

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

On 12.11.24 22:47, Jacob Champion wrote:
> On Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> 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.

Personally, I'm not even a fan of the -Dssl/--with-ssl system.  I'm more 
attached to --with-openssl.  But if you want to stick with that, a more 
suitable naming would be something like, say, --with-httplib=curl, which 
means, use curl for all your http needs.  Because if we later add other 
functionality that can use some http, I don't think we want to enable or 
disable them all individually, or even mix different http libraries for 
different features.  In practice, curl is a widely available and 
respected library, so I'd expect packagers to be just turn it all on 
without much further consideration.


>> 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.

Ah, ok, I totally misread that code.  Could you maybe write this definition

+/* Mechanism declaration */
+const pg_be_sasl_mech pg_be_oauth_mech = {
+   oauth_get_mechanisms,
+   oauth_init,
+   oauth_exchange,
+
+   PG_MAX_AUTH_TOKEN_LENGTH,
+};

with designated initializers:

const pg_be_sasl_mech pg_be_oauth_mech = {
     .get_mechanisms = oauth_get_mechanisms,
     .init = oauth_init,
     .exchange = oauth_exchange,
     .max_message_length = PG_MAX_AUTH_TOKEN_LENGTH,
};


>> 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

It looks like this has been clarified, so let's put that URL into a code 
comment.


>> 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.

I think it's better to not drill a new hole into an established API for 
such a limited use.  So termPQExpBuffer() seems better for now.  If it 
later turns out, many callers are using termPQExpBuffer() for fake error 
handling purposes, then that can be considered independently.


>> 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.

Could you put some kind of explicit conditional or a comment in there. 
Right now, it's not possible to tell that Windows is not supported.