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

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
scram_get_mechanisms().

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".

Check.

> - 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?
--
Michael