Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bunch of issues around auth_user #393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

@pinaraf
Copy link

@pinaraf pinaraf commented Jun 14, 2019

When switching a pgbouncer configuration from using userlist.txt files to using auth_query/auth_user, I encountered several issues, all fixed in this PR.

  1. the wait_start field is not always initialized when using auth_query, displaying weird random stats
    Example:
2019-06-14 15:41:07.500 26350 LOG stats: 0 xacts/s, 0 queries/s, in 0 B/s, out 9 B/s, xact 0 us, query 0 us, wait 183224844063 us
  1. the global auth_user configuration variable is mis-handled: it has to be defined before the databases in the .ini file, it can not be defined using the pgbouncer shell unless a reload is applied… This is fixed here by simply not using it from parse_database and instead having it be checked when needed.

  2. when removing users from userlist.txt, the corresponding objects are not removed from memory. Instead, their passwords are emptied. This prevents auth_query from being called since the if is valid only for non-existing users.

close #391

petere added a commit that referenced this issue Jul 31, 2019
When using auth_user, the transition to the CL_WAITING_LOGIN state
would not initialize the client->wait_start field.  This would either
lead to garbage values being recorded, or under assertions enabled it
would crash in activate_client().

(test_auth_user was actually reproducing this problem, but a crash
requires assertions enabled and new memory being all zero, so it was
difficult to catch it.)

Author: @pinaraf

see #393
@petere
Copy link
Contributor

@petere petere commented Jul 31, 2019

I have committed a fix for the first issue, but with the code a bit different. I'll continue to work through the other issues.

@pinaraf
Copy link
Author

@pinaraf pinaraf commented Oct 23, 2019

Hello @petere

Any news regarding merging fixes for the two other issues? To be honest, the total_wait_time was no the one bothering us the most…

Regards

@gplv2
Copy link
Contributor

@gplv2 gplv2 commented Sep 17, 2020

concerning nr 3 of the OP's list:

I like to bump this fix due to walking under a ladder, putting a black cat over my left shoulder ... and oh.. having spent a good 2 days trying to understand why new users in a DB got authenticated using the query and existing ones never went to the database after removing them from the userslist.txt file.

It's what I'm missing to gradually migrate our countless databases to auth_query from a unmanageable userslist.txt file , it would make a transition so much easier and without interruptions. It really seems like a good fix to me and it would even be so much more awesome if the docs actually mentioned that migrating to auth_query has a few drawbacks. Eventually this issue was the one that brought me the A-HAAA moment right before I was about to jump from the bridge.

Thanks to Pierre, I'm still here today to complain and bump stuff! ;-)

Seriously though, it would be a nice one to merge. before it dies from old age that is. Thanks for all the work, pgbouncer is still awesome.

@davidfetter
Copy link

@davidfetter davidfetter commented Sep 18, 2020

Bump!

Delay initialization of db->auth_user until it is really needed.
This way, only the latest, really wanted cf_auth_user value is considered
…ord through auth_query

This fixes the case where you are using a filled-in userlist.txt file, but
want to switch to auth_query without restarting pgbouncer.
Previous behavior:
- cleaning userlist.txt removed the passwords (replaced by '\0')
- the auth_query is not started because the users still existed
- login failed
New behavior:
- if the password is empty, start the auth_query
- when auth_query answer arrives, fill it back in user->passwd if it already exists
@pinaraf
Copy link
Author

@pinaraf pinaraf commented Sep 20, 2020

I've done a rebase so only the remaining parts are shown here.

@davidfetter
Copy link

@davidfetter davidfetter commented Sep 20, 2020

Thanks!

petere added a commit to petere/pgbouncer that referenced this issue Oct 14, 2020
A global auth_user setting was pretty confusing and fragile.  It was
looked up while the configuration was read, so it only worked if it
was set before the [databases] section.  Changing it at run time or
changing and reloading also worked in strange ways, depending on
circumstances, because of the ordering dependency.

To fix, look up the setting not while the configuration file is parsed
but later when we actually need it when authenticating a client
connection.

analysis and fix by @pinaraf (pgbouncer#393), some tweaking by me

closes pgbouncer#391
petere added a commit that referenced this issue Oct 16, 2020
A global auth_user setting was pretty confusing and fragile.  It was
looked up while the configuration was read, so it only worked if it
was set before the [databases] section.  Changing it at run time or
changing and reloading also worked in strange ways, depending on
circumstances, because of the ordering dependency.

To fix, look up the setting not while the configuration file is parsed
but later when we actually need it when authenticating a client
connection.

analysis and fix by @pinaraf (#393), some tweaking by me

closes #391
@petere
Copy link
Contributor

@petere petere commented Oct 16, 2020

The second item has been fixed now. (In the future, I would recommend sending separate pull requests for separate issues. Otherwise it makes reviewing really hard if you don't know what change applies to what issue. More so if the fixes are right next to each other.)

@petere
Copy link
Contributor

@petere petere commented Oct 16, 2020

Looking at the third item, I'm not sure about the fix. Having a user with an empty password is a valid state, so we can't just take that as license to override the user with an auth_query user. I think we'd either need to actually delete the user if it disappears from auth_file, or at least flag it as invalid in a different way.

@gplv2
Copy link
Contributor

@gplv2 gplv2 commented Oct 22, 2020

Hi Petere,

I think deleting the user is correct, I also figured it would be a cool feature to be able to purge all/certain users from memory using a pgbouncer command. It sure would assist debugging auth problems and help transitioning between both auth systems. Thanks for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants