Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mJ2Gf-0005EP-M4 for pgsql-hackers@arkaria.postgresql.org; Wed, 25 Aug 2021 23:25:13 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1mJ2G7-0000Vs-Vb for pgsql-hackers@arkaria.postgresql.org; Wed, 25 Aug 2021 23:24:39 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mJ2G7-0000Vk-MI for pgsql-hackers@lists.postgresql.org; Wed, 25 Aug 2021 23:24:39 +0000 Received: from mail-lj1-x229.google.com ([2a00:1450:4864:20::229]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1mJ2G5-00059q-Jl for pgsql-hackers@postgresql.org; Wed, 25 Aug 2021 23:24:39 +0000 Received: by mail-lj1-x229.google.com with SMTP id y6so1582086lje.2 for ; Wed, 25 Aug 2021 16:24:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yugabyte.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=swvBYKa24MyxGgUY0GKoKZv1YKSPvlN+mL975drkzc4=; b=Xp4CUBCun8if+XIbN/wVINdMMzyEQcuCWy7eQXj5o/aUNm+94Do/GJ2bhi8byt1uFg 6UW6znmU5b1LXym44ZOqy67PdF8i6VBCCBLBwXfLEKw5J0pgpmy6lyVM2P8gJRSs/BCB CDh6v2a8kQQGuD/+yJ5GSRrfdJxoQ4y+1Su3Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=swvBYKa24MyxGgUY0GKoKZv1YKSPvlN+mL975drkzc4=; b=hPcZo863zm/K52QXo/VBLEbCRMM0229QnEbtdFw+grLe4Q1tbkRlaNUc+yKxibjDrt ZzKSxQcuUfuS730CipZPP5f6sGLgOAq+AivLuf+j2YbIXhBk8kM9KH2YQ3b5Ekh27LKw qZWogplRoce4EdHq0aHcXBhdnO14vAZSC73rXGLfUsO4A7pU1bLskRtB8fhVNhhoWvAx 3SkQ0IwYbqKapMHgQcZWltuWhdRkpIdzuR01vTJZK0EOWy5rEISGlBO0RuTk88EkJqtP slK3YtQ2JN1/jsMxgpbse6y5K3kH8CemWS8lq1k86D2dFgJas84tDjoIlKhwL5aD0Gmw rJMw== X-Gm-Message-State: AOAM532Z0SXnxTouceG+HDJiO7NtHKteiNXt/A0DdzA1t2wEY60jgnD+ x2hMaOkO3TPe8jLdo5z+IUuaxWT5WLuM88C9Co2qOA== X-Google-Smtp-Source: ABdhPJyJ85ftfebIsL0VnRePeOuUeYaUl2b4T6BM/rhfVFu1dJKAeZbm6QjXZQpLgPTs2spNgraZKb/d3R1UsMsRt4o= X-Received: by 2002:a2e:4a1a:: with SMTP id x26mr515960lja.299.1629933875720; Wed, 25 Aug 2021 16:24:35 -0700 (PDT) MIME-Version: 1.0 References: <2f06a4aef1d5defe4b7aaec01478572e5557d32a.camel@vmware.com> <8a5a35b31c3f25f6b047e77def0445a60399981d.camel@vmware.com> In-Reply-To: From: Zhihong Yu Date: Wed, 25 Aug 2021 16:24:19 -0700 Message-ID: Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER To: Jacob Champion Cc: "hlinnaka@iki.fi" , "pgsql-hackers@postgresql.org" , "michael@paquier.xyz" Content-Type: multipart/alternative; boundary="000000000000ab856d05ca6a8db8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000ab856d05ca6a8db8 Content-Type: text/plain; charset="UTF-8" On Wed, Aug 25, 2021 at 3:25 PM Zhihong Yu wrote: > > > On Wed, Aug 25, 2021 at 11:42 AM Jacob Champion > 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 --000000000000ab856d05ca6a8db8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Aug 25, 2021 at 3:25 PM Zhiho= ng Yu <zyu@yugabyte.com> wrot= e:


On Wed, Aug 25, 2021 at 11:42 AM Jacob Champion &l= t;pchampion@vmwar= e.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 fu= nction:
> >
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Handle the client's initi= al message. */
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0p =3D 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:
> >
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 ereport(ERROR,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (errcode(ERRCODE_PROTOCOL_VIOLATION),
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errmsg("malformed OAUTHBEARER me= ssage"),
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errdetail("Comma expected, but f= ound character \"%s\".",
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sa= nitize_char(*p))));
> >
> > I don't think the double quotes are needed here, because sani= tize_char
> > will return quotes if it's a single character. So it would en= d 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=C2=A0v2-0001-common-jsonapi-= support-FRONTEND-clients.patch :

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

At the end of=C2=A0termJsonLexContext(), empty is copied to lex. For = stack based=C2=A0JsonLexContext, the copy seems unnecessary.
Mayb= e introduce a boolean parameter for=C2=A0termJsonLexContext() to signal tha= t the copy can be omitted ?

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

Should= =C2=A0PQExpBufferBroken(lex->strval) be used for the check ?
<= br>
Thanks
Hi,
For= =C2=A0v2-0002-libpq-add-OAUTHBEARER-SASL-mechanism.patch :

+ =C2=A0 i_init_session(&session);
+
+ =C2=A0 if (!conn->o= auth_client_id)
+ =C2=A0 {
+ =C2=A0 =C2=A0 =C2=A0 /* We can't tal= k to a server without a client identifier. */
+ =C2=A0 =C2=A0 =C2=A0 app= endPQExpBufferStr(&conn->errorMessage,
+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0li= bpq_gettext("no oauth_client_id is set for the connection"));
=
+ =C2=A0 =C2=A0 =C2=A0 goto cleanup;

Can=C2= =A0conn->oauth_client_id check be performed ahead of=C2=A0i_init_session= () ? That way, ```goto cleanup``` can be replaced with return.
+ =C2=A0 =C2=A0 =C2=A0 if (!error_code || (strcmp(error_code, = "authorization_pending")
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 && strcmp(e= rror_code, "slow_down")))

What if, i= n the future, there is error code different from the above two which doesn&= #39;t represent=C2=A0"OAuth token retrieval failed" scenario ?

For=C2=A0client_initial_response(),

+ =C2=A0 token_buf =3D createPQExpBuffer();
+ =C2=A0 if (!token= _buf)
+ =C2=A0 =C2=A0 =C2=A0 goto cleanup;

= If=C2=A0token_buf is NULL, there doesn't seem to be anything to free. W= e can return directly.

Cheers

=
--000000000000ab856d05ca6a8db8--