Re: [PATCH] Pull general SASL framework out of SCRAM - Mailing list pgsql-hackers

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