Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

Hi All,

The "issuer" field has been removed to align  with the RFC
implementation - https://www.rfc-editor.org/rfc/rfc7628.
This patch "v6" is a single patch to support the OAUTH BEARER token
through psql connection string.
Below flow is supported. Added the documentation in the commit messages.

 +----------------------+                                 +----------+
  |             +-------+                                  | Postgres |
  | PQconnect ->|       |                                  |          |
  |             |       |                                  |   +-----------+
  |             |       | ---------- Empty Token---------> | > |           |
  |             | libpq | <-- Error(Discovery + Scope ) -- | < | Pre-Auth  |
  |          +------+   |                                  |   |  Hook     |
  |     +- < | Hook |   |                                  |   +-----------+
  |     |    +------+   |                                  |          |
  |     v       |       |                                  |          |
  |  [get token]|       |                                  |          |
  |     |       |       |                                  |          |
  |     +       |       |                                  |   +-----------+
  | PQconnect > |       | --------- Access Token --------> | > | Validator |
  |             |       | <---------- Auth Result -------- | < |   Hook    |
  |             |       |                                  |   +-----------+
  |             +-------+                                  |          |
  +----------------------+                                 +----------+

Please note that we are working on modifying/adding new tests (from
Jacob's Patch) with the latest changes. Will add a patch with tests
soon.

Thanks,
Mahendrakar.

On Wed, 18 Jan 2023 at 07:24, Andrey Chudnovsky <achudnovskij@gmail.com> wrote:
>
> > 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
>
> Flow implementations in libpq are definitely a long term plan, and I
> agree that it would democratise the adoption.
> In the previous posts in this conversation I outlined the ones I think
> we should support.
>
> However, I don't see why it's strictly necessary to couple those.
> As long as the SASL exchange for OAUTHBEARER mechanism is supported by
> the protocol, the Client side can evolve at its own pace.
>
> At the same time, the current implementation allows clients to start
> building provider-agnostic OAUTH support. By using iddawc or OAUTH
> client implementations in the respective platforms.
> So I wouldn't refer to "larger providers", but rather "more motivated
> clients" here. Which definitely overlaps, but keeps the system open.
>
> > 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.)
> I agree with the statement that Device code is the best first choice
> if we absolutely have to pick one.
> Though I don't think we have to.
>
> While device flow can be used for all kinds of user-facing
> applications, it's specifically designed for input-constrained
> scenarios. As clearly stated in the Abstract here -
> https://www.rfc-editor.org/rfc/rfc8628
> The authorization code with pkce flow is recommended by the RFSc and
> major providers for cases when it's feasible.
> The long term goal is to provide both, though I don't see why the
> backbone protocol implementation first wouldn't add value.
>
> Another point is user authentication is one side of the whole story
> and the other critical one is system-to-system authentication. Where
> we have Client Credentials and Certificates.
> With the latter it is much harder to get generically implemented, as
> provider-specific tokens need to be signed.
>
> Adding the other reasoning, I think libpq support for specific flows
> can get in the further iterations, after the protocol support.
>
> > in that case I'd rather spend cycles on generic SASL.
> I see 2 approaches to generic SASL:
> (a). Generic SASL is a framework used in the protocol, with the
> mechanisms implemented on top and exposed to the DBAs as auth types to
> configure in hba.
> This is the direction we're going here, which is well aligned with the
> existing hba-based auth configuration.
> (b). Generic SASL exposed to developers on the server- and client-
> side to extend on. It seems to be a much longer shot.
> The specific points of large ambiguity are libpq distribution model
> (which you pointed to) and potential pluggability of insecure
> mechanisms.
>
> I do see (a) as a sweet spot with a lot of value for various
> participants with much less ambiguity.
>
> > 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.
> Thanks for your feedback on this. We had this discussion as well, and
> added that as a convenience for the client to identify the provider.
> I don't see a reason why an issuer would be absolutely necessary, so
> we will get your point that sticking to RFCs is a safer choice.
>
> > The patches seem to be out of order now (and the documentation in the
> > commit messages has been removed).
> Feedback taken. Work in progress.
>
> On Tue, Jan 17, 2023 at 2:44 PM Jacob Champion <jchampion@timescale.com> wrote:
> >
> > On Sun, Jan 15, 2023 at 12:03 PM Andrey Chudnovsky
> > <achudnovskij@gmail.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