Refactor SASL exchange in preparation for OAuth Bearer
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t49235psql -h localhost -U postgresBuilt from patchset v5 (message #5), July 27, 2026 at 11:05 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t49235_5 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t49235_5 && git checkout t49235_5Patchset v5 (message #5) is on t49235_5
The attached two patches are smaller refactorings to the SASL exchange and init
codepaths which are required for the OAuthbearer work [0]d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com. 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.camel@vmware.com
Attachments:
v1-0002-Explicitly-require-password-for-SCRAM-exchange.patchapplication/octet-stream; name=v1-0002-Explicitly-require-password-for-SCRAM-exchange.patch; x-unix-mode=0644Download+14-13
v1-0001-Refactor-SASL-exchange-to-return-tri-state-status.patchapplication/octet-stream; name=v1-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch; x-unix-mode=0644Download+71-68
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
On 26 Feb 2024, at 19:56, Jacob Champion <jacob.champion@enterprisedb.com> 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
Attachments:
v2-0001-Refactor-SASL-exchange-to-return-tri-state-status.patchapplication/octet-stream; name=v2-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch; x-unix-mode=0644Download+71-68
v2-0002-Explicitly-require-password-for-SCRAM-exchange.patchapplication/octet-stream; name=v2-0002-Explicitly-require-password-for-SCRAM-exchange.patch; x-unix-mode=0644Download+15-14
On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson <daniel@yesql.se> wrote:
I rank that as one of my better typos actually. Fixed though.
LGTM!
Thanks,
--Jacob
On 29 Feb 2024, at 20:58, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson <daniel@yesql.se> 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
Attachments:
t49235_5v3-0002-Explicitly-require-password-for-SCRAM-exchange.patchapplication/octet-stream; name=v3-0002-Explicitly-require-password-for-SCRAM-exchange.patch; x-unix-mode=0644Download+15-14
v3-0001-Refactor-SASL-exchange-to-return-tri-state-status.patchapplication/octet-stream; name=v3-0001-Refactor-SASL-exchange-to-return-tri-state-status.patch; x-unix-mode=0644Download+71-68
On 20 Mar 2024, at 15:28, Daniel Gustafsson <daniel@yesql.se> wrote:
On 29 Feb 2024, at 20:58, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
On Wed, Feb 28, 2024 at 2:54 PM Daniel Gustafsson <daniel@yesql.se> 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