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

From Zhihong Yu
Subject Re: [PoC] Federated Authn/z with OAUTHBEARER
Date
Msg-id CALNJ-vS-GEkGjCir2Yhs-EMdwM+TntuLZ2dAo0pVpuAyRT4FcQ@mail.gmail.com
Whole thread Raw
In response to Re: [PoC] Federated Authn/z with OAUTHBEARER  (Zhihong Yu <zyu@yugabyte.com>)
Responses Re: [PoC] Federated Authn/z with OAUTHBEARER
List pgsql-hackers


On Wed, Aug 25, 2021 at 3:25 PM Zhihong Yu <zyu@yugabyte.com> wrote:


On Wed, Aug 25, 2021 at 11:42 AM Jacob Champion <pchampion@vmware.com> wrote:
On Tue, 2021-06-22 at 23:22 +0000, Jacob Champion wrote:
> On Fri, 2021-06-18 at 11:31 +0300, Heikki Linnakangas wrote:
> >
> > A few small things caught my eye in the backend oauth_exchange function:
> >
> > > +       /* Handle the client's initial message. */
> > > +       p = strdup(input);
> >
> > this strdup() should be pstrdup().
>
> Thanks, I'll fix that in the next re-roll.
>
> > In the same function, there are a bunch of reports like this:
> >
> > >                    ereport(ERROR,
> > > +                          (errcode(ERRCODE_PROTOCOL_VIOLATION),
> > > +                           errmsg("malformed OAUTHBEARER message"),
> > > +                           errdetail("Comma expected, but found character \"%s\".",
> > > +                                     sanitize_char(*p))));
> >
> > I don't think the double quotes are needed here, because sanitize_char
> > will return quotes if it's a single character. So it would end up
> > looking like this: ... found character "'x'".
>
> I'll fix this too. Thanks!

v2, attached, incorporates Heikki's suggested fixes and also rebases on
top of latest HEAD, which had the SASL refactoring changes committed
last month.

The biggest change from the last patchset is 0001, an attempt at
enabling jsonapi in the frontend without the use of palloc(), based on
suggestions by Michael and Tom from last commitfest. I've also made
some improvements to the pytest suite. No major changes to the OAuth
implementation yet.

--Jacob
Hi,
For v2-0001-common-jsonapi-support-FRONTEND-clients.patch :

+   /* Clean up. */
+   termJsonLexContext(&lex); 

At the end of termJsonLexContext(), empty is copied to lex. For stack based JsonLexContext, the copy seems unnecessary.
Maybe introduce a boolean parameter for termJsonLexContext() to signal that the copy can be omitted ?

+#ifdef FRONTEND
+       /* make sure initialization succeeded */
+       if (lex->strval == NULL)
+           return JSON_OUT_OF_MEMORY;

Should PQExpBufferBroken(lex->strval) be used for the check ?

Thanks
Hi,
For v2-0002-libpq-add-OAUTHBEARER-SASL-mechanism.patch :

+   i_init_session(&session);
+
+   if (!conn->oauth_client_id)
+   {
+       /* We can't talk to a server without a client identifier. */
+       appendPQExpBufferStr(&conn->errorMessage,
+                            libpq_gettext("no oauth_client_id is set for the connection"));
+       goto cleanup;

Can conn->oauth_client_id check be performed ahead of i_init_session() ? That way, ```goto cleanup``` can be replaced with return.

+       if (!error_code || (strcmp(error_code, "authorization_pending")
+                           && strcmp(error_code, "slow_down")))

What if, in the future, there is error code different from the above two which doesn't represent "OAuth token retrieval failed" scenario ?

For client_initial_response(),

+   token_buf = createPQExpBuffer();
+   if (!token_buf)
+       goto cleanup;

If token_buf is NULL, there doesn't seem to be anything to free. We can return directly.

Cheers

pgsql-hackers by date:

Previous
From: Zhihong Yu
Date:
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Next
From: "alvherre@alvh.no-ip.org"
Date:
Subject: Re: prevent immature WAL streaming