Thread: dispchar for oauth_client_secret

commit b3f0be7 wrote:
+    {"oauth_scope", NULL, NULL, NULL,
+        "OAuth-Scope", "", 15,
+    offsetof(struct pg_conn, oauth_scope)},

The field containing "" is documented as follows:

    char       *dispchar;        /* Indicates how to display this field in a
                                 * connect dialog. Values are: "" Display
                                 * entered value as is "*" Password field -
                                 * hide value "D"  Debug option - don't show
                                 * by default */

I suspect this should use .dispchar="*" to encourage UIs to display
oauth_client_secret like a password field.  Thoughts?

[I didn't review commit b3f0be7, but this caught my attention.]

On Tue, Apr 15, 2025 at 12:14 PM Noah Misch <noah@leadboat.com> wrote:
> I suspect this should use .dispchar="*" to encourage UIs to display
> oauth_client_secret like a password field.  Thoughts?

Hmm, from a UI perspective I agree. (The builtin flow targets "public
clients", where secrets are discouraged and/or understood to be
not-really-secret, but there's no reason to assume that all flows used
by the application are public.)

From a proxy perspective, this would mess with FDW handling. By making
the dispchar '*', oauth_client_secret will be made into a user mapping
option rather than a server option. (Neither is very useful to
postgres_fdw anyway, because the builtin flow needs an end user to
interact with the provider.) But I'm not sure if we'll need to handle
compatibility in the future if we implement a proxy-friendly flow. Is
it okay to move options back and forth during a major version bump? I
assume it would present a problem for pg_upgrade?

Thanks!
--Jacob

On Tue, Apr 15, 2025 at 01:16:12PM -0700, Jacob Champion wrote:
> On Tue, Apr 15, 2025 at 12:14 PM Noah Misch <noah@leadboat.com> wrote:
> > I suspect this should use .dispchar="*" to encourage UIs to display
> > oauth_client_secret like a password field.  Thoughts?
> 
> Hmm, from a UI perspective I agree. (The builtin flow targets "public
> clients", where secrets are discouraged and/or understood to be
> not-really-secret, but there's no reason to assume that all flows used
> by the application are public.)
> 
> From a proxy perspective, this would mess with FDW handling. By making
> the dispchar '*', oauth_client_secret will be made into a user mapping
> option rather than a server option. (Neither is very useful to
> postgres_fdw anyway, because the builtin flow needs an end user to
> interact with the provider.) But I'm not sure if we'll need to handle
> compatibility in the future if we implement a proxy-friendly flow.

If we think oauth_client_secret should get dispchar="*" UI treatment yet be a
postgres_fdw server option, postgres_fdw code can make it so.  postgres_fdw
already has explicit code to reclassify the "user" option.

> Is
> it okay to move options back and forth during a major version bump?  I
> assume it would present a problem for pg_upgrade?

It would break dump/reload, so it's best not to move options between
optcontext=ForeignServerRelationId and optcontext=UserMappingRelationId, even
at major versions.  As above, we can change dispchar separately from
optcontext.  The documentation of dispchar likely should tell people to update
the postgres_fdw documentation when adding a dispchar="*' option.

It sounds like we might end up wanting to allow oauth_client_secret in both of
ForeignServerRelationId and UserMappingRelationId, each catering to different
oauth implementations.  Is that right?  (It would be fine to allow one today
and later change to allow both.)

On Tue, Apr 15, 2025 at 1:48 PM Noah Misch <noah@leadboat.com> wrote:
> If we think oauth_client_secret should get dispchar="*" UI treatment yet be a
> postgres_fdw server option, postgres_fdw code can make it so.  postgres_fdw
> already has explicit code to reclassify the "user" option.

Agreed, I will add an open item.

> The documentation of dispchar likely should tell people to update
> the postgres_fdw documentation when adding a dispchar="*' option.

(Or dispchar="D".)

> It sounds like we might end up wanting to allow oauth_client_secret in both of
> ForeignServerRelationId and UserMappingRelationId, each catering to different
> oauth implementations.  Is that right?  (It would be fine to allow one today
> and later change to allow both.)

I think, for the short-to-medium term, it is best to think of
oauth_client_id/oauth_client_secret/oauth_issuer as a packaged triple.
You don't want to use a secret with the wrong client ID, you don't
want to send a secret to the wrong provider, and one provider's client
ID won't be recognized by another. There might be some providers that
let you issue multiple secrets per client ID, but I seem to recall
that's intended more for rotation than it is for separate end users.

(It is true that separate users could make use of separate issuers
entirely. But that's conceptually separate from allowing a user to
control the HTTP entry point of server-side libpq without OWNER
privilege on the server. I don't like that at all from a risk
management perspective, and I'd prefer that we keep oauth_issuer on
the server context. It might be safe at some point to pull both
oauth_client_id and _secret down to the user context, once there's a
use case.)

Thanks!
--Jacob

On Tue, 15 Apr 2025 at 22:48, Noah Misch <noah@leadboat.com> wrote:
> yet be a
> postgres_fdw server option, postgres_fdw code can make it so.  postgres_fdw
> already has explicit code to reclassify the "user" option.

I don't understand why it should be a server option instead of a user
mapping option. Having it be a server option means that *any* Postgres
user can read its contents. My knowledge on the purpose is pretty
limited, but having that be the case for something with "secret" in
the name seems very unintuitive.

On Tue, Apr 15, 2025 at 3:25 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I don't understand why it should be a server option instead of a user
> mapping option. Having it be a server option means that *any* Postgres
> user can read its contents.

Thank you for saying something; I'd hallucinated that srvoptions was
limited to the server owner, and that's not true. It's pg_user_mapping
that has the protection.

But if you want to hide the client secret from your users, making it a
user mapping option has not made the situation better. The client ID
and secret are there to authenticate libpq (and by extension,
postgres_fdw), not the end user.

> My knowledge on the purpose is pretty
> limited, but having that be the case for something with "secret" in
> the name seems very unintuitive.

From the point of view of a proxy, whether you use a secret at all is
up to the flow in use. And for 18, the only supported flow is focused
on authorizing an actual human at the keyboard, without any token
caching, making it pretty unhelpful for FDW. A more proxy-friendly
flow would be awesome -- and/or explicit integration of the existing
builtin flow into the proxies, both safely and helpfully -- but that's
not a v18 thing. (I view it as similar in spirit to the
SCRAM-passthrough work.)

So: maybe it'd be best to disallow the OAuth options in the proxy code
entirely, so that a proxy-friendly future implementation is not
accidentally tied to decisions we made in v18.

--Jacob

On Wed, 16 Apr 2025 at 02:03, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Thank you for saying something; I'd hallucinated that srvoptions was
> limited to the server owner, and that's not true. It's pg_user_mapping
> that has the protection.

FWIW, I have some ideas on being able to store secrets in a server in
a safe way. I'll probably start a thread on that somewhere in the next
few months.

> So: maybe it'd be best to disallow the OAuth options in the proxy code
> entirely, so that a proxy-friendly future implementation is not
> accidentally tied to decisions we made in v18.

Seems fine to me.

On Wed, 16 Apr 2025 at 02:03, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Tue, Apr 15, 2025 at 3:25 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > I don't understand why it should be a server option instead of a user
> > mapping option. Having it be a server option means that *any* Postgres
> > user can read its contents.
>
> Thank you for saying something; I'd hallucinated that srvoptions was
> limited to the server owner, and that's not true. It's pg_user_mapping
> that has the protection.
>
> But if you want to hide the client secret from your users, making it a
> user mapping option has not made the situation better. The client ID
> and secret are there to authenticate libpq (and by extension,
> postgres_fdw), not the end user.
>
> > My knowledge on the purpose is pretty
> > limited, but having that be the case for something with "secret" in
> > the name seems very unintuitive.
>
> From the point of view of a proxy, whether you use a secret at all is
> up to the flow in use. And for 18, the only supported flow is focused
> on authorizing an actual human at the keyboard, without any token
> caching, making it pretty unhelpful for FDW. A more proxy-friendly
> flow would be awesome -- and/or explicit integration of the existing
> builtin flow into the proxies, both safely and helpfully -- but that's
> not a v18 thing. (I view it as similar in spirit to the
> SCRAM-passthrough work.)
>
> So: maybe it'd be best to disallow the OAuth options in the proxy code
> entirely, so that a proxy-friendly future implementation is not
> accidentally tied to decisions we made in v18.
>
> --Jacob