Re: Refactor SASL exchange in preparation for OAuth Bearer

On Fri, Feb 23, 2024 at 2:30 AM Daniel Gustafsson <daniel@yesql.se> 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