Re: Custom oauth validator options

Hi all,

First, an apology for the state of this thread -- I thought I had
already responded to the prior message last month. Turns out, I had
not... Instead, I'm dropping a surprise alternative patchset on top of
a thread that I let go cold, which is rude, and I'm sorry.

Here are my overall thoughts on the approaches shared so far. And
thank you, Zsolt, for doing so much legwork here; that effort was not
wasted at all.

1) A GUC-centric solution -- option (d) -- is the Ideal Solution here,
IMO. We shouldn't have to reinvent the wheel.

2) A GUC-centric solution is not going to land for 19, and I'd be
surprised if it landed for 20, given the coalition that I'll need to
build to say either "yes" or "no". There are too many questions about
session_preload_libraries and prefixing and etc., and I'm honestly not
a fan of the guc_prefix_enforcement approach.

3) I'm not convinced yet that GUCs and relation options are similar
enough to share a framework. (This is not a rejection of the reloption
refactoring work, just a statement that I don't want to rely on it to
solve this problem.)

4) I really don't like the hba_parse_option_hook. I prefer APIs over
hooks as a general architectural point, and more practically, I don't
want to hand control to extensions during HBA parsing. I don't think
they're going to coordinate with each other well, and I think they're
likely to couple against internals in ways we don't want to support
(which is my general problem with hooks).

= Option (b) =

I don't want this problem to go unfixed for 2+ years, so I think it
would be best to reinvent a very small wheel that doesn't take up a
lot of maintenance effort once it's in, and then simply replace it
with the Ideal Solution eventually. This is my take on option (b),
which is what Vasuki M advocated for upthread. It's just a
setter/getter API for string keys and values:

static const char* opts[] = { "my_parameter" };

void startup_cb(ValidatorModuleState *state)
{
RegisterOAuthHBAOptions(state, lengthof(opts), opts);
}

bool validate_cb(const ValidatorModuleState *state, ...)
{
const char *param;
if ((param = GetOAuthHBAOption(state, "my_parameter")) != NULL)
do_something_with(param);
...
}

And then in your HBA:

host all all ::1/128 oauth validator.my_parameter=foo ...

The core implementation is conceptually simple. Most of the lines in
the patch are guardrails, to reduce the probability of regret over
this temporary solution:
- Parameter names aren't freeform; they're restricted to
almost-alphanumeric identifiers. (We've wanted to steal symbols for
HBA features in the past.)
- Name syntax is checked on reload, but cross-referencing the
registered list must be deferred to connection time. That's unusual
for users, so the WARNING that gets printed in this case is extremely
verbose; that way hopefully no one will be confused about what is
happening.
- The compiler won't let you register names during validate_cb, and we
won't let you retrieve options during startup_cb, so we retain control
over the internal order of operations. (This is discussed more deeply
in the patch.)

I was worried that we'd need a third API call to report option parsing
failures from the validator. Instead of doing that, I've made it
easier to link an authentication failure to a validator internal error
that caused it, in v3-0001, which serendipitously fixes a separate
sharp edge [1]. I feel good about that patch even if -0002 doesn't
make it.

v3-0002 still lacks user documentation, which I need to write a lot of
-- but if everyone dislikes this approach then I'd rather not spend
the time there.

= WDYT? =

I realize this puts you all in a difficult position; the effect is
kind of "take it or leave it", which again was not my intent. I
considered letting this lapse for 19 instead. But since I believe the
ideal solution is one we can't have for a while, and there's good
research and discussion of alternatives in this thread, waiting may
not produce a more committable short-term solution in the end.

Let me know if any of you disagree, though -- especially if you think
the status quo is preferable! -- and I can shelve -0002 for now. (I'll
continue with -0001 at [1] either way.)

Thanks,
--Jacob

[1] https://postgr.es/m/202601241015.y5uvxd7oxnfs%40alvherre.pgsql