Re: [PoC] Federated Authn/z with OAUTHBEARER

On Thu, Aug 26, 2021 at 9:13 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:

> On Wed, 2021-08-25 at 15:25 -0700, Zhihong Yu wrote:
> >
> > 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 ?
>
> Do you mean heap-based? i.e. destroyJsonLexContext() does an
> unnecessary copy before free? Yeah, in that case it's not super useful,
> but I think I'd want some evidence that the performance hit matters
> before optimizing it.
>
> Are there any other internal APIs that take a boolean parameter like
> that? If not, I think we'd probably just want to remove the copy
> entirely if it's a problem.
>
> > +#ifdef FRONTEND
> > + /* make sure initialization succeeded */
> > + if (lex->strval == NULL)
> > + return JSON_OUT_OF_MEMORY;
> >
> > Should PQExpBufferBroken(lex->strval) be used for the check ?
>
> It should be okay to continue if the strval is broken but non-NULL,
> since it's about to be reset. That has the fringe benefit of allowing
> the function to go as far as possible without failing, though that's
> probably a pretty weak justification.
>
> In practice, do you think that the probability of success is low enough
> that we should just short-circuit and be done with it?
>
> On Wed, 2021-08-25 at 16:24 -0700, Zhihong Yu wrote:
> >
> > 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.
>
> Yeah, I think that makes sense. FYI, this is probably one of the
> functions that will be rewritten completely once iddawc is removed.
>
> > + 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 ?
>
> We'd have to update our code; that would be a breaking change to the
> Device Authorization spec. Here's what it says today [1]:
>
> The "authorization_pending" and "slow_down" error codes define
> particularly unique behavior, as they indicate that the OAuth client
> should continue to poll the token endpoint by repeating the token
> request (implementing the precise behavior defined above). If the
> client receives an error response with any other error code, it MUST
> stop polling and SHOULD react accordingly, for example, by displaying
> an error to the user.
>
> > 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.
>
> That's true today, but implementations have a habit of changing. I
> personally prefer not to introduce too many exit points from a function
> that's already using goto. In my experience, that makes future
> maintenance harder.
>
> Thanks for the reviews! Have you been able to give the patchset a try
> with an OAuth deployment?
>
> --Jacob
>
> [1] https://datatracker.ietf.org/doc/html/rfc8628#section-3.5

Hi,
bq. destroyJsonLexContext() does an unnecessary copy before free? Yeah, in
that case it's not super useful,
but I think I'd want some evidence that the performance hit matters before
optimizing it.

Yes I agree.

bq. In practice, do you think that the probability of success is low enough
that we should just short-circuit and be done with it?

Haven't had a chance to try your patches out yet.
I will leave this to people who are more familiar with OAuth
implementation(s).

bq. I personally prefer not to introduce too many exit points from a
function that's already using goto.

Fair enough.

Cheers