Fix shadow auth for users defined under [users] section #706

Justin-Kwan · 2022-04-14T23:44:36Z

Description

When a user is not defined within userlist.txt and exists as a user in the Postgres server, PgBouncer should send an auth query to retrieve the user's md5 password hash in order to log the user in.

However, as outlined in issue: #484, if a user's password is not defined in userlist.txt and the user has overridden configurations under the [users] section in PgBouncer's ini file, the user is unable to login to the Postgres server through PgBouncer.

Root Cause

This happens because any users defined under [users] are parsed and added as new users in loader.c's parse_user. As result, when the real and live user connects, PgBouncer thinks they already exist (not NULL) and skips the shadow authorization process where the user's md5 password hash is queried from pg_shadow.

Fix

This PR introduces a new flag to track whether a user was created from ini file parsing or from a real live login. Then, we determine whether a new user logging in requires shadow authorization.

This fix should now allow PgBouncer operators to use the max_user_connections per user feature.

cc @viggy28

Justin-Kwan · 2022-04-14T23:47:09Z

test/test.sh

@@ -931,6 +933,40 @@ test_password_server() {
 	return 0
 }

+# test password authentication from PgBouncer to PostgreSQL server using
+# auth_query to retrieve user's shadow password from pg_shadow
+test_shadow_password_server_login() {


These test cases are reproducible examples of bug (they fail without the fix): #484

Justin-Kwan · 2022-04-26T05:15:14Z

src/objects.c

+		original_user->is_preconfigured = false;
+
+	tmp_passwd = strdup(login_user->passwd);
+	safe_strcpy(original_user->passwd, tmp_passwd, sizeof(original_user->passwd));


Copy over password of new login user to account for a single user that may have different passwords for multiple databases.

So you are copying every time in case a user has a different password in different databases? I think even with this code that could lead to problems, because the original_user is shared between clients of different databases. So maybe we should just document that a user should not have different passwords for different databases when using auth_query/auth_user.

Updated the PR. find_original_user no longer exists. To support a user logging into different databases with different passwords (usually when databases are on different hosts), each PgDatabase* now stores a list of user passwords.

struct PgDatabase { struct List head; char name[MAX_DBNAME]; /* db name for clients */ struct AATree user_passwds; /* user passwords for this database from auth_query */ ...

struct PgUserPassword { struct AANode tree_node; /* used to attach user password to tree */ char username[MAX_USERNAME]; char passwd[MAX_PASSWORD]; };

Semantically, this should be clearer since it models reality, where each database may have different passwords per user. PgUser* still retains a password field to store a password from auth_file which will be used (forced) by default if it exists, when connecting to any database.

Storing user passwords in each PgDatabase* also conveniently frees (purges) passwords whenever the database is killed in method kill_database to prevent stale logins. This is preferred over storing a list of passwords in PgUser* (which would require another scan to free users' passwords if belonging to a database killed).

This change seems to somehow have been reverted on your branch. Possibly due to a rebase.

JelteF

The new tests that you added also pass with the following simpler version of find_original_user. Are the rest of things that you added to this function really necessary?

/* find original or preconfigured user given the login user */
PgUser *find_original_user(PgUser *login_user)
{
	/*
	 * The original user will either be the current login user, a preconfigured user via
	 * ini file [users] section, a preconfigured user via global auth_file, a PAM user,
	 * a user attached to a database via auth_query, a forced user with user= entry via
	 * ini file [databases] section, or not exist yet if the user has never connected.
	 */
	PgUser *original_user = find_user(login_user->name);

	/* add new login user to reload their configurations at runtime */
	if (original_user == NULL) {
		return login_user;
	}

	/* original user is no longer preconfigured and won't send auth queries on future login */
	if (original_user->is_preconfigured) {
		original_user->is_preconfigured = false;
		if (original_user->passwd[0] == '\0') {
			safe_strcpy(original_user->passwd, login_user->passwd, sizeof(original_user->passwd));
		}
	}

	return original_user;
}

JelteF · 2022-08-11T11:48:49Z

src/objects.c

+	if (original_user->is_preconfigured)
+		original_user->is_preconfigured = false;
+
+	tmp_passwd = strdup(login_user->passwd);


this strdup seems unnecessary, you should be able to copy the password directly from user to user.

find_original_user no longer exists now.

JelteF · 2022-08-11T11:52:41Z

src/objects.c

+	PgUser *original_user = find_user(login_user->name);
+
+	/* add new login user to reload their configurations at runtime */
+	if (original_user == NULL) {


I'm not sure I understand what this is needed for. Can you add a test showing what you mean with the RELOAD behaviour that you want to support? i.e. one that fails without this put_in_order code.

JelteF · 2022-08-11T13:06:53Z

src/objects.c

+		original_user->is_preconfigured = false;
+
+	tmp_passwd = strdup(login_user->passwd);
+	safe_strcpy(original_user->passwd, tmp_passwd, sizeof(original_user->passwd));


So you are copying every time in case a user has a different password in different databases? I think even with this code that could lead to problems, because the original_user is shared between clients of different databases. So maybe we should just document that a user should not have different passwords for different databases when using auth_query/auth_user.

JelteF · 2022-08-11T13:07:15Z

src/objects.c

@@ -387,6 +387,36 @@ PgUser *add_user(const char *name, const char *passwd)
 	return user;
 }

+/* find original or preconfigured user given the login user */
+PgUser *find_original_user(PgUser *login_user)


This function name isn't really descriptive of what this function is doing, especially the put_in_order call.

JelteF · 2022-08-11T14:01:41Z

test/test.sh

+test_shadow_password_server_login() {
+	$have_getpeereid || return 77
+
+	admin "set auth_type='md5'"


Do you think it makes sense to include the same tests also for scram auth?

Justin-Kwan · 2022-08-12T03:59:24Z

Hi @JelteF! Thanks for taking a look at this PR. This PR is actually very outdated, I'm going to repush everything.

Justin-Kwan · 2022-08-12T04:33:17Z

We've been running these PgBouncer changes in primary production for nearly a month now with no issues.

Some more details around refactoring work done here:

Context:

In the OSS PgBouncer, PgUser* may exist in multiple contexts: (see OSS discussion) pgbouncer crashes when user is equal to auth_user in database config #314 (comment)
If a user is defined in userlist.txt or pgbouncer.ini [users] section, then they were stored in global user_tree
Any user not defined in userlist.txt (requiring an auth_query to retrieve the user's password from Postgres) would be stored locally under PgDatabase->user_tree
From this, you can imagine that a single user connecting to multiple databases would be duplicated in memory
This is confusing to understand, because semantically, one user exists across all databases in a single Postgres cluster (host server)
The fragmented storage of PgUser* prevents max_user_connections per user to function correctly, because PgUser* will be loaded from [users] and stored in the global user_tree. set_pool assumes that if a user is in the global tree, it's password exists in auth_file, however this may not be true (if a user with max_user_connections is defined in [users] but it's password is not defined in auth_file)

This PR:

In order to correctly support global PgUser configuration settings (max_user_connections, etc.), all users now reside globally in user_tree to simplify centrally storing/enforcing configuration values against
Refactors PgBouncer to handle multiple passwords per user
PgDatabase->user_passwds contains all passwords for users to log into it (user with multiple passwords rather than multiple user instances)
This approach was chosen to model reality where each database keeps track of all passwords for users all users
It is cleaner to store user to password pairs per database because each user is unique by name (easily identifiable), storing the other way around (database to password pairs in PgUser would be more convoluted because database names aren't unique, and would require appending port, etc)
When the database is periodically deallocated (in kill_database), all passwords will be cleanly freed as well, as opposed to having to make an expensive lookup through all users to find which have passwords for the freed database, and removing those passwords

Justin-Kwan · 2022-08-12T04:42:11Z

src/admin.c


 	/* PAM requires passwords as well since they are not stored externally */
-	if (cf_auth_type == AUTH_PAM && !find_user(sk->login_user->name))
-		password = sk->login_user->passwd;
+	if (is_auth_user_present || is_pam_auth_used)


This is done to prevent unnecessary extra call to find_user.

Justin-Kwan · 2022-08-12T04:44:34Z

src/client.c

@@ -339,13 +343,12 @@ bool set_pool(PgSocket *client, const char *dbname, const char *username, const
 		}
 	} else {
 		client->login_user = find_user(username);
-		if (!client->login_user) {
+		if (!client->login_user || user_requires_auth_query(client->login_user)) {


Existence of login_user does not imply authentication of the user, as the user could exist due to being parsed from [users] section, without a password in auth_file. This motivates the use of method user_requires_auth_query.

Justin-Kwan · 2022-08-12T04:45:53Z

src/client.c

@@ -394,7 +398,9 @@ bool handle_auth_query_response(PgSocket *client, PktHdr *pkt) {
 		}
 		break;
 	case 'D':	/* DataRow */
-		memset(&user, 0, sizeof(user));
+		memset(&real_user, 0, sizeof(real_user));


Code cleanup to use two variables (simpler) rather than partial fields of PgUser* struct.

Justin-Kwan · 2022-08-12T04:47:00Z

src/client.c


-		client->login_user = add_db_user(client->db, user.name, user.passwd);
+		client->login_user = add_user(real_user, "");


We can leave PgUser*->passwd empty here because we've just received the user's password hash from Postgres pg_shadow and store it under the appropriate PgDatabase*. This means that the user's password must not be in auth_file.

Justin-Kwan · 2022-08-12T04:47:58Z

src/client.c

@@ -584,6 +596,7 @@ static bool scram_client_first(PgSocket *client, uint32_t datalen, const uint8_t
 	char *input;
 	int res;
 	PgUser *user = client->login_user;
+	const char *real_passwd = user_password(user, client_database(client));


Lookup user's password from the database.

Justin-Kwan · 2022-08-12T04:49:16Z

src/janitor.c

@@ -703,6 +703,7 @@ void kill_database(PgDatabase *db)
 			kill_pool(pool);
 	}

+	aatree_destroy(&db->user_passwds);


Free user passwords associated to the database being killed.

Justin-Kwan · 2022-08-12T04:51:10Z

src/loader.c

@@ -584,7 +612,8 @@ bool load_auth_file(const char *fn)
 		*p++ = 0; /* tag password end */

 		/* send them away */
-		unquote_add_user(user, password);
+		user = unquote_add_user(username, password);
+		user->from_auth_file = true;


Flag that user was from auth_file to know whether to send off auth_query upon login.

Justin-Kwan · 2022-08-12T04:52:40Z

src/server.c

+void user_passwd_free(struct AANode *node, void *arg)
+{
+	PgUserPassword *user_passwd = container_of(node, PgUserPassword, tree_node);
+	free(user_passwd);


PgUserPassword* allocation/deallocation could be handled by the slab allocator if you prefer.

Justin-Kwan · 2022-08-12T04:55:22Z

test/test.sh

@@ -768,6 +770,22 @@ test_enable_disable() {
 	return 0
 }

+test_database_kill() {


This test ensures that PgUser* still globally exists (with configurations such as max_user_connections) even if a database (which contains one of its passwords) is freed.

Justin-Kwan · 2022-08-12T05:03:31Z

test/test.sh

@@ -986,6 +986,28 @@ test_shadow_password_server_login() {
 	echo "curuser=$curuser"
 	test "$curuser" = "shadowuser2" || return 1

+	# user defined in ini [users] section with good recently changed password from PostgreSQL server


More test cases to ensure that users can still login with new password when it's changed at Postgres.

Justin-Kwan · 2022-08-12T05:21:00Z

include/bouncer.h

@@ -335,6 +336,7 @@ struct PgUser {
 struct PgDatabase {
 	struct List head;
 	char name[MAX_DBNAME];	/* db name for clients */
+	struct AATree user_passwds;	/* user passwords for this database from auth_query */


A hashtable to store user passwords is more ideal but is more difficult as we'd have to pull in glibc.

Justin-Kwan · 2022-08-12T05:22:10Z

src/client.c

@@ -35,38 +35,39 @@ static const char *hdr2hex(const struct MBuf *data, char *buf, unsigned buflen)
 	return bin2hex(bin, dlen, buf, buflen);
 }

-static bool check_client_passwd(PgSocket *client, const char *passwd)
+static bool check_client_passwd(PgSocket *client, const char *provided_passwd)


More cleanup to easily discern between the user's real password (from Postgres) vs client provided password.

Justin-Kwan · 2022-08-12T06:08:48Z

src/server.c

+	if (db == NULL || user == db->forced_user || user->from_auth_file)
+		return (char *)user->passwd;
+
+	node = aatree_search((struct AATree *)&db->user_passwds, (uintptr_t)user->name);


Cast db away from const* here. Alternatively could add const* modifier to signature aatree_search.

JelteF · 2022-08-12T07:25:24Z

Thanks for sharing the updated code, I'll try to look at it soonish. If you rebase with master the Alpine tests should hopefully start passing again. And one of the valgrind tests is currently failing, but seemingly a pretty unrelated test, so maybe it's just a flaky test.

goksgie · 2022-09-13T14:54:29Z

If I am not misinterpreting the final version of this PR, it does not contain the proposed design of maintaining user password in databases. Instead, it seems to be storing users in user_tree and update when new login is attempted. Is that the intended behavior?

Additionally, I do not think user_tree is getting cleared unlike databases. Thus, question becomes, what is the resource implications of this design, especially for those systems that rather have limited resources?

Justin-Kwan · 2022-09-13T21:04:12Z

Please take a look at this commit in our official fork of PgBouncer, this implements the newly proposed design (which has been running smoothly under the load of roughly 20K QPS in production): cloudflare@999133e

Hi @goksgie! In our workloads, tenants typically stay connected for long periods, and so the number of users should not be frequently changing or linearly increasing. Struct allocation should be fairly cheap even for a few thousand PgUser structs. However, if you're still concerned about active heap usage given many many users that become stale frequently (IMO an uncommon usage pattern), an eviction callback could be registered with libevent to periodically purge all users without custom configurations (with amortized cost of traversing all nodes).

I won't have any cycles to work on this PR as I am back in school now.

JelteF

Sorry for taking way too long to look at this again. But looking at the current change it seems that it has reverted back to one of the earliest versions of the PR because find_original_user is back (even though you mentioned that it is removed now).

Other than that:

This needs a rebase and should use the new testing infrastructure
We should probably have some cleanup mechanism for the users like @goksgie described

But mostly it seems problematic that this PR is containing the old code again, which means I cannot review the latest version.

veshant · 2023-10-12T21:00:19Z

Sorry for taking way too long to look at this again. But looking at the current change it seems that it has reverted back to one of the earliest versions of the PR because find_original_user is back (even though you mentioned that it is removed now).

Other than that:

This needs a rebase and should use the new testing infrastructure

We should probably have some cleanup mechanism for the users like @goksgie described

But mostly it seems problematic that this PR is containing the old code again, which means I cannot review the latest version.

It might be worth considering to merge the cf-pgbouncer fork. Probably requires some heavy refactoring but they have some useful fixes and features including the users section.

Justin-Kwan changed the title ~~Allow shadow login for users with custom ini configurations~~ Force shadow auth against users with custom ini configurations Apr 14, 2022

Justin-Kwan commented Apr 14, 2022

View reviewed changes

Justin-Kwan mentioned this pull request Apr 14, 2022

pgbouncer didn't even try auth_query to verify user password #484

Closed

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch from 9b60cca to 35e939c Compare April 15, 2022 00:20

Justin-Kwan mentioned this pull request Apr 16, 2022

Configure and enforce max connections per user at runtime #707

Open

viggy28 approved these changes Apr 18, 2022

View reviewed changes

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch 5 times, most recently from 6b5ec9a to 2b39436 Compare April 21, 2022 21:15

Justin-Kwan commented Apr 26, 2022

View reviewed changes

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch 3 times, most recently from a9e4fdd to f6a718f Compare May 11, 2022 02:33

Justin-Kwan changed the title ~~Force shadow auth against users with custom ini configurations~~ Fix shadow auth for users defined under [users] section May 16, 2022

JelteF reviewed Aug 11, 2022

View reviewed changes

Justin-Kwan commented Aug 12, 2022

View reviewed changes

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch 2 times, most recently from 79382e4 to 2516a9c Compare August 12, 2022 05:16

Justin-Kwan commented Aug 12, 2022

View reviewed changes

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch from 2516a9c to 6303356 Compare August 12, 2022 06:07

Justin-Kwan commented Aug 12, 2022

View reviewed changes

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch from 6303356 to caeebbb Compare August 12, 2022 06:10

Justin-Kwan requested a review from JelteF August 12, 2022 06:26

Justin-Kwan added 4 commits August 12, 2022 01:24

Record if user has custom configurations while parsing ini file

bb88725

Replace login user with preconfigured user to preserve user ini values

ae4e16f

Force shadow auth on existing users added from configuration parsing

b4eedc4

Test shadow auth against users defined in ini users section

c8d3118

Justin-Kwan force-pushed the Justin-Kwan/bugfix/allow_users_section_shadow_login branch from caeebbb to c8d3118 Compare August 12, 2022 08:25

JelteF requested changes Oct 12, 2023

View reviewed changes

JelteF mentioned this pull request Mar 13, 2024

Teach pgBouncer to do auth_query for users defined without a password. #1039

Closed

JelteF mentioned this pull request Apr 18, 2024

Refactor user management #1052

Merged

JelteF closed this in #1052 May 2, 2024

JelteF closed this in b003f7b May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shadow auth for users defined under [users] section #706

Fix shadow auth for users defined under [users] section #706

Justin-Kwan commented Apr 14, 2022 •

edited

Justin-Kwan Apr 14, 2022 •

edited

Justin-Kwan Apr 26, 2022

JelteF Aug 11, 2022

Justin-Kwan Aug 12, 2022 •

edited

JelteF Oct 12, 2023

JelteF left a comment •

edited

JelteF Aug 11, 2022

Justin-Kwan Aug 12, 2022

JelteF Aug 11, 2022

JelteF Aug 11, 2022

JelteF Aug 11, 2022

JelteF Aug 11, 2022

Justin-Kwan commented Aug 12, 2022

Justin-Kwan commented Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022

Justin-Kwan Aug 12, 2022 •

edited

JelteF commented Aug 12, 2022 •

edited

goksgie commented Sep 13, 2022

Justin-Kwan commented Sep 13, 2022 •

edited

JelteF left a comment

veshant commented Oct 12, 2023


		client->login_user = add_db_user(client->db, user.name, user.passwd);
		client->login_user = add_user(real_user, "");

Fix shadow auth for users defined under [users] section #706

Fix shadow auth for users defined under [users] section #706

Conversation

Justin-Kwan commented Apr 14, 2022 • edited

Justin-Kwan Apr 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan commented Aug 12, 2022

Justin-Kwan commented Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Justin-Kwan Aug 12, 2022 • edited

Choose a reason for hiding this comment

JelteF commented Aug 12, 2022 • edited

goksgie commented Sep 13, 2022

Justin-Kwan commented Sep 13, 2022 • edited

JelteF left a comment

Choose a reason for hiding this comment

veshant commented Oct 12, 2023

Justin-Kwan commented Apr 14, 2022 •

edited

Justin-Kwan Apr 14, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

JelteF left a comment •

edited

Justin-Kwan commented Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

Justin-Kwan Aug 12, 2022 •

edited

JelteF commented Aug 12, 2022 •

edited

Justin-Kwan commented Sep 13, 2022 •

edited