On Wed, Jul 07, 2021 at 03:07:14PM +0000, 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(&conn->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.

Attachment: signature.asc
Description: PGP signature

Reply via email to