Re: [PoC] Federated Authn/z with OAUTHBEARER

> On 15 Feb 2025, at 02:14, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)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