Improve authentication error messages.

Most of the improvements were in the new SCRAM code:

* In SCRAM protocol violation messages, use errdetail to provide the
  details.

* If pg_backend_random() fails, throw an ERROR rather than just LOG. We
  shouldn't continue authentication if we can't generate a random nonce.

* Use ereport() rather than elog() for the "invalid SCRAM verifier"
  messages. They shouldn't happen, if everything works, but it's not
  inconceivable that someone would have invalid scram verifiers in
  pg_authid, e.g. if a broken client application was used to generate the
  verifier.

But this change applied to old code:

* Use ERROR rather than COMMERROR for protocol violation errors. There's
  no reason to not tell the client what they did wrong. The client might be
  confused already, so that it cannot read and display the error correctly,
  but let's at least try. In the "invalid password packet size" case, we
  used to actually continue with authentication anyway, but that is now a
  hard error.

Patch by Michael Paquier and me. Thanks to Daniel Varrazzo for spotting
the typo in one of the messages that spurred the discussion and these
larger changes.

Discussion: https://www.postgresql.org/message-id/CA%2Bmi_8aZYLhuyQi1Jo0hO19opNZ2OEATEOM5fKApH7P6zTOZGg%40mail.gmail.com

1 files changed, 6 insertions, 9 deletions

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 5b68e3b7a16..081c06a1e6b 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -656,7 +656,7 @@ recv_password_packet(Port *port)
 			 * log.
 			 */
 			if (mtype != EOF)
-				ereport(COMMERROR,
+				ereport(ERROR,
 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
 					errmsg("expected password response, got message type %d",
 						   mtype)));
@@ -684,7 +684,7 @@ recv_password_packet(Port *port)
 	 * StringInfo is guaranteed to have an appended '\0'.
 	 */
 	if (strlen(buf.data) + 1 != buf.len)
-		ereport(COMMERROR,
+		ereport(ERROR,
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
 				 errmsg("invalid password packet size")));
 
@@ -897,11 +897,10 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			/* Only log error if client didn't disconnect. */
 			if (mtype != EOF)
 			{
-				ereport(COMMERROR,
+				ereport(ERROR,
 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
 						 errmsg("expected SASL response, got message type %d",
 								mtype)));
-				return STATUS_ERROR;
 			}
 			else
 				return STATUS_EOF;
@@ -935,11 +934,9 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			selected_mech = pq_getmsgrawstring(&buf);
 			if (strcmp(selected_mech, SCRAM_SHA256_NAME) != 0)
 			{
-				ereport(COMMERROR,
+				ereport(ERROR,
 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
 						 errmsg("client selected an invalid SASL authentication mechanism")));
-				pfree(buf.data);
-				return STATUS_ERROR;
 			}
 
 			inputlen = pq_getmsgint(&buf, 4);
@@ -1144,7 +1141,7 @@ pg_GSS_recvauth(Port *port)
 		{
 			/* Only log error if client didn't disconnect. */
 			if (mtype != EOF)
-				ereport(COMMERROR,
+				ereport(ERROR,
 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
 						 errmsg("expected GSS response, got message type %d",
 								mtype)));
@@ -1384,7 +1381,7 @@ pg_SSPI_recvauth(Port *port)
 		{
 			/* Only log error if client didn't disconnect. */
 			if (mtype != EOF)
-				ereport(COMMERROR,
+				ereport(ERROR,
 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
 						 errmsg("expected SSPI response, got message type %d",
 								mtype)));