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

[benchub]

As described in #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.

Fixes #484
Fixes #706

[benchub]

I'm not really sure what's going on with these tests, and I think it's clear I'm flailing around more than enough on github. However, this really does seem to address an outstanding bug with auth_query and configured users.

[JelteF]

Thanks for your work on this! This is indeed quite an annoying bug. I think overall direction is good, but I think we should go a bit further with the change because I dislike the cf_user checks that now proliferate the codebase.

Also, I just created while reviewing this PR #1040 I'd appreciate it if you could take a look at that and let me know if you think that change makes sense.

[benchub]

OK @JelteF, I think I addressed all your points. Let me know what you think.

[emelsimsek]

The current logic that adds those users to the auth_file users (struct AATree user_tree;) seems wrong to me.

I believe the following would be a better approach:

The users in the [user] section are not added to the same list with auth_file users. A new structure is used to maintain them, e.g:

struct AATree cf_user_user_tree;

If a user is not found in auth_file users

  Run auth_query
	  Create a PgUser from the query results
	  find_user_user_config()  ---> finds user in cf_user_user_tree.
	  Update the PgUser from auth_query results with the user config in cf_users.

[benchub]

Sorry @emelsimsek, I'm afraid I'm not seeing what we gain by having two different user trees? My understanding of user_tree is that it is used as the collection of users that pgBouncer knows about. What do we gain by having one set for users whose password comes from auth_query and another set where the password comes from auth_file? Wouldn't we need to check both trees in every place where we are currently checking just one?

[emelsimsek]

I'm afraid I'm not seeing what we gain by having two different user trees?

@benchub My thinking goes like this: The Users section is for overriding user configurations for users that comes from various sources. The users and their authentication tokens could be defined in either auth_file or remote user directories such as LDAP. Therefore for flexibility reasons, the user configurations should be maintained in a different list then auth_file's list.

[benchub]

OK @emelsimsek, I've spent some time trying to understand more of the pgBouncer codebase and I'm afraid I don't understand how to implement what you're suggesting, or even understand enough to see how it would help.

I assume when you say, "remote user directories such as LDAP" you are talking about PAM authentication and using LDAP as a PAM provider. I can appreciate that having user credentials being possibly defined in 3 places (pam, auth_file, or auth_query) could get messy, but the documentation is clear that pam is incompatible with the other two options. And indeed, when I look at the code base, there are multiple times that pgBouncer says "Oh, in the case of PAM, do this all differently" - for example, client.c:set_pool(), admin.c:show_one_fd(), or objects.c:use_server_socket().

It's a fair question to ask if pgBouncer should do it all differently, or if it should, as you seem to be suggesting, be able to use auth_query, auth_file, and pam all at the same time. I would suggest that such a change in behavior is beyond the scope of this bug fix.

More importantly, I don't know how to test pgBouncer using PAM. I don't see any unit tests doing so to work from, I don't have PAM set up in any of my environments, and frankly, while I remember PAM coming to linux almost 30 years ago, I've never set it up in all that time. I'm sure somebody is using it, but it's not me. I'm uncomfortable proposing code I don't have the ability to test.

Less importantly, in many of the places that the (currently only) user_tree is queried, it looks like having additional trees would seem to complicate the code for little gain. For example, objects.c:user_client_socket(), admin.c:admin_setup(), or admin.c:show_one_fd().

[emelsimsek]

OK @emelsimsek, I've spent some time trying to understand more of the pgBouncer codebase and I'm afraid I don't understand how to implement what you're suggesting, or even understand enough to see how it would help.

I assume when you say, "remote user directories such as LDAP" you are talking about PAM authentication and using LDAP as a PAM provider.

Yes, the users may be defined in an LDAP directory and this is integrated into pgbouncer using PAM libraries.

I can appreciate that having user credentials being possibly defined in 3 places (pam, auth_file, or auth_query) could get messy, but the documentation is clear that pam is incompatible with the other two options. And indeed, when I look at the code base, there are multiple times that pgBouncer says "Oh, in the case of PAM, do this all differently" - for example, client.c:set_pool(), admin.c:show_one_fd(), or objects.c:use_server_socket().

auth_file and LDAP are alternatives to each other. users section in the configuration file on the other hand looks like intended to overrride user configurations for particular users.
I am also not sure about the maturity/extent of LDAP support in pgbouncer.

It's a fair question to ask if pgBouncer should do it all differently, or if it should, as you seem to be suggesting, be able to use auth_query, auth_file, and pam all at the same time. I would suggest that such a change in behavior is beyond the scope of this bug fix.

I agree that for getting this functionality work in its simplest form, the code change can be kept limited as you had done.
Especially given the absence of test automation for PAM feature, it may be best to keep it as it is.

More importantly, I don't know how to test pgBouncer using PAM. I don't see any unit tests doing so to work from, I don't have PAM set up in any of my environments, and frankly, while I remember PAM coming to linux almost 30 years ago, I've never set it up in all that time. I'm sure somebody is using it, but it's not me. I'm uncomfortable proposing code I don't have the ability to test.

Less importantly, in many of the places that the (currently only) user_tree is queried, it looks like having additional trees would seem to complicate the code for little gain. For example, objects.c:user_client_socket(), admin.c:admin_setup(), or admin.c:show_one_fd().

[benchub]

Yeah. FWIW I did modify add_pam_user in the same way I modified add_db_user, and the unit test I added does make sure that add_db_user (for auth_query users) does work correctly, so I don't think it's unreasonable to assume pam users also work with this change. But again, I don't know how to test that.

So @JelteF, anything else needed to let this be merged?

[benchub]

@JelteF are you waiting on me for anything?

[goksgie]

I feel like [users] section should be specific to configurations, rather than mixing it with authentication. Thus, @emelsimsek 's suggestion of utilizing different data structures for both makes more sense, as then you would have a clear separation of "configurations" and "authentication". Doing so would allow you to fetch configurations defined in [users] when such connection is being made, if it is not initialized already. I think this would probably be a cleaner solution in terms of maintainability and being future proof.

As a side note, adding self referencing pointer and additional boolean to make this work feels hacky to me. But that is of course up to @JelteF and @emelsimsek to decide.

Edit: some grammar improvements.

Edit-2: If we were to separate them into different structures, I guess it would be a breaking change, deserving a major version bump? Though this would require some compatibility tests when implemented.

[JelteF]

The more I look into this the more I realize how big of a mess our user management currently is. I definitely think we need some distinction between different types of users. It's super confusing to reason about. I'm trying out a few options for this at the moment.

[JelteF]

I created a PR for the user management refactor here: #1052