Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Daniel Gustafsson <daniel@yesql.se>

View thread
From: Daniel Gustafsson <daniel@yesql.se>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Peter Eisentraut <peter@eisentraut.org>, Antonin Houska <ah@cybertec.at>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2025-02-17T12:03:36Z
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

Attachments

> On 15 Feb 2025, at 02:14, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

>> +   Since a misbehaving validator might let unauthorized users into the database,
>> +   correct implementation is critical. See
>> "Don't make any bugs" isn't very helpful advice =) Expanded on it slightly.
> 
> Hmm, I think the overloading of "validate" in the replacement text
> could be confusing. I guess my point is less "don't write bugs" and
> more "a bug here has extreme impact"? I've taken another shot at it;
> see what you think.

I'm not sure we're at the right wording still.  I have a feeling this topic is
worth a longer paragraph describing the potential severity of various error
conditions, but I don't think that needs to go in now, we can iterate on that
over time as well.

>> +    The server has ensured that the token is well-formed syntactically, but no
>> "server" is an overloaded nomenclature here, perhaps using libpq instead to
>> clearly indicate that it's postgres and not an OAuth server.
> 
> I've replaced this with "PostgreSQL" to match up with Peter's earlier
> feedback (we were using "libpq" to describe the backend and he wanted
> to avoid that).

Ah yes, much better.

>> +sanitize_char(char c)
>> +{
>> +       static char buf[5];
>> With the multithreading work on the horizon we should probably avoid static
>> variables like these to not create work for our future selves?  The code isn't
>> as neat when passing in a buffer/length but it avoids the need for a static or
>> threadlocal variable. Or am I overthinking this?
> 
> This is the only part of the feedback patch that I'm not a fan of,
> mostly because it begins to diverge heavily from the SCRAM code it
> copied from. I don't disagree with the goal of getting rid of the
> static buffers, but I would like to see them modified at the same time
> so that we can refactor easily if/when a third SASL mechanism shows
> up. (Maybe with a psprintf() rather than buffers?)

Fair enough, I can get behind that.

>> +                       p++;
>> +                       if (*p != ',')
>> If the SASL exchange, are we certain that a rogue client cannot inject a
>> message which trips us past the end of string?  Should we be doublecheck when
>> advancing p across the message?
> 
> The existing != checks will bail out if they get to the end of the
> string. It relies on byte-at-a-time advancement for safety, as well as
> the SASL code higher in the stack that ensures that the input buffer
> is always null terminated. (SCRAM relies on that too.) If we ever
> jumped farther than a byte, we'd need stronger checks, but at the
> moment I don't think this change helps us.

Thanks for clarifying.

>> In load_validator_library we don't explicitly verify that the required callback
>> is defined in the returned structure, which seems like a cheap enough belts and
>> suspenders level check.
> 
> Yeah, there's a later check at time of use, but it's not as
> user-friendly. I've adjusted the new error message to make it a bit
> closer to the logical plugin wording.

-    errmsg("%s module \"%s\" must define the symbol %s",
+    errmsg("%s module \"%s\" must provide a %s callback",

My rationale for picking the former message was that it's same as we have
earlier in the file, so it didn't add more translator work for (ideally) rarely
used errors.

That being said, I agree that should probably align these messages with the
counterparts for archive modules and logical plugins, which currently use the
following:

    errmsg("archive modules have to define the symbol %s", "_PG_archive_module_init")
    errmsg("archive modules must register an archive callback")
    elog(ERROR, "output plugins have to declare the _PG_output_plugin_init symbol");
    elog(ERROR, "output plugins have to register a begin callback");

It's a bit surprising to me that we use elog() for output plugins, while these
errors should be rare they can be triggered by third-party code so it seems
more appropriate to use ereport() IMHO.  Given that these are so similar we
should be able to reduce translator burden by providing more or less just two
messages.

Since this will be reaching into other parts of the code, it should be its own
patch though, so for now let's go with what you proposed and we can revisit this.

>> +       if (parsed < 1)
>> +               return actx->debugging ? 0 : 1;
>> Is 1 second a sane lower bound on interval for all situations?  I'm starting to
>> wonder if we should be more conservative here, or even make it configurable in
>> some way? The default if not set of 5 seconds is quite a lot higher than 1.
> 
> Mmm, maybe it should be made configurable, but one second seems like a
> long time from a CPU perspective. Maybe it would be applicable to
> embedded clients? But only if some provider out there actually starts
> using smaller intervals than their clients can stand... Should we wait
> to hear from someone who is interested in configuring it?

I indeed think we should await feedback, making it configurable isn't exactly
free so I hesitate to do it if nobody wants it.

The attached v51 squashes your commits together, discarding the changes
discussed here, and takes a stab at a commit message for these as this is
getting very close to be able to go in.  There are no additional changes.

--
Daniel Gustafsson