public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jacob Champion <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Refactor SASL exchange in preparation for OAuth Bearer
Date: Mon, 26 Feb 2024 10:56:39 -0800
Message-ID: <CAOYmi+=DE-Uu8jN9gYaiEjV8g0jHOZZphS8xj1zM7nQNP0tq8g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

On Fri, Feb 23, 2024 at 2:30 AM Daniel Gustafsson <[email protected]> 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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Refactor SASL exchange in preparation for OAuth Bearer
  In-Reply-To: <CAOYmi+=DE-Uu8jN9gYaiEjV8g0jHOZZphS8xj1zM7nQNP0tq8g@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox