Re: [PoC] Federated Authn/z with OAUTHBEARER

On Sun, Jan 15, 2023 at 12:03 PM Andrey Chudnovsky
<achudnovskij(at)gmail(dot)com> wrote:
> 2. Removed Device Code implementation in libpq. Several reasons:
> - Reduce scope and focus on the protocol first.
> - Device code implementation uses iddawc dependency. Taking this
> dependency is a controversial step which requires broader discussion.
> - Device code implementation without iddaws would significantly
> increase the scope of the patch, as libpq needs to poll the token
> endpoint, setup different API calls, e.t.c.
> - That flow should canonically only be used for clients which can't
> invoke browsers. If it is the only flow to be implemented, it can be
> used in the context when it's not expected by the OAUTH protocol.

I'm not understanding the concern in the final point -- providers
generally require you to opt into device authorization, at least as far
as I can tell. So if you decide that it's not appropriate for your use
case... don't enable it. (And I haven't seen any claims that opting into
device authorization weakens the other flows in any way. So if we're
going to implement a flow in libpq, I still think device authorization
is the best choice, since it works on headless machines as well as those
with browsers.)

All of this points at a bigger question to the community: if we choose
not to provide a flow implementation in libpq, is adding OAUTHBEARER
worth the additional maintenance cost?

My personal vote would be "no". I think the hook-only approach proposed
here would ensure that only larger providers would implement it in
practice, and in that case I'd rather spend cycles on generic SASL.

> 3. Temporarily removed test suite. We are actively working on aligning
> the tests with the latest changes. Will add a patch with tests soon.

Okay. Case in point, the following change to the patch appears to be
invalid JSON:

> + appendStringInfo(&buf,
> + "{ "
> + "\"status\": \"invalid_token\", "
> + "\"openid-configuration\": \"%s\","
> + "\"scope\": \"%s\" ",
> + "\"issuer\": \"%s\" ",
> + "}",

Additionally, the "issuer" field added here is not part of the RFC. I've
written my thoughts about unofficial extensions upthread but haven't
received a response, so I'm going to start being more strident: Please,
for the sake of reviewers, call out changes you've made to the spec, and
why they're justified.

The patches seem to be out of order now (and the documentation in the
commit messages has been removed).

Thanks,
--Jacob