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

On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote:
> 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.

Looks very good, thanks! A few comments on the docs changes:

> + * Output parameters:
> + *
> + * buf: A StringInfo buffer that the callback should populate with
> + * supported mechanism names. The names are appended into this
> + * StringInfo, separated by '\0' bytes.

Each name must be null-terminated, not just null-separated. That way
the list of names ends with an empty string:

name-one\0 <- added by the mechanism
name-two\0 <- added by the mechanism
\0 <- added by the framework

The way it's worded now, I could see some implementers failing to
terminate the final name because the framework adds a trailing null
already -- but the framework is terminating the list, not the final
name.

> + * init()
> + *
> + * Initializes mechanism-specific state for a connection. This
> + * callback must return a pointer to its allocated state, which will
> + * be passed as-is as the first argument to the other callbacks.
> + * free() is called to release any state resources.

Maybe say "The free() callback is called" to differentiate it from
standard free()?

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

Yeah. I think that additional improvements/refactoring here will come
naturally if clients are ever allowed to negotiate SASL mechanisms in
the future. Doesn't need to happen now.

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

It's possible for conn->sasl to be NULL here, say if the client has
channel_binding=require but connects as a user with an MD5 secret. The
SCRAM TAP tests have one such case.

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

Looks good to me.

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

Just in general terms:

> Each authentication exchange consists of a message from the client to
> the server requesting authentication via a particular mechanism,
> followed by one or more pairs of challenges from the server and
> responses from the client, followed by a message from the server
> indicating the outcome of the authentication exchange. (Note:
> exchanges may also be aborted as discussed in Section 3.5.)

So a challenge must be met with a response, or the exchange must be
aborted. (And I don't think our protocol implementation provides a
client abort message; if something goes wrong, we just tear down the
connection.)

Thanks,
--Jacob