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

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 99feb0ce9477cdafa9919934f5c6a421e55105d4..e4a535f4a103074d0178cb6844cfe5686f5a69e6 100644 (file)
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -195,7 +195,8 @@ pg_be_scram_init(const char *username, const char *shadow_pass)
                                 * The password looked like a SCRAM verifier, but could not be
                                 * parsed.
                                 */
-                               elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
+                               ereport(LOG,
+                                               (errmsg("invalid SCRAM verifier for user \"%s\"", username)));
                                got_verifier = false;
                        }
                }
@@ -283,11 +284,13 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
        if (inputlen == 0)
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                (errmsg("malformed SCRAM message (empty message)"))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("The message is empty.")));
        if (inputlen != strlen(input))
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                (errmsg("malformed SCRAM message (length mismatch)"))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Message length does not match input length.")));
 
        switch (state->state)
        {
@@ -319,7 +322,8 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
                        if (!verify_final_nonce(state))
                                ereport(ERROR,
                                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                          (errmsg("invalid SCRAM response (nonce mismatch)"))));
+                                                errmsg("invalid SCRAM response"),
+                                                errdetail("Nonce does not match.")));
 
                        /*
                         * Now check the final nonce and the client proof.
@@ -391,14 +395,9 @@ pg_be_scram_build_verifier(const char *password)
 
        /* Generate random salt */
        if (!pg_backend_random(saltbuf, SCRAM_DEFAULT_SALT_LEN))
-       {
-               ereport(LOG,
+               ereport(ERROR,
                                (errcode(ERRCODE_INTERNAL_ERROR),
                                 errmsg("could not generate random salt")));
-               if (prep_password)
-                       pfree(prep_password);
-               return NULL;
-       }
 
        result = scram_build_verifier(saltbuf, SCRAM_DEFAULT_SALT_LEN,
                                                                  SCRAM_DEFAULT_ITERATIONS, password);
@@ -435,7 +434,8 @@ scram_verify_plain_password(const char *username, const char *password,
                /*
                 * The password looked like a SCRAM verifier, but could not be parsed.
                 */
-               elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
+               ereport(LOG,
+                               (errmsg("invalid SCRAM verifier for user \"%s\"", username)));
                return false;
        }
 
@@ -443,7 +443,8 @@ scram_verify_plain_password(const char *username, const char *password,
        saltlen = pg_b64_decode(encoded_salt, strlen(encoded_salt), salt);
        if (saltlen == -1)
        {
-               elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
+               ereport(LOG,
+                               (errmsg("invalid SCRAM verifier for user \"%s\"", username)));
                return false;
        }
 
@@ -582,14 +583,16 @@ read_attr_value(char **input, char attr)
        if (*begin != attr)
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-               (errmsg("malformed SCRAM message (attribute '%c' expected, %s found)",
-                               attr, sanitize_char(*begin)))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Expected attribute '%c' but found %s.",
+                                                  attr, sanitize_char(*begin))));
        begin++;
 
        if (*begin != '=')
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                (errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Expected character = for attribute %c.", attr)));
        begin++;
 
        end = begin;
@@ -669,8 +672,9 @@ read_any_attr(char **input, char *attr_p)
                  (attr >= 'a' && attr <= 'z')))
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                (errmsg("malformed SCRAM message (attribute expected, invalid char %s found)",
-                                                sanitize_char(attr)))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Attribute expected, but found invalid character %s.",
+                                                  sanitize_char(attr))));
        if (attr_p)
                *attr_p = attr;
        begin++;
@@ -678,7 +682,8 @@ read_any_attr(char **input, char *attr_p)
        if (*begin != '=')
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                (errmsg("malformed SCRAM message (expected = in attr %c)", attr))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Expected character = for attribute %c.", attr)));
        begin++;
 
        end = begin;
@@ -795,14 +800,16 @@ read_client_first_message(scram_state *state, char *input)
                default:
                        ereport(ERROR,
                                        (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                        (errmsg("malformed SCRAM message (unexpected channel-binding flag %s)",
-                                                        sanitize_char(*input)))));
+                                        errmsg("malformed SCRAM message"),
+                                        errdetail("Unexpected channel-binding flag %s.",
+                                                          sanitize_char(*input))));
        }
        if (*input != ',')
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                errmsg("malformed SCRAM message (comma expected, got %s)",
-                                               sanitize_char(*input))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Comma expected, but found character %s.",
+                                                  sanitize_char(*input))));
        input++;
 
        /*
@@ -815,8 +822,9 @@ read_client_first_message(scram_state *state, char *input)
        if (*input != ',')
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                errmsg("malformed SCRAM message (unexpected attribute %s in client-first-message)",
-                                               sanitize_char(*input))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Unexpected attribute %s in client-first-message.",
+                                                  sanitize_char(*input))));
        input++;
 
        state->client_first_message_bare = pstrdup(input);
@@ -831,7 +839,7 @@ read_client_first_message(scram_state *state, char *input)
        if (*input == 'm')
                ereport(ERROR,
                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("client requires mandatory SCRAM extension")));
+                                errmsg("client requires an unsupported SCRAM extension")));
 
        /*
         * Read username.  Note: this is ignored.  We use the username from the
@@ -960,7 +968,7 @@ build_server_first_message(scram_state *state)
        int                     encoded_len;
 
        if (!pg_backend_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
-               ereport(COMMERROR,
+               ereport(ERROR,
                                (errcode(ERRCODE_INTERNAL_ERROR),
                                 errmsg("could not generate random nonce")));
 
@@ -1044,14 +1052,16 @@ read_client_final_message(scram_state *state, char *input)
        if (pg_b64_decode(value, strlen(value), client_proof) != SCRAM_KEY_LEN)
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                (errmsg("malformed SCRAM message (malformed proof in client-final-message"))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Malformed proof in client-final-message.")));
        memcpy(state->ClientProof, client_proof, SCRAM_KEY_LEN);
        pfree(client_proof);
 
        if (*p != '\0')
                ereport(ERROR,
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
-                                (errmsg("malformed SCRAM message (garbage at end of client-final-message)"))));
+                                errmsg("malformed SCRAM message"),
+                                errdetail("Garbage found at the end of client-final-message.")));
 
        state->client_final_message_without_proof = palloc(proof - begin + 1);
        memcpy(state->client_final_message_without_proof, input, proof - begin);

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 5b68e3b7a16d3e50cb3427a3b8c7f63092a8c853..081c06a1e6bf4749ddbd785e1ff4ff1b8c85f3e3 100644 (file)
--- 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)));