On Sat, Jan 17, 2026 at 1:37 AM Zsolt Parragi <[email protected]> wrote:
> The patches look good, I don't have comments for them directly.
>
> I also tried out a simple client-server plugin pair with them, both
> built as external plugins, and it works.
Thanks very much for the review!
> I have some practical findings based on that:
>
> 1. If I provide an incorrect library path to psql, it suggests that I
> should install the libpq-oauth package. It would be better to tell the
> user that PGOAUTHMODULE is invalid in this case.
Agreed, will fix.
> 2. Even if I use a custom library (which doesn't default to the usual
> get the discovery json - do a device flow - etc flow), it tries to
> construct/validate the well known discovery url. Why does it do that,
> if it doesn't try to use the URL directly? The only validation done
> inside libpq is now to validate that the URL matches the server URL.
> The server doesn't do this validation at all, it accepts any string I
> provide and starts up. If we want this URL validation, shouldn't it
> happen in the server, and in the client side oauth plugin?
I'm assuming you're talking about issuer_from_well_known_uri()? That's
a security gate, not just a syntax validation. We still need to avoid
mixup attacks, and I didn't want to punt that responsibility down to
the plugins, because they probably won't do it.
(Does that help clarify enough? One caller has a comment to this
effect, but not the other, and maybe I could add some to the doc
comment for the function itself?)
> (Not doing
> it on the server but enforcing it on the client seems like a strange
> choice, as typos/misconfigurations will need server restarts)
Hopefully you mean reloads, or else there's a bad bug somewhere. As
for the usability request to validate syntax in the server, I agree
that would be good. I think that came up during v18 development...
> 3. Is it really still OAuth, and not a generic pluggable SASLBEARER
> authentication?
It's still OAUTHBEARER, yes. The underlying mechanism is tied to OAuth
via both scopes and OAuth status codes. The latter would become more
apparent if we find use for any resource-server error responses other
than `invalid_token`. `insufficient_scope` might be nice eventually...
> Yes, I still have to provide the "oauth_issuer" and
> "oauth_client_id" parameters, but I don't have to do anything with it.
> I can implement any client side authentication I want in a libpq
> plugin, as long as I am able to verify it on the server side by
> sending a single token using SASL.
For now, yes. I can't really stop anyone from tunneling magic junk
through Bearer tokens, or LDAP passwords, or anything else opaque.
(Until and unless the governing specs require us to. Things change,
after all; that's the risk you take when you write code on top of a
layering violation.)
But I'm also not very interested in supporting that use case with this
feature. You can already (ab)use PAM this way, as far as I know; you
don't need to assume the extra architectural cost and security checks
of OAUTHBEARER. And the architecture changes for OAuth introduced
enough of the bones of pluggable auth that a particular SASL mechanism
shouldn't *have* to be abused in this way, if what people really want
is pluggable auth.
> That token doesn't have to be an
> OAuth token, because I can change both the creation and validation
> part. So why don't you call this SASLBEARER, with the default provided
> implementation being OAuthBearer, that seems to be a better fitting
> name for it?
Well, SASLBEARER isn't a SASL mechanism. We didn't invent OAUTHBEARER,
and if a future revision to its spec binds it even more tightly to new
OAuth-specific features, we should feel free to adopt them in our
mechanism implementation.
> 4. Have you thought about parameter passing, if my custom plugin needs
> extra configuration?
Yeah, that's actually part of why I got stuck in my first revision. I
realized I was creating an entirely new ecosystem of stuff for libpq
to worry about, and I needed to walk back the feature scope. So
plugins will need out-of-band config for now, which precludes
per-connection settings.
My idea for the first revision was an oauth_flow connection parameter,
with the syntax
oauth_flow=<plugin_name>
oauth_flow=<plugin_name>:<plugin-specific-parameter-string>
So maybe the default would be oauth_flow=builtin, or
oauth_flow=libpq:device, or oauth_flow=libpq-device, or... And then
third parties could do their own thing.
> 5. I was about to submit a separate patch we got some requests about:
> a way to supply an OAuth token directly to psql (PGOAUTHTOKEN=...
> bin/psql ...), by implementing PQsetAuthDataHook in psql/other command
> line tools. (Multiple users asked for this)
I'm still fairly opposed to manual token passing. It destroys any
semblance of token secrecy; it makes it trivial to exfiltrate
laterally, via `cat /proc/xxx/environ`; it removes a forcing function
for proper flow support (my weakest argument, to be fair). It'll also
become completely useless as soon as we have sender constraints,
because if you don't have the binding material, you can't use the
token.
Are they asking for this because it'd be an easy way around the v18
flow limitation? Because that's been the primary motivation in the
conversations I've had. I'd rather give them the ability to obtain the
token, in-process, the way they want, and then weird user-specific
tradeoffs are their decision and not ours. We can try to focus on
best-possible-implementation here in libpq.
> 6. Still about PQsetAuthDataHook, this seems to have a limited use
> case: if I am in the control of the application, writing it,
> PQsetAuthDataHook seems to be a better choice. These plugins are
> useful if I have to modify an existing generic application, to use a
> different authentication method, and that seems to be useful,
> especially if I treat it as a generic SASLBEARER API. But this also
> goes back to the security questions I raised in another thread: could
> an application say that it doesn't want to use plugins? Could it be
> configured in another way, other than environment variables? (if an
> application has a config file for example, it seems to be more
> practical to make this plugin part of that config file, instead of
> relying on a completely different environment variable)
Yeah, the question of "how do I configure this thing that is nested
five layers deep in my application stack" is everpresent, and I'm not
sure that the world is converging on a solution. (My preference is
"trusted-file-on-disk plus client API" for this, personally. I'm
wrestling with that over at [1] and it's not baked enough to solve
this.)
--Jacob
[1]
https://postgr.es/m/CAOYmi%2BnDMumexG4X4N9_jMU%3DyEiHOB%3D3TdYBPr0aYgPVb_TYAw%40mail.gmail.com
