Refactor SASL exchange in preparation for OAuth Bearer
The attached two patches are smaller refactorings to the SASL exchange and init codepaths which are required for the OAuthbearer work [0]. Regardless of the future of that patchset, these refactorings are nice cleanups and can be considered in isolation. Another goal is of course to reduce scope of the OAuth patchset to make it easier to review. The first patch change state return from the exchange call to use a tri-state return value instead of the current output parameters. This makes it possible to introduce async flows, but it also makes the code a lot more readable due to using descriptve names IMHO. The second patch sets password_needed during SASL init on the SCRAM exchanges. This was implicit in the code but since not all future exchanges may require password, do it explicitly per mechanism instead. -- Daniel Gustafsson [0] d1b467a78e0e36ed85a09adf979d04cf124a9d4b.ca...@vmware.com v1-0002-Explicitly-require-password-for-SCRAM-exchange.patch Description: Binary data v1-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch Description: Binary data
Re: Refactor SASL exchange in preparation for OAuth Bearer
On Fri, Feb 23, 2024 at 2:30 AM Daniel Gustafsson wrote: > > The attached two patches are smaller refactorings to the SASL exchange and > init > codepaths which are required for the OAuthbearer work [0]. Regardless of the > future of that patchset, these refactorings are nice cleanups and can be > considered in isolation. Another goal is of course to reduce scope of the > OAuth patchset to make it easier to review. Thanks for pulling these out! They look good overall, just a few notes below. In 0001: > + * SASL_FAILED: The exchance has failed and the connection should be s/exchance/exchange/ > - if (final && !done) > + if (final && !(status == SASL_FAILED || status == SASL_COMPLETE)) Since there's not yet a SASL_ASYNC, I wonder if this would be more readable if it were changed to if (final && status == SASL_CONTINUE) to match the if condition shortly after it. In 0002, at the beginning of pg_SASL_init, the `password` variable now has an uninitialized code path. The OAuth patchset initializes it to NULL: > +++ b/src/interfaces/libpq/fe-auth.c > @@ -425,7 +425,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) > int initialresponselen; > const char *selected_mechanism; > PQExpBufferData mechanism_buf; > - char *password; > + char *password = NULL; > SASLStatus status; > > initPQExpBuffer(&mechanism_buf); I'll base the next version of the OAuth patchset on top of these. Thanks! --Jacob
Re: Refactor SASL exchange in preparation for OAuth Bearer
> On 26 Feb 2024, at 19:56, Jacob Champion > wrote: >> + * SASL_FAILED: The exchance has failed and the connection should be > > s/exchance/exchange/ I rank that as one of my better typos actually. Fixed though. >> - if (final && !done) >> + if (final && !(status == SASL_FAILED || status == SASL_COMPLETE)) > > Since there's not yet a SASL_ASYNC, I wonder if this would be more > readable if it were changed to >if (final && status == SASL_CONTINUE) > to match the if condition shortly after it. Fair point, that's more readable in this commit. > In 0002, at the beginning of pg_SASL_init, the `password` variable now > has an uninitialized code path. The OAuth patchset initializes it to > NULL: Nice catch, fixed. -- Daniel Gustafsson v2-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch Description: Binary data v2-0002-Explicitly-require-password-for-SCRAM-exchange.patch Description: Binary data
Re: Refactor SASL exchange in preparation for OAuth Bearer
On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson wrote: > I rank that as one of my better typos actually. Fixed though. LGTM! Thanks, --Jacob
Re: Refactor SASL exchange in preparation for OAuth Bearer
> On 29 Feb 2024, at 20:58, Jacob Champion > wrote: > > On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson wrote: >> I rank that as one of my better typos actually. Fixed though. > > LGTM! Thanks for review, and since Heikki marked it ready for committer I assume that counting as a +1 as well. Attached is a rebase on top of HEAD to get a fresh run from the CFBot before applying this. -- Daniel Gustafsson v3-0002-Explicitly-require-password-for-SCRAM-exchange.patch Description: Binary data v3-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch Description: Binary data
Re: Refactor SASL exchange in preparation for OAuth Bearer
> On 20 Mar 2024, at 15:28, Daniel Gustafsson wrote: > >> On 29 Feb 2024, at 20:58, Jacob Champion >> wrote: >> >> On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson wrote: >>> I rank that as one of my better typos actually. Fixed though. >> >> LGTM! > > Thanks for review, and since Heikki marked it ready for committer I assume > that > counting as a +1 as well. Attached is a rebase on top of HEAD to get a fresh > run from the CFBot before applying this. And after another pass over it I ended up pushing it today. -- Daniel Gustafsson