MIME-Version: 1.0
References: <F51F8777-FAF5-49F2-BC5E-8F9EB423ECE0@yesql.se>
In-Reply-To: <F51F8777-FAF5-49F2-BC5E-8F9EB423ECE0@yesql.se>
From: Jacob Champion <jacob.champion@enterprisedb.com>
Date: Mon, 26 Feb 2024 10:56:39 -0800
Message-ID: <CAOYmi+=DE-Uu8jN9gYaiEjV8g0jHOZZphS8xj1zM7nQNP0tq8g@mail.gmail.com>
Subject: Re: Refactor SASL exchange in preparation for OAuth Bearer
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAOYmi%2B%3DDE-Uu8jN9gYaiEjV8g0jHOZZphS8xj1zM7nQNP0tq8g%40mail.gmail.com>
Precedence: bulk

On Fri, Feb 23, 2024 at 2:30=E2=80=AFAM Daniel Gustafsson <daniel@yesql.se>=
 wrote:
>
> The attached two patches are smaller refactorings to the SASL exchange an=
d 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 th=
e
> OAuth patchset to make it easier to review.

Thanks for pulling these out! They look good overall, just a few notes belo=
w.

In 0001:

> + * SASL_FAILED: The exchance has failed and the connection should be

s/exchance/exchange/

> - if (final && !done)
> + if (final && !(status =3D=3D SASL_FAILED || status =3D=3D 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 =3D=3D 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 =3D NULL;
>         SASLStatus      status;
>
>         initPQExpBuffer(&mechanism_buf);

I'll base the next version of the OAuth patchset on top of these.

Thanks!
--Jacob