Re: [PoC] Federated Authn/z with OAUTHBEARER

On Thu, Feb 13, 2025 at 2:56 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> To make it easier for you to see what I mean I have implemented most of the
> comments and attached as a fixup patch, from which you can cherry-pick hunks
> you agree with. Those I didn't implement should be marked as such below.
>
> As we discussed off-list I took the liberty of squashing the previous fixup
> patches into a single one, and squashed your fixes for my comments against v47
> into 0001. All of my proposals are in 0004.

Great! I have attached v50; 0001 has almost all of v49 squashed in,
0002 has my changes on top (includes a pgindent/pgperltidy), and 0003
holds the only part I don't like (see below). 0004 contains the
FreeBSD hack that I suspect we'll need to merge in until Cirrus images
are updated.

> + The organization, product vendor, or other entity which develops and/or
> + administers the OAuth servers and clients for a given application.
> Since we define terminology here, shouldn't this be "OAuth resource servers"?

The resource server is Postgres for our purposes; I've changed it to
"authorization servers".

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

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

> +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?)

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

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

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

> + parsed = parse_json_number(expires_in_str);
> + parsed = round(parsed);
> Shouldn't we floor() the value here to ensure we never report an expiration
> time longer than the actual expiration?

Sounds reasonable. Done in 0002.

> register_socket() doesn't have an error catch for the case when neither epoll
> nor kqeue is supported. Shouldn't it set actx_error() here as well? (Not done
> in my review patch.)

Done.

> + if (actx->curl_err[0])
> + {
> + size_t len;
> +
> + appendPQExpBuffer(&conn->errorMessage, " (%s)", actx->curl_err);
> Should this also qualify that the error comes from outside of postgres?
> Something like "(libcurl:%s)" to match? I haven't changed this in the attached
> since I'm still on the fence, but I'm leanings that we probably should.
> Thoughts?

Done. More context is probably better than less here.

> - * We only support one mechanism at the moment, so rather than deal with a
> + * We only support two mechanisms at the moment, so rather than deal with a
> While there's nothing incorrect about this comment, I have a feeling we won't
> support more mechanisms than we can justify having a simple array for anytime
> soon =)

Yeah. My goal was mostly to justify the use of the (unusual)
static-length list to future readers.

Thank you so much for the reviews!

--Jacob