Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, Jul 13, 2021 at 10:41:01PM +, Jacob Champion wrote: > Just to make sure -- do we want to export the fe-auth-sasl.h header as > opposed to forward-declaring the pg_fe_sasl_mech struct? Installing fe-auth-sasl.h has the advantage to make the internals of the callbacks available to applications playing with the internals. For SASL, it makes things easier to define new mechanisms out of core. > Is the use > case for libpq-int.h just "here, have at the internals, and if you > break it then it's on you"? Yes, it can be useful for applications willing to use the internals of libpq, like in forks. There is no guarantee that this will not break across major version upgrades, so that's up to the user to fix things once they play with the internals. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, 2021-07-13 at 22:41 +, Jacob Champion wrote: > On Tue, 2021-07-13 at 19:31 +0900, Michael Paquier wrote: > > On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > > > > > > I think the new fe-auth-sasl.h file should be installed too. > > > Correction proposal in the attached file (but I'm not sure that fix > > > of Install.pm is correct). > > > > That looks correct to me. I'll check that tomorrow. > > Looks right to me too. I'm currently rebuilding my Windows dev > environment so I haven't been able to double-check that piece of it. (Confirmed that this patch works for me on Windows.) Thanks, --Jacob
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, 2021-07-13 at 19:31 +0900, Michael Paquier wrote: > On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > > I got an error while building one of the extensions. > > /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: > > fatal error: fe-auth-sasl.h: No such file or directory > > #include "fe-auth-sasl.h" > > ^~~~ > > Right. I overlooked the fact that libpq-int.h is installed. Thanks for catching that Mikhail. > > I think the new fe-auth-sasl.h file should be installed too. > > Correction proposal in the attached file (but I'm not sure that fix > > of Install.pm is correct). > > That looks correct to me. I'll check that tomorrow. Looks right to me too. I'm currently rebuilding my Windows dev environment so I haven't been able to double-check that piece of it. Just to make sure -- do we want to export the fe-auth-sasl.h header as opposed to forward-declaring the pg_fe_sasl_mech struct? Is the use case for libpq-int.h just "here, have at the internals, and if you break it then it's on you"? --Jacob
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > I got an error while building one of the extensions. > /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: > fatal error: fe-auth-sasl.h: No such file or directory > #include "fe-auth-sasl.h" > ^~~~ Right. I overlooked the fact that libpq-int.h is installed. > I think the new fe-auth-sasl.h file should be installed too. > Correction proposal in the attached file (but I'm not sure that fix > of Install.pm is correct). That looks correct to me. I'll check that tomorrow. -- Michael signature.asc Description: PGP signature
RE: [PATCH] Pull general SASL framework out of SCRAM
Hello, hackers! I got an error while building one of the extensions. /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: fatal error: fe-auth-sasl.h: No such file or directory #include "fe-auth-sasl.h" ^~~~ I think the new fe-auth-sasl.h file should be installed too. Correction proposal in the attached file (but I'm not sure that fix of Install.pm is correct). Regards, Mikhail A. Kulagin PostgresPro missing_fe-auth-sasl.h.patch Description: Binary data
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, Jul 13, 2021 at 12:01:46AM +, Jacob Champion wrote: > Ah, right. I think the (!done && !success) case is probably indicative > of an API smell, but that's probably something to clean up in a future > pass. Yeah, agreed. I feel that it would should be cleaner to replace those two booleans with a status enum or a bitmask. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Pull general SASL framework out of SCRAM
On Sun, 2021-07-11 at 13:16 +0900, Michael Paquier wrote: > On Fri, Jul 09, 2021 at 11:31:48PM +, Jacob Champion wrote: > > On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: > > > + * outputlen: The length (0 or higher) of the client response > > > buffer, > > > + * invalid if output is NULL. > > > > nitpick: maybe "ignored" instead of "invalid"? > > Thanks, applied as 44bd012 after using your suggestion. Thanks! > Another thing I noticed after more review is that the check in > fe-auth.c to make sure that a message needs to be generated if the > exchange is not completed yet has no need to depend on "success", only > "done". Ah, right. I think the (!done && !success) case is probably indicative of an API smell, but that's probably something to clean up in a future pass. --Jacob
Re: [PATCH] Pull general SASL framework out of SCRAM
On Fri, Jul 09, 2021 at 11:31:48PM +, Jacob Champion wrote: > On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: >> + * outputlen: The length (0 or higher) of the client response >> buffer, >> + * invalid if output is NULL. > > nitpick: maybe "ignored" instead of "invalid"? Thanks, applied as 44bd012 after using your suggestion. Another thing I noticed after more review is that the check in fe-auth.c to make sure that a message needs to be generated if the exchange is not completed yet has no need to depend on "success", only "done". -- Michael signature.asc Description: PGP signature
Re: [PATCH] Pull general SASL framework out of SCRAM
On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: > I agree that this looks like an improvement in terms of the > expectations behind a SASL mechanism, so I have done the attached to > strengthen a bit all those checks. However, I don't really see a > point in back-patching any of that, as SCRAM satisfies with its > implementation already all those conditions AFAIK. Agreed. > Thoughts? LGTM, thanks! > + * outputlen: The length (0 or higher) of the client response > buffer, > + * invalid if output is NULL. nitpick: maybe "ignored" instead of "invalid"? --Jacob
Re: [PATCH] Pull general SASL framework out of SCRAM
On Wed, Jul 07, 2021 at 03:07:14PM +, Jacob Champion wrote:
> That's correct. But the client may not simply ignore the challenge and
> keep the exchange open waiting for a new one, as pg_SASL_continue()
> currently allows. That's what my TODO is referring to.
I have been looking more at your three points from upthread and
feasted on the SASL RFC, as of:
- Detection that no output is generated on PG_SASL_EXCHANGE_FAILURE
for the backend.
- Handling of zero-length messages in the frontend. The backend
handles that already, and SCRAM would complain if sending such
messages, but I can see why you'd want to allow that for other
mechanisms.
- Making sure that a mechanism generates a message in the middle of
the exchange in the frontend.
I agree that this looks like an improvement in terms of the
expectations behind a SASL mechanism, so I have done the attached to
strengthen a bit all those checks. However, I don't really see a
point in back-patching any of that, as SCRAM satisfies with its
implementation already all those conditions AFAIK. So that's an
improvement of the current code, and it fits nicely with the SASL
refactoring for the documentation of the callbacks.
Thoughts?
--
Michael
diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c
index 3e4f763b60..6d25997079 100644
--- a/src/backend/libpq/auth-sasl.c
+++ b/src/backend/libpq/auth-sasl.c
@@ -171,6 +171,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
if (output)
{
+ /*
+ * PG_SASL_EXCHANGE_FAILURE with some output is forbidden by
+ * SASL. Make sure here that the mechanism used got that right.
+ */
+ if (result == PG_SASL_EXCHANGE_FAILURE)
+elog(ERROR, "output message found after SASL exchange failure");
+
/*
* Negotiation generated data to be sent to the client.
*/
diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h
index 0aec588a9e..180f4642aa 100644
--- a/src/interfaces/libpq/fe-auth-sasl.h
+++ b/src/interfaces/libpq/fe-auth-sasl.h
@@ -78,11 +78,12 @@ typedef struct pg_fe_sasl_mech
* Output parameters, to be set by the callback function:
*
* output: A malloc'd buffer containing the client's response to
- * the server, or NULL if the exchange should be aborted.
- * (*success should be set to false in the latter case.)
+ * the server (can be empty), or NULL if the exchange should
+ * be aborted. (*success should be set to false in the
+ * latter case.)
*
- * outputlen: The length of the client response buffer, or zero if no
- * data should be sent due to an exchange failure
+ * outputlen: The length (0 or higher) of the client response buffer,
+ * invalid if output is NULL.
*
* done: Set to true if the SASL exchange should not continue,
* because the exchange is either complete or failed
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index eaba0ba56d..b383a575d3 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -674,7 +674,22 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
return STATUS_ERROR;
}
- if (outputlen != 0)
+ /*
+ * If the exchange is a success but not yet finished, we need to make sure
+ * that the SASL mechanism has generated a message to send back.
+ */
+ if (output == NULL && success && !done)
+ {
+ appendPQExpBufferStr(>errorMessage,
+ libpq_gettext("no client response found after SASL exchange success\n"));
+ return STATUS_ERROR;
+ }
+
+ /*
+ * SASL allows zero-length responses, so this check uses "output" and
+ * not "outputlen" to allow the case of an empty message.
+ */
+ if (output)
{
/*
* Send the SASL response to the server.
signature.asc
Description: PGP signature
Re: [PATCH] Pull general SASL framework out of SCRAM
On Wed, 2021-07-07 at 14:08 +0900, Michael Paquier wrote: > On Tue, Jul 06, 2021 at 06:20:49PM +, Jacob Champion wrote: > > On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote: > > > > > 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. At the same time, section 3.5 also says that the client may > send a message to abort. So one can interpret that the client has > also the choice to abort without sending a response back to the > server? Or I am just interpreting incorrectly the use of "may" in > this context? That's correct. But the client may not simply ignore the challenge and keep the exchange open waiting for a new one, as pg_SASL_continue() currently allows. That's what my TODO is referring to. --Jacob
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, Jul 06, 2021 at 06:20:49PM +, Jacob Champion wrote: > On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote: > 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. Good point. I have used ending with '\0' bytes instead. >> + * 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()? Yes, that could be confusing. Switched to your wording instead. > 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. Indeed. >> 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. At the same time, section 3.5 also says that the client may send a message to abort. So one can interpret that the client has also the choice to abort without sending a response back to the server? Or I am just interpreting incorrectly the use of "may" in this context? -- Michael signature.asc Description: PGP signature
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 +, 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
Re: [PATCH] Pull general SASL framework out of SCRAM
On Wed, Jun 30, 2021 at 10:30:12PM +, 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. 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. So I'd like to apply that to clarify this code layer, without the TODOs. - pg_be_scram_get_mechanisms(port, _mechs); - /* Put another '\0' to mark that list is finished. */ - appendStringInfoChar(_mechs, '\0'); I was wondering for a couple of seconds if it would not be better to let the last '\0' being set within the callback, but what you have here looks better. - 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. 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. > I added a first pass at API documentation as well. This exposed some > additional front-end TODOs that I added inline, but they should > probably be dealt with independently of the refactor: > > - Zero-length client responses are legal in the SASL framework; > currently we use zero as a sentinel for "don't send a response". Check. > - 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? -- Michael From 3bfae6e6f563acfe63f2ae44feb5799f4e49324b Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 5 Jul 2021 17:16:15 +0900 Subject: [PATCH v4] Generalize SASL exchange code for the backend and the frontend --- src/include/libpq/auth.h | 2 + src/include/libpq/sasl.h | 136 +++ src/include/libpq/scram.h| 13 +- src/backend/libpq/Makefile | 1 + src/backend/libpq/auth-sasl.c| 196 +++ src/backend/libpq/auth-scram.c | 51 --- src/backend/libpq/auth.c | 167 +-- src/interfaces/libpq/fe-auth-sasl.h | 130 ++ src/interfaces/libpq/fe-auth-scram.c | 40 -- src/interfaces/libpq/fe-auth.c | 23 +++- src/interfaces/libpq/fe-auth.h | 11 +- src/interfaces/libpq/fe-connect.c| 6 +- src/interfaces/libpq/libpq-int.h | 2 + src/tools/pgindent/typedefs.list | 2 + 14 files changed, 558 insertions(+), 222 deletions(-) create mode 100644 src/include/libpq/sasl.h create mode 100644 src/backend/libpq/auth-sasl.c create mode 100644 src/interfaces/libpq/fe-auth-sasl.h diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h index 3610fae3ff..3d6734f253 100644 --- a/src/include/libpq/auth.h +++ b/src/include/libpq/auth.h @@ -21,6 +21,8 @@ extern bool pg_krb_caseins_users; extern char *pg_krb_realm; extern void ClientAuthentication(Port *port); +extern void sendAuthRequest(Port *port, AuthRequest areq, const char *extradata, + int extralen); /* Hook for plugins to get control in ClientAuthentication() */ typedef void (*ClientAuthentication_hook_type) (Port *, int); diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h new file mode 100644 index 00..f119a62d68 --- /dev/null +++ b/src/include/libpq/sasl.h @@ -0,0 +1,136 @@ +/*- + * + * sasl.h + * Defines the SASL mechanism interface for the backend. + * + * Each SASL mechanism defines a frontend and a backend callback structure. + * + * See src/interfaces/libpq/fe-auth-sasl.h for the frontend counterpart. + * + * Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/libpq/sasl.h + * + *- + */ + +#ifndef PG_SASL_H +#define PG_SASL_H + +#include "lib/stringinfo.h" +#include "libpq/libpq-be.h" + +/* Status codes for message exchange */ +#define PG_SASL_EXCHANGE_CONTINUE 0 +#define PG_SASL_EXCHANGE_SUCCESS 1 +#define PG_SASL_EXCHANGE_FAILURE 2 + +/* + * Backend SASL mechanism callbacks. + * + * To implement a backend mechanism, declare a pg_be_sasl_mech struct with + * appropriate callback implementations. Then pass the mechanism to + * CheckSASLAuth() during ClientAuthentication(), once the server has decided + * which
Re: [PATCH] Pull general SASL framework out of SCRAM
On Sat, 2021-06-26 at 09:47 +0900, Michael Paquier wrote:
> On Fri, Jun 25, 2021 at 11:40:33PM +, Jacob Champion wrote:
> > 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.
>
> auth-sasl.c is a name consistent with the existing practice.
>
> > Can do. Does libpq-int-sasl.h work as a filename? This should not be
> > exported to applications.
>
> I would still with the existing naming used by fe-gssapi-common.h, so
> that would be fe-auth-sasl.c and fe-auth-sasl.h, with the header
> remaining internal. Not strongly wedded to this name, of course, that
> just seems consistent.
Done in v3, with a second patch for the code motion.
I added a first pass at API documentation as well. This exposed some
additional front-end TODOs that I added inline, but they should
probably be dealt with independently of the refactor:
- Zero-length client responses are legal in the SASL framework;
currently we use zero as a sentinel for "don't send a response".
- 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.
--Jacob
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 8d1d16b0fc..6d385fd6a4 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -15,6 +15,7 @@ include $(top_builddir)/src/Makefile.global
# be-fsstubs is here for historical reasons, probably belongs elsewhere
OBJS = \
+ auth-sasl.o \
auth-scram.o \
auth.o \
be-fsstubs.o \
diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c
new file mode 100644
index 00..b7cdb2ecf6
--- /dev/null
+++ b/src/backend/libpq/auth-sasl.c
@@ -0,0 +1,187 @@
+/*-
+ *
+ * auth-sasl.c
+ * Routines to handle network authentication via SASL
+ *
+ * Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/libpq/auth-sasl.c
+ *
+ *-
+ */
+
+#include "postgres.h"
+
+#include "libpq/auth.h"
+#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
+#include "libpq/sasl.h"
+
+/*
+ * Perform a SASL exchange with a libpq client, using a specific mechanism
+ * implementation.
+ *
+ * shadow_pass is an optional pointer to the shadow entry for the client's
+ * presented user name. For mechanisms that use shadowed passwords, a NULL
+ * pointer here means that an entry could not be found for the user (or the
user
+ * does not exist), and the mechanism should fail the authentication exchange.
+ *
+ * Mechanisms must take care not to reveal to the client that a user entry does
+ * not exist; ideally, the external failure mode is identical to that of an
+ * incorrect password. Mechanisms may instead use the logdetail output
parameter
+ * to internally differentiate between failure cases and assist debugging by
the
+ * server admin.
+ *
+ * A mechanism is not required to utilize a shadow entry, or even a password
+ * system at all; for these cases, shadow_pass may be ignored and the caller
+ * should just pass NULL.
+ */
+int
+CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
+ char **logdetail)
+{
+ StringInfoData sasl_mechs;
+ int mtype;
+ StringInfoData buf;
+ void *opaq = NULL;
+ char *output = NULL;
+ int outputlen = 0;
+ const char *input;
+ int inputlen;
+ int result;
+ boolinitial;
+
+ /*
+* Send the SASL authentication request to user. It includes the list
of
+* authentication mechanisms that are supported.
+*/
+ initStringInfo(_mechs);
+
+ mech->get_mechanisms(port, _mechs);
+ /* Put another '\0' to mark that list is finished. */
+ appendStringInfoChar(_mechs, '\0');
+
+ sendAuthRequest(port, AUTH_REQ_SASL, sasl_mechs.data, sasl_mechs.len);
+ pfree(sasl_mechs.data);
+
+ /*
+* Loop through SASL message exchange. This exchange can consist of
+* multiple messages sent in both directions. First message is always
+* from the client. All messages from client to server are password
+* packets (type 'p').
+*/
+ initial = true;
+ do
+ {
+ pq_startmsgread();
+ mtype = pq_getbyte();
+ if (mtype != 'p')
+ {
+ /* Only log error if client didn't disconnect. */
+ if (mtype != EOF)
+
Re: [PATCH] Pull general SASL framework out of SCRAM
On Fri, Jun 25, 2021 at 11:40:33PM +, Jacob Champion wrote: > 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. auth-sasl.c is a name consistent with the existing practice. > Can do. Does libpq-int-sasl.h work as a filename? This should not be > exported to applications. I would still with the existing naming used by fe-gssapi-common.h, so that would be fe-auth-sasl.c and fe-auth-sasl.h, with the header remaining internal. Not strongly wedded to this name, of course, that just seems consistent. -- Michael signature.asc Description: PGP signature
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 +, 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_funcinit;
> + pg_be_sasl_exchange_funcexchange;
> +} 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(_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
Re: [PATCH] Pull general SASL framework out of SCRAM
On Tue, Jun 22, 2021 at 10:37:29PM +, 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_funcinit;
+ pg_be_sasl_exchange_funcexchange;
+} 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(_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
signature.asc
Description: PGP signature
[PATCH] Pull general SASL framework out of SCRAM
(This is split off from my work on OAUTHBEARER [1].)
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.
In the original thread, Michael Paquier commented:
> + /* TODO: SASL_EXCHANGE_FAILURE with output is forbidden in SASL */
> if (result == SASL_EXCHANGE_SUCCESS)
> sendAuthRequest(port,
> AUTH_REQ_SASL_FIN,
> output,
> outputlen);
> Perhaps that's an issue we need to worry on its own? I didn't recall
> this part..
Yeah, it was non-obvious to me on the first read too. It's a
consequence of a couple parts of the SASL spec [2]:
>The protocol may include an optional additional data field in this
>outcome message. This field can only include additional data when
>the outcome is successful.
and
>SASL mechanism specifications MUST supply the following information:
>
>[...]
>
> b) An indication of whether the server is expected to provide
> additional data when indicating a successful outcome. If so,
> if the server sends the additional data as a challenge, the
> specification MUST indicate that the response to this challenge
> is an empty response.
There isn't a corresponding provision for data for a *failed* outcome,
so any such data would have to be sent as a separate, mechanism-
specific, challenge. This is what OAUTHBEARER has to do, with an
annoying "failure handshake".
(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.)
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.
--Jacob
[1]
https://www.postgresql.org/message-id/d1b467a78e0e36ed85a09adf979d04cf124a9d4b.ca...@vmware.com
[2] https://datatracker.ietf.org/doc/html/rfc4422
From a6a65b66cc3dc5da7219378dbadb090ff10fd42b Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 13 Apr 2021 10:25:48 -0700
Subject: [PATCH 1/7] auth: generalize SASL mechanisms
Split the SASL logic out from the SCRAM implementation, so that it can
be reused by other mechanisms. New implementations will implement both
a pg_sasl_mech and a pg_be_sasl_mech.
---
src/backend/libpq/auth-scram.c | 34 ++-
src/backend/libpq/auth.c | 34 ---
src/include/libpq/sasl.h | 34 +++
src/include/libpq/scram.h| 13 +++--
src/interfaces/libpq/fe-auth-scram.c | 40 +++-
src/interfaces/libpq/fe-auth.c | 16 ---
src/interfaces/libpq/fe-auth.h | 11 ++--
src/interfaces/libpq/fe-connect.c| 6 +
src/interfaces/libpq/libpq-int.h | 14 ++
9 files changed, 139 insertions(+), 63 deletions(-)
create mode 100644 src/include/libpq/sasl.h
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index f9e1026a12..db3ca75a60 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -101,11 +101,25 @@
#include "common/sha2.h"
#include "libpq/auth.h"
#include "libpq/crypt.h"
+#include "libpq/sasl.h"
#include "libpq/scram.h"
#include "miscadmin.h"
#include "utils/builtins.h"
#include "utils/timestamp.h"
+static void scram_get_mechanisms(Port *port, StringInfo buf);
+static void *scram_init(Port *port, const char *selected_mech,
+ const char *shadow_pass);
+static int scram_exchange(void *opaq, const char *input, int inputlen,
+ char **output, int *outputlen, char **logdetail);
+
+/* Mechanism declaration */
+const pg_be_sasl_mech pg_be_scram_mech = {
+ scram_get_mechanisms,
+ scram_init,
+ scram_exchange,
+};
+
/*
* Status data for a SCRAM authentication exchange. This should be kept
* internal to this file.
@@ -170,16 +184,14 @@ static char *sanitize_str(const char *s);
static char *scram_mock_salt(const char *username);
/*
- * pg_be_scram_get_mechanisms
- *
* Get a list of SASL mechanisms that this module supports.
*
* For the convenience of building the FE/BE packet that lists the
* mechanisms, the names are appended to the given StringInfo buffer,
* separated by '\0' bytes.
*/
-void
-pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
+static void
+scram_get_mechanisms(Port *port, StringInfo buf)
{
/*
* Advertise the mechanisms in decreasing order of importance. So the
@@ -199,8 +211,6 @@ pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
}
/*
- * pg_be_scram_init
- *
* Initialize a new SCRAM authentication exchange status tracker. This
* needs to be called before doing any exchange. It will be filled
