Conversation
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query. Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query. Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
As described in pgbouncer#484, if a user is defined in the [users] section of the config file (perhaps to take advantage of per-user overrides) but then that user is *not* defined in auth_file, pgBouncer currently gets confused when trying to check their password. It sees the user exists, but fails to notice a password was never defined, resulting in no running of auth_query.t Add a test case to catch this. The case successfully fails before this patchset and succeeds after it.
This reduces checks that must be made throughout the code, though it does not reduce the need to know which record is the proper one (user or user->cf_user) to use. Also, pre-emptively make uncrustify happy *before* github has a go at it.
The comments are more accurate, and the unit test now runs against freebsd as well as other OSs.
benchub
left a comment
benchub
left a comment
There was a problem hiding this comment.
I think it's a fine refactor. I do think there's more work to do if this is the direction - a lot of field names that used to be of type PgUser are still named some permutation of "user", even though they are now storing credential info instead of the new GlobalUser type.
include/bouncer.h
Outdated
|
|
||
| PgDatabase *db; /* corresponding database */ | ||
| PgUser *user; /* user logged in as, this field is NULL for peer pools */ | ||
| PgAuthInfo *user; /* user logged in as, this field is NULL for peer pools */ |
There was a problem hiding this comment.
Given GlobalUser as a type, I am tempted to suggest this field should be called auth_info or credentials instead of user, to reduce confusion. (And yes, this implies a fair amount of substitutions elsewhere.)
In general, how do you feel about never using "user" for pgAuthInfo data, and keeping that reserved for GlobalUser types?
There was a problem hiding this comment.
Yeah, it's definitely something I think we should be consistent about and was originally trying to do what you are suggesting. But when I got to the auth_user field, I didn't really like the resulting auth_user_auth_info name.
So I held off on changing everything for now, because maybe it fits better to rename PgAuthInfo to PgUser
There was a problem hiding this comment.
What about s/PgAuthInfo/PgCredentials/, so that you end up with auth_user_credentials or the like?
There was a problem hiding this comment.
I like that suggestion. I'll try that out tomorrow.
There was a problem hiding this comment.
Okay, I finally found the time to do this. Could you take another look? I'm pretty happy with the result.
benchub
left a comment
benchub
left a comment
There was a problem hiding this comment.
It's a lot of changes, but it looks good enough at this point that I unleashed my inner grammar nazi for some nits. Assuming the unit tests pass I think it's good enough to get it out there.
doc/config.md
Outdated
|
|
||
| Only a few settings are available here. | ||
|
|
||
| Note that when `auth_file` configured, if a user is defined in this section but |
There was a problem hiding this comment.
...auth_file is configured...
include/bouncer.h
Outdated
| bool dynamic_passwd; /* does the password need to be refreshed every use */ | ||
|
|
||
| /* | ||
| * global_user points at the global user that is used for configuration |
There was a problem hiding this comment.
...global user which is used...
include/bouncer.h
Outdated
|
|
||
| /* | ||
| * The global user is used for configuration settings and connection count. It | ||
| * includes credentials, but this are empty if the user is not configured in |
After the refactoring of the user managament in #1052, we incorrectly freed credentials by calling `slab_free` on a user_cache, instead of `credentials_cache` in two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead.
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in #1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes #1116 Fixes #1103 Fixes #502 Fixes #314 Fixes #90 Resolves #1117 Resolves #1106 Related to #817 Related to #1052 Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com>
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in pgbouncer#1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes pgbouncer#1116 Fixes pgbouncer#1103 Fixes pgbouncer#502 Fixes pgbouncer#314 Fixes pgbouncer#90 Resolves pgbouncer#1117 Resolves pgbouncer#1106 Related to pgbouncer#817 Related to pgbouncer#1052 Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com>
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in pgbouncer#1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes pgbouncer#1116 Fixes pgbouncer#1103 Fixes pgbouncer#502 Fixes pgbouncer#314 Fixes pgbouncer#90 Resolves pgbouncer#1117 Resolves pgbouncer#1106 Related to pgbouncer#817 Related to pgbouncer#1052 Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com>
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in #1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes #1116 Fixes #1103 Fixes #502 Resolves #1117 Resolves #1106 Related to #817 Related to #1052 --------- Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com>
After the refactoring of the user managament in pgbouncer#1052, we incorrectly freed credentials by calling `slab_free` on a user_cache, instead of `credentials_cache` in two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead. (cherry picked from commit 6c8a1df)
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in pgbouncer#1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes pgbouncer#1116 Fixes pgbouncer#1103 Fixes pgbouncer#502 Resolves pgbouncer#1117 Resolves pgbouncer#1106 Related to pgbouncer#817 Related to pgbouncer#1052 --------- Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com> (cherry picked from commit f51bd95)
After the refactoring of the user managament in #1052, we incorrectly freed credentials by calling `slab_free` on a user_cache, instead of `credentials_cache` in two places. This could later cause segfaults because ththe user type is larger than the credentials type, so out of bound reads would be possible when the credential that was put into the user_cache was taken out again and used as a user instead. (cherry picked from commit 6c8a1df)
For a long time we've had issues where users with the same named in the config would cause strange "put_in_order" errors. These errors happened in even more cases following the user management refactor that was done in #1052. Luckily this same user management refactor made fixing the root-cause very easy: By moving the pool_list to the global user struct from the credential struct we now ensure that a single pool is only added once. Fixes #1116 Fixes #1103 Fixes #502 Resolves #1117 Resolves #1106 Related to #817 Related to #1052 --------- Co-authored-by: Andrew Jackson <andrewjackson947@gmail.com> Co-authored-by: Dmitry Kovalenko <dmitry.lipetsk@gmail.com> (cherry picked from commit f51bd95)
@JelteF - There are auth modes (e.g. PAM) that pass along client credentials, not just Example use case: passing a dynamic JWT as the client credential. This is how Microsoft Entra (formerly Azure Active Directory) does authentication for Azure Postgres. A PAM module can work in tandem with PgBouncer to ensure that the client credential is valid even when dynamic. NOTE: it is up to the admin to ensure the JWT expiration is longer than the maximum server connection age. |
3 participants
Based upon #1039, but changed a lot by splitting the
PgUsertype into two different types. First we introduce aPgGlobalUsertype which stores user settings from the ini file, credentials inauth_fileand connection count tracking. And secondly we introducePgCredentialswhich only stores user credentials, these could either come from theauth_fileorauth_query. This type is embedded in thePgGlobalUserto store any global credentials fromauth_file, and it also links back to the matchingPgGlobalUser.This makes user configuration work for dynamically created users and will also list dynamically created users in the output of
SHOW USERS.Fixes #484
Fixes #706
Resolves #1039