From: Jacob Champion <pchampion@vmware.com>
To: michael@paquier.xyz <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Pull general SASL framework out of SCRAM
Date: Tue, 6 Jul 2021 18:20:49 +0000
Message-ID: <39a6bd825fd776f5f9c3a8eb7ca1c62f77cea433.camel@vmware.com> (raw)
In-Reply-To: <YOLAIngluq5wOmgE@paquier.xyz>
References: <3d2a6f5d50e741117d6baf83eb67ebf1a8a35a11.camel@vmware.com>
<YNLlBncQGBIAMWho@paquier.xyz>
<9dc553fe86ba34bff1e0b636c493b527fc7bc030.camel@vmware.com>
<YNZ5Bqq4GvhNvbp0@paquier.xyz>
<0907fda41fb40b23e2cbdf7eb05a8d61dd5307b3.camel@vmware.com>
<YOLAIngluq5wOmgE@paquier.xyz>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pchampion@vmware.com, michael@paquier.xyz
Subject: Re: [PATCH] Pull general SASL framework out of SCRAM
In-Reply-To: <39a6bd825fd776f5f9c3a8eb7ca1c62f77cea433.camel@vmware.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox