Re: [PoC] Federated Authn/z with OAUTHBEARER

On Fri, Feb 7, 2025 at 12:12 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> Is it really enough to do this at build time? A very small percentage of users
> running this will also be building their own libpq so the warning is lost on
> them. That being said, I'm not entirely sure what else we could do (bleeping a
> warning every time is clearly not userfriendly) so maybe this is a TODO in the
> code?

I've added a TODO back. At the moment, I don't have any good ideas; if
the user isn't building libpq, they're not going to be able to take
action on the warning anyway, and for many use cases they're probably
not going to care.

> + oauth_json_set_error(ctx, /* don't bother translating */
> With the project style format for translator comments this should be:
>
> + /* translator: xxx */
> + oauth_json_set_error(ctx,

This comment was just meant to draw attention to the lack of
libpq_gettext(). Does it still need a translator note if we don't run
it through translation?

> Does it make sense to prefix the printing in debug_callback() with some header
> stating that the following data is debug output from curl and not postgres? I
> have a feeling I'm stockholm syndromed by knowing the internals so I'm not sure
> if that would be helpful to someone not knowing the implementation details.

Seems reasonable; I've added "[libcurl]" to the front.

> Aha, cool! I was a bit surprised to not find a definition of expires_in in RFC
> 8628, as in what happens if -1 is passed? 8628 seems to broadly speaking fall
> into the category of "just dont do the wrong thing and all will be fine" =/.

Yup. :(

> Another question that comes to mind is how the reciever should interpret the
> information since it doesn't know when the device_code/user_code was generated
> so it doesn't know how much of expires_in which has already passed. (Which is
> not something for us to solve, just a general observation.)

And even if we passed the Date header value through from the server,
that'd do the wrong thing if the clocks are off. I think for now,
expires_in is likely to be a best-effort UX, in the vein of "hey,
maybe type faster if you don't want a timeout."

> To align more with the rest of the documentation I think something along these
> lines is better: "For more information, see section 3.3.1 in <ulink ..>RFC
> 8628</ulink>.

Done.

> While not introduced in this fixup patch, reading it again now I sort of think
> we should make that Assert(false) to clearly indicate that we don't think the
> assertion will ever pass, we're just asking to error out since we already know
> the failure condition holds.

Done.

> Maybe we should add a translator note explaining that kqueue should not be
> translated since it's very easy to mistake it for "queue". Doing it on the
> first string including kqueue should be enough I suppose.

Done.

--

v48 is attached.

- 0001 contains all of the v47 fixups squashed into v47-0001.
- 0002 contains all the above feedback and rewrites two more commented TODOs.
- 0003 completes a couple of summary paragraphs in the documentation,
and makes it clear that the builtin flow is not currently supported on
Windows.
- 0004 gets a missed pgperltidy and explicitly skips unsupported tests
on Windows.
- 0005 cowardly pulls the MAX_OAUTH_RESPONSE_SIZE down to 256k.

- 0006 gives us additional levers to pull in the event that API or ABI
changes must be backported for security reasons:

Daniel and I talked at FOSDEM about wanting to have additional
guardrails on the server-side validator API. Ideally, we'd wait for
major version boundaries to change APIs, as per usual. But if any bugs
come to light that affect the security of the system, we may want to
have more control over the boundary between the server and the
validator. So I've added two features to the API.

The first is a magic number embedded in the OAuthValidatorCallbacks
struct. Should it ever be necessary to force a recompilation of
validator modules, that number can be bumped in an emergency to allow
the server to reject modules with an older ABI (or otherwise treat
them differently).

The second is state->sversion, added to the ValidatorModuleState
struct, which contains the PG_VERSION_NUM. This currently has no use,
but if there's ever a situation where the ValidatorModule* structs
need to gain new members within a stable release line, this would let
module developers make sense of the situation. It also provides an
easy way for modules to enforce a minimum minor version, for example
if there's a critical security bug in older versions that they'd
rather not deal with.

By adding these fields in addition to the existing module magic
machinery, we've probably doomed them to be unused cruft. But that
seems better than the reverse situation.

Thanks!

--Jacob