MXS-2479 PAMBackendAuth is more tolerant of extra messages.

The authenticator can now receive additional questions from the server even
after the original password-query.

server/modules/authenticator/PAM/PAMAuth/pam_client_session.cc

@@ -243,9 +243,7 @@ bool validate_pam_password(const string& user, const string& password, const str
 }
 
 PamClientSession::PamClientSession(sqlite3* dbhandle, const PamInstance& instance)
-    : m_state(PAM_AUTH_INIT)
+    : m_dbhandle(dbhandle)
-    , m_sequence(0)
-    , m_dbhandle(dbhandle)
     , m_instance(instance)
 {
 }
@@ -371,7 +369,7 @@ Buffer PamClientSession::create_auth_change_packet() const
     gw_mysql_set_byte3(pData, plen);
     pData += 3;
     *pData++ = m_sequence;
-    *pData++ = 0xfe;                            // AuthSwitchRequest command
+    *pData++ = MYSQL_REPLY_AUTHSWITCHREQUEST;
     memcpy(pData, DIALOG.c_str(), DIALOG_SIZE); // Plugin name
     pData += DIALOG_SIZE;
     *pData++ = DIALOG_ECHO_DISABLED;
@@ -388,7 +386,7 @@ int PamClientSession::authenticate(DCB* dcb)
     if (*ses->user)
     {
         rval = MXS_AUTH_FAILED;
-        if (m_state == PAM_AUTH_INIT)
+        if (m_state == State::INIT)
         {
             /** We need to send the authentication switch packet to change the
              * authentication to something other than the 'mysql_native_password'
@@ -396,11 +394,11 @@ int PamClientSession::authenticate(DCB* dcb)
             Buffer authbuf = create_auth_change_packet();
             if (authbuf.length() && dcb->func.write(dcb, authbuf.release()))
             {
-                m_state = PAM_AUTH_DATA_SENT;
+                m_state = State::ASKED_FOR_PW;
                 rval = MXS_AUTH_INCOMPLETE;
             }
         }
-        else if (m_state == PAM_AUTH_DATA_SENT)
+        else if (m_state == State::PW_RECEIVED)
         {
             /** We sent the authentication change packet + plugin name and the client
              * responded with the password. Try to continue authentication without more
@@ -452,6 +450,7 @@ int PamClientSession::authenticate(DCB* dcb)
             {
                 rval = MXS_AUTH_SUCCEEDED;
             }
+            m_state = State::DONE;
         }
     }
     return rval;
@@ -465,20 +464,22 @@ bool PamClientSession::extract(DCB* dcb, GWBUF* buffer)
 
     switch (m_state)
     {
-    case PAM_AUTH_INIT:
+        case State::INIT:
-        // The buffer doesn't have any PAM-specific data yet
+        // The buffer doesn't have any PAM-specific data yet, as it's the normal HandShakeResponse.
         rval = true;
         break;
 
-    case PAM_AUTH_DATA_SENT:
+        case State::ASKED_FOR_PW:
+        // Client should have responses with password.
         if (store_client_password(dcb, buffer))
         {
+            m_state = State::PW_RECEIVED;
             rval = true;
         }
         break;
 
     default:
-        MXS_ERROR("Unexpected authentication state: %d", m_state);
+        MXS_ERROR("Unexpected authentication state: %d", static_cast<int>(m_state));
         mxb_assert(!true);
         break;
     }

server/modules/authenticator/PAM/PAMAuth/pam_client_session.hh

@@ -36,8 +36,16 @@ private:
     void get_pam_user_services(const DCB* dcb, const MYSQL_session* session, StringVector* services_out);
     maxscale::Buffer create_auth_change_packet() const;
 
-    pam_auth_state     m_state;     /**< Authentication state*/
+    enum class State
-    uint8_t            m_sequence;  /**< The next packet seqence number */
+    {
-    sqlite3* const     m_dbhandle;  /**< SQLite3 database handle */
+        INIT,
-    const PamInstance& m_instance;  /**< Authenticator instance */
+        ASKED_FOR_PW,
+        PW_RECEIVED,
+        DONE
+    };
+
+    State              m_state {State::INIT};   /**< Authentication state*/
+    uint8_t            m_sequence {0};          /**< The next packet seqence number */
+    sqlite3* const     m_dbhandle;              /**< SQLite3 database handle */
+    const PamInstance& m_instance;              /**< Authenticator instance */
 };

server/modules/authenticator/PAM/PAMBackendAuth/pam_backend_auth.cc

@@ -21,8 +21,7 @@
 
 static void* pam_backend_auth_alloc(void* instance)
 {
-    PamBackendSession* pses = new(std::nothrow) PamBackendSession();
+    return new(std::nothrow) PamBackendSession();
-    return pses;
 }
 
 static void pam_backend_auth_free(void* data)

server/modules/authenticator/PAM/PAMBackendAuth/pam_backend_session.cc

@@ -16,148 +16,129 @@
 
 namespace
 {
+
 /**
- * Check that the AuthSwitchRequest packet is as expected. Partially an inverse of
+ * Parse packet type and plugin name from packet data. Advances pointer.
- * create_auth_change_packet() in pam_auth.cc.
  *
- * To deal with arbitrary messages in the packet, this needs to mirror also the server
+ * @param data Data from server. The pointer is advanced.
- * plugin code.
+ * @param end Pointer to after the end of data
- *
+ * @param server_name Server name for logging
- * @param dcb Backend DCB
+ * @return True if all expected fields were parsed
- * @param buffer Buffer containing an AuthSwitchRequest packet
- * @return True on success, false on error
  */
-bool check_auth_switch_request(DCB* dcb, GWBUF* buffer)
+bool parse_authswitchreq(const uint8_t** data, const uint8_t* end, const char* server_name)
 {
-    /**
+    const uint8_t* ptr = *data;
-     * The server PAM plugin sends data usually once, at the moment it gets a prompt-type message
+    if (ptr >= end)
-     * from the api. The "message"-segment may contain multiple messages from the api separated by \n.
-     * MaxScale should ignore this text and search for "Password: " near the end of the message. If
-     * server sends more data, authentication ends in error, but this should only happen if the server
-     * asks multiple questions.
-     *
-     * The AuthSwitchRequest packet:
-     * 4 bytes     - Header
-     * 0xfe        - Command byte
-     * string[NUL] - Auth plugin name, should be "dialog"
-     * byte        - Message type, 2 or 4
-     * string[EOF] - Message(s)
-     *
-     * Authenticators receive complete packets from protocol.
-     */
-
-    // Smallest buffer that is parsed, header + cmd byte.
-    int min_readable_buflen = MYSQL_HEADER_LEN + 1;
-    // The buffer should be small, don't accept big messages since it most likely means a complicated auth
-    // scheme.
-    const int MAX_BUFLEN = 2000;
-
-    // Smallest buffer with every packet component, although the components may be wrong.
-    int min_msg_buflen = min_readable_buflen + 3;
-    // Smallest buffer with all expected data.
-    int min_acceptable_buflen = MYSQL_HEADER_LEN + 1 + DIALOG_SIZE + 1 + PASSWORD.length();
-
-    bool rval = false;
-    const char* srv_name = dcb->server->name;
-    int buflen = gwbuf_length(buffer);
-    if (buflen <= min_readable_buflen || buflen > MAX_BUFLEN)
     {
-        MXB_ERROR("Authentication start packet from '%s' is %i bytes. Expected length of packet is "
+        return false;
-                  "between %i and %i.", srv_name, buflen, min_acceptable_buflen, MAX_BUFLEN);
-        // Lengths between min_readable_buflen and min_acceptable_buflen are checked below.
     }
-    else
-    {
-        uint8_t data[buflen + 1]; // + 1 to ensure that the end has a zero.
-        data[buflen] = 0;
-        gwbuf_copy_data(buffer, 0, buflen, data);
-        uint8_t cmdbyte = data[MYSQL_HEADER_LEN];
-        if (cmdbyte == MYSQL_REPLY_AUTHSWITCHREQUEST)
-        {
-            bool malformed_packet = false;
-            // Correct packet type.
-            if (buflen >= min_msg_buflen)
-            {
-                // Buffer is long enough to contain at least some of the fields. Try to read plugin name.
-                const char* ptr = reinterpret_cast<char*>(data) + min_readable_buflen;
-                const char* end = reinterpret_cast<char*>(data) + buflen;
-                if (strcmp(ptr, DIALOG.c_str()) == 0)
-                {
-                    // Correct plugin.
-                    ptr += DIALOG_SIZE;
-                    if (end - ptr >= 2) // message type + message
-                    {
-                        int msg_type = *ptr++;
-                        if (msg_type == DIALOG_ECHO_ENABLED || msg_type == DIALOG_ECHO_DISABLED)
-                        {
-                            // The rest of the buffer contains a message.
-                            // The server separates messages with linebreaks. Search for the last.
-                            const char* linebrk_pos = strrchr(ptr, '\n');
-                            if (linebrk_pos)
-                            {
-                                int msg_len = linebrk_pos - ptr;
-                                MXS_INFO("Server '%s' PAM plugin sent messages: '%.*s'",
-                                         srv_name, msg_len, ptr);
-                                ptr = linebrk_pos + 1;
-                            }
 
-                            if (ptr == PASSWORD)
+    bool success = false;
-                            {
+    uint8_t cmdbyte = *ptr++;
-                                rval = true;
+    if (cmdbyte == MYSQL_REPLY_AUTHSWITCHREQUEST)
-                            }
+    {
-                            else
+        // Correct packet type.
-                            {
+        if (ptr < end)
-                                MXB_ERROR("'%s' asked for '%s' when '%s was expected.",
+        {
-                                          srv_name, ptr, PASSWORD.c_str());
+            const char* plugin_name = reinterpret_cast<const char*>(ptr);
-                            }
+            if (strcmp(plugin_name, DIALOG.c_str()) == 0)
-                        }
+            {
-                        else
+                // Correct plugin.
-                        {
+                ptr += DIALOG_SIZE;
-                            malformed_packet = true;
+                success = true;
-                        }
-                    }
-                    else
-                    {
-                        malformed_packet = true;
-                    }
-                }
-                else
-                {
-                    MXB_ERROR("'%s' asked for authentication plugin '%s' when '%s' was expected.",
-                              srv_name, ptr, DIALOG.c_str());
-                }
             }
             else
             {
-                malformed_packet = true;
+                MXB_ERROR("'%s' asked for authentication plugin '%s' when '%s' was expected.",
+                          server_name, plugin_name, DIALOG.c_str());
             }
-
-            if (malformed_packet)
-            {
-                MXB_ERROR("Received malformed AuthSwitchRequest-packet from '%s'.", srv_name);
-            }
-        }
-        else if (cmdbyte == MYSQL_REPLY_OK)
-        {
-            // Authentication is already done? Maybe the server authenticated us as the anonymous user. This
-            // is quite insecure. */
-            MXB_ERROR("Authentication to '%s' was complete before it even started, "
-                      "anonymous users may be enabled.", srv_name);
         }
         else
         {
-            MXB_ERROR("Expected AuthSwitchRequest-packet from '%s' but received %#x.", srv_name, cmdbyte);
+            MXB_ERROR("Received malformed AuthSwitchRequest-packet from '%s'.", server_name);
         }
     }
-    return rval;
+    else if (cmdbyte == MYSQL_REPLY_OK)
+    {
+        // Authentication is already done? Maybe the server authenticated us as the anonymous user. This
+        // is quite insecure. */
+        MXB_ERROR("Authentication to '%s' was complete before it even started, anonymous users may "
+                  "be enabled.", server_name);
+    }
+    else
+    {
+        MXB_ERROR("Expected AuthSwitchRequest-packet from '%s' but received %#x.", server_name, cmdbyte);
+    }
+
+    if (success)
+    {
+        *data = ptr;
+    }
+    return success;
 }
-}
+
-PamBackendSession::PamBackendSession()
+/**
-    : m_state(PAM_AUTH_INIT)
+ * Parse prompt type and message text from packet data. Advances pointer.
-    , m_sequence(0)
+ *
+ * @param data Data from server. The pointer is advanced.
+ * @param end Pointer to after the end of data
+ * @param server_name Server name for logging
+ * @return True if all expected fields were parsed
+ */
+bool parse_password_prompt(const uint8_t** data, const uint8_t* end, const char* server_name)
 {
+    const uint8_t* ptr = *data;
+    if (end - ptr < 2) // Need at least message type + message
+    {
+        return false;
+    }
+
+    bool success = false;
+    int msg_type = *ptr++;
+    if (msg_type == DIALOG_ECHO_ENABLED || msg_type == DIALOG_ECHO_DISABLED)
+    {
+        const char* messages = reinterpret_cast<const char*>(ptr);
+        // The rest of the buffer contains a message.
+        // The server separates messages with linebreaks. Search for the last.
+        const char* linebrk_pos = strrchr(messages, '\n');
+        const char* prompt;
+        if (linebrk_pos)
+        {
+            int msg_len = linebrk_pos - messages;
+            MXS_INFO("PAM plugin of '%s' sent message: '%.*s'", server_name, msg_len, messages);
+            prompt = linebrk_pos + 1;
+        }
+        else
+        {
+            prompt = messages; // No additional messages.
+        }
+
+        if (prompt == PASSWORD)
+        {
+            success = true;
+        }
+        else
+        {
+            MXB_ERROR("'%s' asked for '%s' when '%s was expected.", server_name, prompt, PASSWORD.c_str());
+        }
+    }
+    else
+    {
+        MXB_ERROR("'%s' sent an unknown message type %i.", server_name, msg_type);
+    }
+
+    if (success)
+    {
+        *data = ptr;
+    }
+    return success;
 }
 
+}
+
+PamBackendSession::PamBackendSession()
+{}
+
 /**
  * Send password to server
  *
@@ -177,53 +158,134 @@ bool PamBackendSession::send_client_password(DCB* dcb)
 
 bool PamBackendSession::extract(DCB* dcb, GWBUF* buffer)
 {
-    gwbuf_copy_data(buffer, MYSQL_SEQ_OFFSET, 1, &m_sequence);
+    /**
-    m_sequence++;
+     * The server PAM plugin sends data usually once, at the moment it gets a prompt-type message
-    bool rval = false;
+     * from the api. The "message"-segment may contain multiple messages from the api separated by \n.
+     * MaxScale should ignore this text and search for "Password: " near the end of the message. See
+     * https://github.com/MariaDB/server/blob/10.3/plugin/auth_pam/auth_pam.c
+     * for how communication is handled on the other side.
+     *
+     * The AuthSwitchRequest packet:
+     * 4 bytes     - Header
+     * 0xfe        - Command byte
+     * string[NUL] - Auth plugin name, should be "dialog"
+     * byte        - Message type, 2 or 4
+     * string[EOF] - Message(s)
+     *
+     * Additional prompts after AuthSwitchRequest:
+     * 4 bytes     - Header
+     * byte        - Message type, 2 or 4
+     * string[EOF] - Message(s)
+     *
+     * Authenticators receive complete packets from protocol.
+     */
 
-    if (m_state == PAM_AUTH_INIT && check_auth_switch_request(dcb, buffer))
+    // Smallest buffer that is parsed, header + (cmd-byte/msg-type + message).
+    const int min_readable_buflen = MYSQL_HEADER_LEN + 1 + 1;
+    // The buffer should be reasonable size. Large buffers likely mean that the auth scheme is complicated.
+    const int MAX_BUFLEN = 2000;
+    const char* srv_name = dcb->server->name;
+    const int buflen = gwbuf_length(buffer);
+    if (buflen <= min_readable_buflen || buflen > MAX_BUFLEN)
     {
-        rval = true;
+        MXB_ERROR("Received packet of size %i from '%s' during authentication. Expected packet size is "
+                  "between %i and %i.", buflen, srv_name, min_readable_buflen, MAX_BUFLEN);
+        return false;
     }
-    else if (m_state == PAM_AUTH_DATA_SENT)
+
+    uint8_t data[buflen + 1]; // + 1 to ensure that the end has a zero.
+    data[buflen] = 0;
+    gwbuf_copy_data(buffer, 0, buflen, data);
+    m_sequence = data[MYSQL_SEQ_OFFSET] + 1;
+    const uint8_t* data_ptr = data + MYSQL_COM_OFFSET;
+    const uint8_t* end_ptr = data + buflen;
+    bool success = false;
+    bool unexpected_data = false;
+
+    switch (m_state)
     {
-        /** Read authentication response */
+        case State::INIT:
-        if (mxs_mysql_is_ok_packet(buffer))
+        // Server should have sent the AuthSwitchRequest + 1st prompt
+        if (parse_authswitchreq(&data_ptr, end_ptr, srv_name)
+            && parse_password_prompt(&data_ptr, end_ptr, srv_name))
         {
-            MXS_DEBUG("pam_backend_auth_extract received ok packet from '%s'.", dcb->server->name);
+            m_state = State::RECEIVED_PROMT;
-            m_state = PAM_AUTH_OK;
+            success = true;
-            rval = true;
         }
         else
         {
-            MXS_ERROR("Expected ok from server but got something else. Authentication failed.");
+            unexpected_data = true;
         }
+        break;
+
+        case State::PW_SENT:
+        {
+            /** Read authentication response. This is typically either OK packet or ERROR, but can be another
+             *  prompt. */
+            uint8_t cmdbyte = data[MYSQL_COM_OFFSET];
+            if (cmdbyte == MYSQL_REPLY_OK)
+            {
+                MXS_DEBUG("pam_backend_auth_extract received ok packet from '%s'.", srv_name);
+                m_state = State::DONE;
+                success = true;
+            }
+            else if (cmdbyte == MYSQL_REPLY_ERR)
+            {
+                MXS_DEBUG("pam_backend_auth_extract received error packet from '%s'.", srv_name);
+                m_state = State::DONE;
+            }
+            else
+            {
+                // The packet may contain another prompt, try parse it. Currently, it's expected to be
+                // another "Password: ", in the future other setups may be supported.
+                if (parse_password_prompt(&data_ptr, end_ptr, srv_name))
+                {
+                    m_state = State::RECEIVED_PROMT;
+                    success = true;
+                }
+                else
+                {
+                    MXS_ERROR("Expected OK, ERR or PAM prompt from '%s' but received something else. ",
+                              srv_name);
+                    unexpected_data = true;
+                }
+            }
+        }
+        break;
+
+
+        default:
+            // This implicates an error in either PAM authenticator or backend protocol.
+            mxb_assert(!true);
+            unexpected_data = true;
+            break;
     }
 
-    if (!rval)
+    if (unexpected_data)
     {
-        MXS_DEBUG("pam_backend_auth_extract to backend '%s' failed for user '%s'.",
+        MXS_ERROR("Failed to read data from '%s' when authenticating user '%s'.", srv_name, dcb->user);
-                  dcb->server->name,
-                  dcb->user);
     }
-    return rval;
+    return success;
 }
 
 int PamBackendSession::authenticate(DCB* dcb)
 {
     int rval = MXS_AUTH_FAILED;
 
-    if (m_state == PAM_AUTH_INIT)
+    if (m_state == State::RECEIVED_PROMT)
     {
-        MXS_DEBUG("pam_backend_auth_authenticate sending password to '%s'.",
+        MXS_DEBUG("pam_backend_auth_authenticate sending password to '%s'.", dcb->server->name);
-                  dcb->server->name);
         if (send_client_password(dcb))
         {
+            m_state = State::PW_SENT;
             rval = MXS_AUTH_INCOMPLETE;
-            m_state = PAM_AUTH_DATA_SENT;
+        }
+        else
+        {
+            m_state = State::DONE;
         }
     }
-    else if (m_state == PAM_AUTH_OK)
+    else if (m_state == State::DONE)
     {
         rval = MXS_AUTH_SUCCEEDED;
     }

server/modules/authenticator/PAM/PAMBackendAuth/pam_backend_session.hh

@@ -18,9 +18,10 @@
 
 class PamBackendSession
 {
-    PamBackendSession(const PamBackendSession& orig);
-    PamBackendSession& operator=(const PamBackendSession&);
 public:
+    PamBackendSession(const PamBackendSession& orig) = delete;
+    PamBackendSession& operator=(const PamBackendSession&) = delete;
+
     PamBackendSession();
     bool extract(DCB* dcb, GWBUF* buffer);
     int  authenticate(DCB* dcb);
@@ -28,6 +29,14 @@ public:
 private:
     bool send_client_password(DCB* dcb);
 
-    pam_auth_state m_state;     /**< Authentication state*/
+    enum class State
-    uint8_t        m_sequence;  /**< The next packet seqence number */
+    {
+        INIT,
+        RECEIVED_PROMT,
+        PW_SENT,
+        DONE
+    };
+
+    State   m_state {State::INIT}; /**< Authentication state*/
+    uint8_t m_sequence {0};  /**< The next packet sequence number */
 };