Re: [PATCH] Pull general SASL framework out of SCRAM

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Jacob Champion <pchampion(at)vmware(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Pull general SASL framework out of SCRAM
Date: 2021-07-05 08:17:38
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jun 30, 2021 at 10:30:12PM +0000, Jacob Champion wrote:
> Done in v3, with a second patch for the code motion.

I have gone through that, tweaking the documentation you have added as
that's the meat of the patch, reworking a bit the declarations of the
callbacks (no need for several typedef gere) and doing some small
format changes to make the indentation happy. And that looks pretty
good. It is a bit sad that the SCRAM part cannot be completely
unplugged from the auth part, because of the call to the free function
and the HBA checks, but adding more wrappers to accomodate with that
is not really worth it. So I'd like to apply that to clarify this
code layer, without the TODOs.

- pg_be_scram_get_mechanisms(port, &sasl_mechs);
- /* Put another '\0' to mark that list is finished. */
- appendStringInfoChar(&sasl_mechs, '\0');
I was wondering for a couple of seconds if it would not be better to
let the last '\0' being set within the callback, but what you have
here looks better.

- if (!pg_fe_scram_channel_bound(conn->sasl_state))
+ if (!conn->sasl || !conn->sasl->channel_bound(conn->sasl_state))
conn->sasl should be set in this code path. This style is safer.

The top comment of scram_init() still mentioned
pg_be_scram_get_mechanisms(), while it should be

PG_MAX_SASL_MESSAGE_LENGTH can stay within auth-sasl.c.

> I added a first pass at API documentation as well. This exposed some
> additional front-end TODOs that I added inline, but they should
> probably be dealt with independently of the refactor:
> - Zero-length client responses are legal in the SASL framework;
> currently we use zero as a sentinel for "don't send a response".


> - I don't think it's legal for a client to refuse a challenge from the
> server without aborting the exchange, so we should probably check to
> make sure that client responses are non-NULL in the success case.

Hmm. Does the RFCs tell us anything about that?

Attachment Content-Type Size
v4-0001-Generalize-SASL-exchange-code-for-the-backend-and.patch text/x-diff 37.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey Borodin 2021-07-05 08:50:19 Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)
Previous Message David Rowley 2021-07-05 08:00:39 Re: [PATCH] expand the units that pg_size_pretty supports on output