Fixed connection with unknown stats_users or admin_users which throws…

[wwoytenko]

Description

This PR contains a bug fix for not existed stats or admin users when auth_query is turned on

This fixes #314

Clarification

I have found in my environment unexpected behavior when some user is trying to connect to pgbouncer. As a result pgbouncer instance crashed with error @src/objects.c:316 in function put_in_order(): put_in_order: found existing elem

Debug setting verbose=2 shows me that some that this user is stats.

2023-03-22 11:49:13.588 EET [162420] NOISE C-0x55e9e01f3360: (nodb)/(nouser)@[::1]:33504 read pkt='!' len=82
2023-03-22 11:49:13.588 EET [162420] DEBUG C-0x55e9e01f3360: (nodb)/(nouser)@[::1]:33504 got var: user=stats
2023-03-22 11:49:13.588 EET [162420] DEBUG C-0x55e9e01f3360: (nodb)/(nouser)@[::1]:33504 got var: database=pgbouncer
2023-03-22 11:49:13.588 EET [162420] DEBUG C-0x55e9e01f3360: (nodb)/(nouser)@[::1]:33504 using application_name: psql - [::1]:33504
2023-03-22 11:49:13.588 EET [162420] DEBUG C-0x55e9e01f3360: (nodb)/(nouser)@[::1]:33504 got var: client_encoding=UTF8
2023-03-22 11:49:13.588 EET [162420] FATAL @src/objects.c:334 in function put_in_order(): put_in_order: found existing elem

The psql call is
psql "host=localhost port=6432 dbname=pgbouncer user=stats sslmode=disable"

I looked through pgbouncer.ini and userlixt.txt and found that the stats user was set in pgbouncer.ini with the setting stats_users = stats but it was not in the userlixt.txt file.

So the original behavior (in this case) is:

• Get creds from userlist.txt
• Get cred from auth_query
• Reject the connection if cred was not found or mismatch

It is right when we are connecting to the real database (not admin) or auth_dbname was defined and set up in the [databases] section. But due to we are connecting to the admin DB we are using it as an auth_dbname by default and the source code does not contain any checks about the admin flag. Eventually, before the login query we are adding existing db into the pool and it throws @src/objects.c:316 in function put_in_order(): put_in_order: found existing elem

To repeat this problem use config

pgbouncer.ini

[databases]
;; postgres = host=127.0.0.1 port=5432 dbname=postgres
* = host=127.0.0.1 port=5432

[pgbouncer]
auth_query = SELECT usename, passwd FROM pg_shadow where usename = $1
auth_user = pgbouncer
stats_users = stats
listen_addr = localhost
verbose = 2
admin_users = pgbouncer
auth_file = ./etc/userlist.txt
;; auth_dbname = postgres

userlist.txt

"pgbouncer" "1234"

[JelteF]

Thanks for the contribution. Could you add a regression test for this as well?

[wwoytenko]

@JelteF Surely I'll do this in a few days

[wwoytenko]

I found another case when we set up auth_dbname = pgbouncer we are still able to crash the pgbouncer instance. Going to add additional checks

[wwoytenko]

@JelteF We have done all regression tests and are ready for review

[wwoytenko]

I found another case when we set up auth_dbname = pgbouncer we are still able to crash the pgbouncer instance. Going to add additional checks

The check has been moved to start_auth_query() level - it covers both cases.

[wwoytenko]

Going to add additional checks and tests soon:

1. Check for cf_auth_dbname
2. Check for autodb that it does not contain param auth_dbname check with "pgbouncer" value

[wwoytenko]

@goksgie The PR is ready to review. All fixes mentioned above were added in the last commit.

I should mention that this issue we can meet in any version higher that 1.14, even though I didn’t try to perform the same tests with versions not from master branch. But I almost sure that the root cause is the same.

[goksgie]

I think mostly looks good. Though, I think we should remove the added loop in the "cf_autodb_connstr" parser, set_autodb.

Edit: I think I left the review incorrectly 😢 , please check the comments

[wwoytenko]

Seems that this MR is ready for review.
I have simplified the code changes as much as possible and resolved all the requested threads.
Now pgbouncer checks:

• [pgbouncer] auth_dbname setting during config reloading and does not allow to apply config with reserved auth_dbname
• During auth stage pgbouncer determines the auth_dbname and closes the connection with an error if it is an attempt to connect using auth_dbname=pgbouncer
• Slightly refactored tests. Now we've got only three that cover - global setting reloading, connection with wrong auth_dbname (non-global), and positive cases

[JelteF]

Overall this is very close to merging. Some final feedback. Once that is adressed I think this is good.

[wwoytenko]

Total notes about the last changes:

• Fixed doubled test cases with their description
• Added a few checks for the test_auth_dbname_works_fine test
• Reverted changes in QueryBuilder in utils.py