PAMAuth does not eliminate duplicate PAM services during authentication

The PAMAuth authenticator is only supposed to try authenticating each user with a particular PAM service at most twice--once before reloading the users, and once after reloading the users. The second attempt only happens if the PAM service has changed since the first attempt. See here:

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_client_session.cc#L427

However, it looks like it is actually possible for the PAMAuth authenticator to try a specific PAM service more times than that, because PAMAuth does not eliminate duplicate PAM services during authentication.

For example, let's say that we do the following:

INSTALL SONAME 'auth_pam';

CREATE USER 'alice'@'%' IDENTIFIED VIA pam USING 'mariadb';

GRANT SELECT ON *.* TO 'alice'@'%';

CREATE USER 'alice'@'192.168.1.%' IDENTIFIED VIA pam USING 'mariadb';

GRANT SELECT ON *.* TO 'alice'@'192.168.1.%';

Now we have two distinct user accounts named "alice", and they both use the "mariadb" PAM service.

The PAMAuth authenticator loads user accounts using the following query:

SELECT u.user, u.host, d.db, u.select_priv, u.authentication_string FROM 

mysql.user AS u LEFT JOIN mysql.db AS d ON (u.user = d.user AND u.host = d.host) WHERE 

(u.plugin = 'pam' AND (d.db IS NOT NULL OR u.select_priv = 'Y')) 

UNION 

SELECT u.user, u.host, t.db, u.select_priv, u.authentication_string FROM 

mysql.user AS u LEFT JOIN mysql.tables_priv AS t ON (u.user = t.user AND u.host = t.host) WHERE 

(u.plugin = 'pam' AND t.db IS NOT NULL AND u.select_priv = 'N') 

ORDER BY user;

See here:

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_instance.cc#L219

The results for this specific scenario would be the following:

MariaDB [(none)]> SELECT u.user, u.host, d.db, u.select_priv, u.authentication_string FROM

    -> mysql.user AS u LEFT JOIN mysql.db AS d ON (u.user = d.user AND u.host = d.host) WHERE

    -> (u.plugin = 'pam' AND (d.db IS NOT NULL OR u.select_priv = 'Y'))

    -> UNION

    -> SELECT u.user, u.host, t.db, u.select_priv, u.authentication_string FROM

    -> mysql.user AS u LEFT JOIN mysql.tables_priv AS t ON (u.user = t.user AND u.host = t.host) WHERE

    -> (u.plugin = 'pam' AND t.db IS NOT NULL AND u.select_priv = 'N')

    -> ORDER BY user;

+-------+-------------+------+-------------+-----------------------+

| user  | host        | db   | select_priv | authentication_string |

+-------+-------------+------+-------------+-----------------------+

| alice | 192.168.1.% | NULL | Y           | mariadb               |

| alice | %           | NULL | Y           | mariadb               |

+-------+-------------+------+-------------+-----------------------+

2 rows in set (0.00 sec)

Let's say that the "alice" user tries to log in from a host with an IP address that is something like 192.168.1.10. In this case, the get_pam_user_services() function will tell PAMAuth to try the "mariadb" service twice--once for the 'alice'@'%' user account, and once for the 'alice'@'192.168.1.%' user account. See here:

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_client_session.cc#L304

It's not really helpful to try authenticating with the same service multiple times. This can be especially problematic if the user's environment locks a user's account after so many failed authentication attempts. In environments like that, if MaxScale tries the same bad password multiple times for each login attempt, then that will cause the user account to be locked much sooner.

I think MaxScale should fix this by eliminating duplicate PAM services, so that it only tries each unique service once.

I see two potential ways to fix this that are both pretty easy:

1.) The SQLite query that fetches the PAM services in the get_pam_user_services() function does not currently eliminate duplicates. One way to fix this would be by adding the "DISINCT" keyword to the SQLite query, so that SQLite will remove duplicate PAM services from the results. e.g.:

    string services_query = string("SELECT DISTINCT authentication_string FROM ") + m_instance.m_tablename + " WHERE "

...

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_client_session.cc#L307

2.) The get_pam_user_services() function currently uses a std::vector<std::string> to store the PAM services returned by the SQLite query. C++'s std::vector allows duplicate objects. Another way to fix this would be to switch from std::vector<std::string> to something like std::unordered_set<std::string>, which does not allow duplicates.

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_client_session.hh#L29

https://github.com/mariadb-corporation/MaxScale/blob/maxscale-2.3.11/server/modules/authenticator/PAM/PAMAuth/pam_client_session.cc#L304

http://www.cplusplus.com/reference/unordered_set/unordered_set/