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

On Wed, 2021-06-23 at 16:38 +0900, Michael Paquier wrote:
> On Tue, Jun 22, 2021 at 10:37:29PM +0000, Jacob Champion wrote:
> > Currently, the SASL logic is tightly coupled to the SCRAM
> > implementation. This patch untangles the two, by introducing callback
> > structs for both the frontend and backend.
>
> The approach to define and have a set callbacks feels natural.

Good, thanks!

> +/* Status codes for message exchange */
> +#define SASL_EXCHANGE_CONTINUE 0
> +#define SASL_EXCHANGE_SUCCESS 1
> +#define SASL_EXCHANGE_FAILURE 2
>
> It may be better to prefix those with PG_ as they begin to be
> published.

Added in v2.

> +/* Backend mechanism API */
> +typedef void (*pg_be_sasl_mechanism_func)(Port *, StringInfo);
> +typedef void *(*pg_be_sasl_init_func)(Port *, const char *, const
> char *);
> +typedef int (*pg_be_sasl_exchange_func)(void *, const char *, int,
> char **, int *, char **);
> +
> +typedef struct
> +{
> + pg_be_sasl_mechanism_func get_mechanisms;
> + pg_be_sasl_init_func init;
> + pg_be_sasl_exchange_func exchange;
> +} pg_be_sasl_mech;
>
> All this is going to require much more documentation to explain what
> is the role of those callbacks and what they are here for.

Yes, definitely. If the current approach seems generally workable, I'll
get started on that.

> Another thing that is not tackled by this patch is the format of the
> messages exchanged which is something only in SCRAM now. Perhaps it
> would be better to extract the small-ish routines currently in
> fe-auth-scram.c and auth-scram.c that we use to grab values associated
> to an attribute in an exchange message and put them in a central place
> like an auth-sasl.c and fe-auth-sasl.c. This move could also make
> sense for the exising init and continue routines for SASL in
> fe-auth.c.

We can. I recommend waiting for another GS2 mechanism implementation,
though.

The attribute/value encoding is not part of core SASL (see [1] for that
RFC), and OAUTHBEARER is not technically a GS2 mechanism -- though it
makes use of a vestigal GS2 header block, apparently in the hopes that
one day it might become one. So we could pull out the similarities now,
but I'd hate to extract the wrong abstractions and make someone else
untangle it later.

> +static int
> +CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
> +{
> + return SASL_exchange(&pg_be_scram_mech, port, shadow_pass, logdetail);
> +}
> It may be cleaner to live without this thin wrapper. It is a bit
> strange to have a SCRAM API in a file where we want mostly SASL things
> (Okay, uaScram does not count as this is assigned after the HBA
> lookup). Moving any SASL-related things into a separate file may be a
> cleaner option, especially considering that we have a bit more than
> the exchange itself, like message handling.

Heh, I figured that at ~3500 lines, you all just really wanted the
Check* implementations to live in auth.c. :D

I can definitely move it (into, say, auth-sasl.c?). I'll probably do
that in a second commit, though, since keeping it in place during the
refactor makes the review easier IMO.

> +typedef void *(*pg_sasl_init_func)(PGconn *, const char *, const char
> *);
> +typedef void (*pg_sasl_exchange_func)(void *, char *, int, char **,
> int *, bool *, bool *);
> +typedef bool (*pg_sasl_channel_bound_func)(void *);
> +typedef void (*pg_sasl_free_func)(void *);
> +
> +typedef struct
> +{
> + pg_sasl_init_func init;
> + pg_sasl_exchange_func exchange;
> + pg_sasl_channel_bound_func channel_bound;
> + pg_sasl_free_func free;
> +} pg_sasl_mech;
> These would be better into a separate header, with more
> documentation.

Can do. Does libpq-int-sasl.h work as a filename? This should not be
exported to applications.

> It may be more consistent with the backend to name
> that pg_fe_sasl_mech?

Done in v2.

> It looks like there is enough material for a callback able to handle
> channel binding. In the main patch for OAUTHBEARER, I can see for
> example that the handling of OAUTHBEARER-PLUS copied from its SCRAM
> sibling. That does not need to be tackled in the same patch. Just
> noting it on the way.

OAUTHBEARER doesn't support channel binding -- there's no OAUTHBEARER-
PLUS, and there probably won't ever be, given the mechanism's
simplicity -- so I'd recommend that this wait for a second GS2
mechanism implementation, as well.

> > (Note that our protocol implementation provides an "additional data"
> > field for the initial client response, but *not* for the authentication
> > outcome. That seems odd to me, but it is what it is, I suppose.)
>
> You are referring to the protocol implementation as of
> AuthenticationSASLFinal, right?

Yes, but I misremembered. My statement was wrong -- we do allow for
additional data in the authentication outcome from the server.

For AuthenticationSASLFinal, we don't distinguish between "no
additional data" and "additional data of length zero", which IIRC is a
violation of the SASL protocol. That may cause problems with a
theoretical future mechanism implementation, but I don't think it
affects SCRAM. I believe we *do* distinguish between those cases
correctly for the initial client response packet.

Sorry for the confusion; let me double-check again when I have fresh
eyes at the start of the week, before sending you on a goose chase.

> > Regarding that specific TODO -- I think it'd be good for the framework
> > to fail hard if a mechanism tries to send data during a failure
> > outcome, as it probably means the mechanism isn't implemented to spec.
>
> Agreed. That would mean patching libpq to add more safeguards in
> pg_SASL_continue() if I am following correctly.

Right.

Thanks for the review!
--Jacob

[1] https://datatracker.ietf.org/doc/html/rfc5801