Thread: [PATCH] Support using "all" for the db user in pg_ident.conf
While pg_hba.conf has supported the "all" keyword since a very long time, pg_ident.conf doesn't have this same functionality. This changes permission checking in pg_ident.conf to handle "all" differently from any other value in the database-username column. If "all" is specified and the system-user matches the identifier, then the user is allowed to authenticate no matter what user it tries to authenticate as. This change makes it much easier to have a certain database administrator peer or cert authentication, that allows connecting as any user. Without this change you would need to add a line to pg_ident.conf for every user that is in the database. In some small sense this is a breaking change if anyone is using "all" as a user currently and has pg_ident.conf rules for it. This seems unlikely, since "all" was already handled specially in pg_hb.conf. Also it can easily be worked around by quoting the all token in pg_ident.conf. As long as this is called out in the release notes it seems okay to me. However, if others disagree there would be the option of changing the token to "pg_all". Since any pg_ prefixed users are reserved by postgres there can be no user. For now I used "all" though to stay consistent with pg_hba.conf.
Attachment
On Tue, 27 Dec 2022 at 10:54, Jelte Fennema <Jelte.Fennema@microsoft.com> wrote:
This change makes it much easier to have a certain database
administrator peer or cert authentication, that allows connecting as
any user. Without this change you would need to add a line to
pg_ident.conf for every user that is in the database.
In some small sense this is a breaking change if anyone is using "all"
as a user currently and has pg_ident.conf rules for it. This seems
unlikely, since "all" was already handled specially in pg_hb.conf.
Also it can easily be worked around by quoting the all token in
pg_ident.conf. As long as this is called out in the release notes
it seems okay to me. However, if others disagree there would
be the option of changing the token to "pg_all". Since any
pg_ prefixed users are reserved by postgres there can be no user.
For now I used "all" though to stay consistent with pg_hba.conf.
+1 from me. I recently was setting up a Vagrant VM for testing and wanted to allow the OS user which runs the application to connect to the database as whatever user it wants and was surprised to find I had to list all the potential target DB users in the pg_ident.conf (in production it uses password authentication and each server gets just the passwords it needs stored in ~/.pgpass). I like the idea that both config files would be consistent, although the use of keywords such as "replication" in the DB column has always made me a bit uncomfortable.
Related question: is there a reason why pg_ident.conf can't/shouldn't be replaced by a system table? As far as I can tell, it's just a 3-column table, essentially, with all columns in the primary key. This latest proposal changes that a little; strictly, it should probably introduce a second table with just two columns identifying which OS users can connect as any user, but existing system table style seems to suggest that we would just use a special value in the DB user column for "all".
On Tue, Dec 27, 2022 at 03:54:46PM +0000, Jelte Fennema wrote: > This change makes it much easier to have a certain database > administrator peer or cert authentication, that allows connecting as > any user. Without this change you would need to add a line to > pg_ident.conf for every user that is in the database. That seems pretty dangerous to me. For one, how does this work in cases where we expect the ident entry to be case-sensitive, aka authentication methods where check_ident_usermap() and check_usermap() use case_insensitive = false? Anyway, it is a bit confusing to see a patch touching parts of the ident code related to the system-username while it claims to provide a mean to shortcut a check on the database-username. If you think that some renames should be done to IdentLine, these ought to be done first. -- Michael
Attachment
@Michael > Anyway, it is a bit confusing to see a patch touching parts of the > ident code related to the system-username while it claims to provide a > mean to shortcut a check on the database-username. That's totally fair, I attached a new iteration of this patchset where this refactoring and the new functionality are split up in two patches. > That seems pretty dangerous to me. For one, how does this work in > cases where we expect the ident entry to be case-sensitive, aka > authentication methods where check_ident_usermap() and check_usermap() > use case_insensitive = false? I'm not sure if I'm understanding your concern correctly, but the system username will still be compared case sensitively if requested. The only thing this changes is that: before comparing the pg_role column to the requested pg_role case sensitively, it now checks if the value of the pg_role column is lowercase "all". If that's the case, then the pg_role column is not compared to the requested pg|role anymore, and instead access is granted. @Isaac > is there a reason why pg_ident.conf can't/shouldn't be replaced by a system table? I'm not sure of the exact reason, maybe someone else can clarify this. But even if it could be replaced by a system table, I think that should be a separate patch from this patch.
Attachment
On Wed, Dec 28, 2022 at 09:11:05AM +0000, Jelte Fennema wrote: > That's totally fair, I attached a new iteration of this patchset where this > refactoring and the new functionality are split up in two patches. The confusion that 0001 is addressing is fair (cough, fc579e1, cough), still I am wondering whether we could do a bit better to be more consistent with the lines of the ident file, as of: - renaming "pg_role" to "pg_user", as we use pg-username or database-username when referring to it in the docs or the conf sample file. - renaming "systemuser" to "system_user_token" to outline that this is not a simple string but an AuthToken with potentially a regexp? - Changing the order of the elements in IdentLine to map with the actual ident lines: usermap, systemuser and pg_user. > I'm not sure if I'm understanding your concern correctly, but > the system username will still be compared case sensitively if requested. > The only thing this changes is that: before comparing the pg_role > column to the requested pg_role case sensitively, it now checks if > the value of the pg_role column is lowercase "all". If that's the case, > then the pg_role column is not compared to the requested > pg|role anymore, and instead access is granted. Yeah, still my spider sense reacts on this proposal, and I think that I know why.. In what is your proposal different from the following entry in pg_ident.conf? As of: mapname /^(.*)$ \1 This would allow everything, and use for the PG user the same user as the one provided by the client. That's what your proposal with "all" does, no? The heart of the problem is that you still require PG roles that have a 1:1 mapping the user names provided by the clients. -- Michael