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

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.

+/* 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.

+/* 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.

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.

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

+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. It may be more consistent with the backend to name
that pg_fe_sasl_mech?

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.

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

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