[PATCH] Support using "all" for the db user in pg_ident.conf

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

@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.

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

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

Yeah, even after 0001 it's definitely suboptimal. I tried to keep the changes
minimal to not distract from the main purpose of this patch. But I'll update
the patch to have some more. I'll respond to your other question first

In what is your proposal different from the following
entry in pg_ident.conf? As of:
mapname /^(.*)$ \1

It's very different. I think easiest is to explain by example:

If there exist three users on the postgres server: admin, jelte and michael

Then this rule (your suggested rule):
mapname /^(.*)$ \1

Is equivalent to:
mapname admin admin
mapname jelte jelte
mapname michael michael

While with the "all" keyword you can create a rule like this:
mapname admin all

which is equivalent to:
mapname admin admin
mapname admin jelte
mapname admin michael

On Wed, Jan 11, 2023 at 09:04:56AM +0000, Jelte Fennema wrote:

It's very different. I think easiest is to explain by example:

If there exist three users on the postgres server: admin, jelte and michael

Then this rule (your suggested rule):
mapname /^(.*)$ \1

Is equivalent to:
mapname admin admin
mapname jelte jelte
mapname michael michael

While with the "all" keyword you can create a rule like this:
mapname admin all

which is equivalent to:
mapname admin admin
mapname admin jelte
mapname admin michael

Thanks for the explanation, I was missing your point. Hmm. On top
of my mind, couldn't we also use a regexp for the pg-role rather than
just a hardcoded keyword here then, so as it would be possible to
allow a mapping to pass for a group of role names? "all" is just a
pattern to allow everything, at the end.
--
Michael

couldn't we also use a regexp for the pg-role rather than
just a hardcoded keyword here then, so as it would be possible to
allow a mapping to pass for a group of role names? "all" is just a
pattern to allow everything, at the end.

That's a good point. I hadn't realised that you added support for
regexes in pg_hba.conf in 8fea868. Attached is a patchset
where I reuse the pg_hba.conf code path to add support to
pg_ident.conf for: all, +group and regexes.

The main uncertainty I have is if the case insensitivity check is
actually needed in check_role. It seems like a case insensitive
check against the database user shouldn't actually be necessary.
I only understand the need for the case insensitive check against
the system user. But I have too little experience with LDAP/kerberos
to say for certain. So for now I kept the existing behaviour to
not regress.

The patchset also contains 3 preparatory patches with two refactoring
passes and one small bugfix + test additions.

- renaming "systemuser" to "system_user_token" to outline that this is
not a simple string but an AuthToken with potentially a regexp?

I decided against this, since now both system user and database user
are tokens. Furthermore, compiler warnings should avoid any confusion
against using this as a normal string. If you feel strongly about this
though, I'm happy to change this.

On Wed, Jan 11, 2023 at 03:22:35PM +0100, Jelte Fennema wrote:

The main uncertainty I have is if the case insensitivity check is
actually needed in check_role. It seems like a case insensitive
check against the database user shouldn't actually be necessary.
I only understand the need for the case insensitive check against
the system user. But I have too little experience with LDAP/kerberos
to say for certain. So for now I kept the existing behaviour to
not regress.

                if (!identLine->pg_user->quoted &&
+                       identLine->pg_user->string[0] != '+' &&
+                       !token_is_keyword(identLine->pg_user, "all") &&
+                       !token_has_regexp(identLine->pg_user) &&
If we finish by allowing a regexp for the PG user in an IdentLine, I
would choose to drop "all" entirely.  Simpler is better when it comes
to authentication, though we are working on getting things more..
Complicated.

+   Quoting a <replaceable>database-username</replaceable> containing
+   <literal>\1</literal> makes the <literal>\1</literal>
+   lose its special meaning.
0002 and 0003 need careful thinking.

+# Success as the regular expression matches and \1 is replaced
+reset_pg_ident($node, 'mypeermap', qq{/^$system_user(.*)\$},
+       'test\1mapuser');
+test_role(
+       $node,
+       qq{testmapuser},
+       'peer',
+       0,
+       'with regular expression in user name map with \1',
+       log_like =>
+         [qr/connection authenticated: identity="$system_user" method=peer/]);
Relying on kerberos to check the substitution pattern is a bit
annoying..  I would be really tempted to extract and commit that
independently of the rest, actually, to provide some coverage of the
substitution case in the peer test.

The patchset also contains 3 preparatory patches with two refactoring
passes and one small bugfix + test additions.

Applied 0001, which looked fine and was an existing issue. At the
end, I had no issues with the names you suggested.
--
Michael

Simpler is better when it comes to authentication

I definitely agree with that, and if we didn't have existing
parsing logic for pg_hba.conf I would agree. But given the existing
logic for pg_hba.conf, I think the path of least surprises is to
support all of the same patterns that pg_hbac.conf supports.

It also makes the code simpler as we can simply reuse the
check_role function, since that. I removed the lines you quoted
since those are actually not strictly necessary. They only change
the detection logic a bit in case of a \1 existing in the string.
And I'm not sure what the desired behaviour is for those.

I would be really tempted to extract and commit that
independently of the rest, actually, to provide some coverage of the
substitution case in the peer test.

I split up that patch in two parts now and added the tests in a new 0001
patch.

0002 and 0003 need careful thinking.

0002 should change no behaviour, since it simply stores the token in
the IdentLine struct, but doesn't start using the quoted or the regex field
yet. 0003 is debatable indeed. To me it makes sense conceptually, but
having a literal \1 in a username seems like an unlikely scenario and
there might be pg_ident.conf files in existence where the \1 is quoted
that would break because of this change. I haven't removed 0003 from
the patch set yet, but I kinda feel that the advantage is probably not
worth the risk of breakage.

0004 adds some breakage too. But there I think the advantages far outweigh
the risk of breakage. Both because added functionality is a much bigger
advantage, and because we only risk breaking when there exist users that
are called "all", start with a literal + or start with a literal /.
Only "all" seems
like a somewhat reasonable username, but such a user existing seems
unlikely to me given all its special meaning in pg_hba.conf. (I added this
consideration to the commit message)

The main uncertainty I have is if the case insensitivity check is
actually needed in check_role. It seems like a case insensitive
check against the database user shouldn't actually be necessary.
I only understand the need for the case insensitive check against
the system user. But I have too little experience with LDAP/kerberos
to say for certain. So for now I kept the existing behaviour to
not regress.

You didn't write a response about this, but you did quote it. Did you intend
to respond to it?

Applied 0001

Awesome :)

Finally, one separate thing I noticed is that regcomp_auth_token only
checks the / prefix, but doesn't check if the token was quoted or not.
So even if it's quoted it will be interpreted as a regex. Maybe we should
change that? At least for the regex parsing that is not released yet.

On Thu, Jan 12, 2023 at 10:10:02AM +0100, Jelte Fennema wrote:

It also makes the code simpler as we can simply reuse the
check_role function, since that. I removed the lines you quoted
since those are actually not strictly necessary. They only change
the detection logic a bit in case of a \1 existing in the string.
And I'm not sure what the desired behaviour is for those.

Hmm. This is a very good point. 0004 gets really easy to follow
now.

I split up that patch in two parts now and added the tests in a new 0001
patch.

Thanks, applied 0001.

0002 should change no behaviour, since it simply stores the token in
the IdentLine struct, but doesn't start using the quoted or the regex field
yet. 0003 is debatable indeed. To me it makes sense conceptually, but
having a literal \1 in a username seems like an unlikely scenario and
there might be pg_ident.conf files in existence where the \1 is quoted
that would break because of this change. I haven't removed 0003 from
the patch set yet, but I kinda feel that the advantage is probably not
worth the risk of breakage.

0003 would allow folks to use \1 in a Postgres username if quoted. My
choice would be to agree with you here. Even if folks applying quotes
would not be able anymore to replace the pattern, the risk seems a bit
remote? I would suspect that basically everybody does not rely on
'\1' being in the middle of pg-username string, using it only as a
strict replacement of the result coming from system-username to keep a
simpler mapping between the PG roles and the krb5/gss system roles.
Even if they use a more complex schema that depends on strstr(),
things would break if they began the pg-username with quotes. Put it
simply, I'd agree with your 0003.

0004 adds some breakage too. But there I think the advantages far outweigh
the risk of breakage. Both because added functionality is a much bigger
advantage, and because we only risk breaking when there exist users that
are called "all", start with a literal + or start with a literal /.
Only "all" seems
like a somewhat reasonable username, but such a user existing seems
unlikely to me given all its special meaning in pg_hba.conf. (I added this
consideration to the commit message)

I don't see how much that's different from the recent discussion with
regexps added for databases and users to pg_hba.conf. And consistency
sounds pretty good to me here.

Finally, one separate thing I noticed is that regcomp_auth_token only
checks the / prefix, but doesn't check if the token was quoted or not.
So even if it's quoted it will be interpreted as a regex. Maybe we should
change that? At least for the regex parsing that is not released yet.

regcomp_auth_token() should not decide to compile a regexp depending
on if an AuthToken is quoted or not. Regexps can have commas, and
this would impact the case of database or role lists in HBA entries.
And that could be an issue with spaces as well? See the docs for
patterns like:
db1,"/^db\d{2,4}$",db2

Point taken that we don't care about lists for pg_ident entries,
though.

You didn't write a response about this, but you did quote it. Did you intend
to respond to it?

Nah, I should have deleted it. I had no useful opinion on this
particular point.
--
Michael

Even if folks applying quotes
would not be able anymore to replace the pattern, the risk seems a bit
remote?

Yeah I agree the risk is remote. To be clear, the main pattern I'm
worried about breaking is simply "\1". Where people had put
quotes around \1 for no reason. All in all, I'm fine if 0003 gets
merged, but I'd also be fine with it if it doesn't. Both the risk
and the advantage seem fairly small.

I don't see how much that's different from the recent discussion with
regexps added for databases and users to pg_hba.conf. And consistency
sounds pretty good to me here.

It's not much different, except that here also all and + change their meaning
(for pg_hba.conf those special cases already existed). Mainly I called it out
because I realised this discussion was called out in that commit too.

Regexps can have commas

That's a really good reason to allow quoted regexes indeed. Even for pg_ident
entries, commas in unquoted regexes would cause the AuthToken parsing to fail.

Is there anything you still want to see changed about any of the patches?

On Fri, Jan 13, 2023 at 09:19:10AM +0100, Jelte Fennema wrote:

Even if folks applying quotes
would not be able anymore to replace the pattern, the risk seems a bit
remote?

Yeah I agree the risk is remote. To be clear, the main pattern I'm
worried about breaking is simply "\1". Where people had put
quotes around \1 for no reason. All in all, I'm fine if 0003 gets
merged, but I'd also be fine with it if it doesn't. Both the risk
and the advantage seem fairly small.

Still, I am having a few second thoughts about 0003 after thinking
about it over the weekend. Except if I am missing something, there
are no issues with 0004 if we keep the current behavior of always
replacing \1 even if pg-user is quoted? I would certainly add a new
test case either way.

I don't see how much that's different from the recent discussion with
regexps added for databases and users to pg_hba.conf. And consistency
sounds pretty good to me here.

It's not much different, except that here also all and + change their meaning
(for pg_hba.conf those special cases already existed). Mainly I called it out
because I realised this discussion was called out in that commit too.

Regexps can have commas

That's a really good reason to allow quoted regexes indeed. Even for pg_ident
entries, commas in unquoted regexes would cause the AuthToken parsing to fail.

Is there anything you still want to see changed about any of the patches?

+           /*
+            * Mark the token as quoted, so it will only be compared literally
+            * and not for special meanings like, such as "all" and membership
+            * checks using the + prefix.
+            */
+           expanded_pg_user_token = make_auth_token(expanded_pg_user, true);
It is critical to quote this AuthToken after the replacement, indeed.
Or we are in big trouble.

-           /* no substitution, so copy the match */
-           expanded_pg_user = pstrdup(identLine->pg_user->string);
+           expanded_pg_user_token = identLine->pg_user;
Perhaps it would be simpler to use copy_auth_token() in this code path
and always free the resulting token?

In the code path where system-user is a regexp, could it be better
to skip the replacement of \1 in the new AuthToken if pg-user is
itself a regexp? The compiled regexp would be the same, but it could
be considered as a bit confusing, as it can be thought that the
compiled regexp of pg-user happened after the replacement?

No issues with 0002 after a second look, so applied to move on.
--
Michael

Still, I am having a few second thoughts about 0003 after thinking
about it over the weekend. Except if I am missing something, there
are no issues with 0004 if we keep the current behavior of always
replacing \1 even if pg-user is quoted? I would certainly add a new
test case either way.

Yes, 0004 is not dependent on 003 at all. I attached a new version
of 0003 where only a test and some documentation is added.

Perhaps it would be simpler to use copy_auth_token() in this code path
and always free the resulting token?

I initially tried that when working on the patch, but copy_auth_token
(surprisingly) doesn't copy the regex field into the new AuthToken.
So we'd have to regenerate it conditionally. Making the copy
conditional seemed just as simple code-wise, with the added
bonus that it's not doing a useless copy.

In the code path where system-user is a regexp, could it be better
to skip the replacement of \1 in the new AuthToken if pg-user is
itself a regexp? The compiled regexp would be the same, but it could
be considered as a bit confusing, as it can be thought that the
compiled regexp of pg-user happened after the replacement?

I updated 0004 to prioritize membership checks and regexes over
substitution of \1. I also added tests for this. Prioritizing "all" over
substitution of \1 is not necessary, since by definition "all" does
not include \1.

On Mon, Jan 16, 2023 at 11:53:57AM +0100, Jelte Fennema wrote:

Perhaps it would be simpler to use copy_auth_token() in this code path
and always free the resulting token?

I initially tried that when working on the patch, but copy_auth_token
(surprisingly) doesn't copy the regex field into the new AuthToken.
So we'd have to regenerate it conditionally. Making the copy
conditional seemed just as simple code-wise, with the added
bonus that it's not doing a useless copy.

Okay, I can live with that.

In the code path where system-user is a regexp, could it be better
to skip the replacement of \1 in the new AuthToken if pg-user is
itself a regexp? The compiled regexp would be the same, but it could
be considered as a bit confusing, as it can be thought that the
compiled regexp of pg-user happened after the replacement?

I updated 0004 to prioritize membership checks and regexes over
substitution of \1. I also added tests for this. Prioritizing "all" over
substitution of \1 is not necessary, since by definition "all" does
not include \1.

Thanks, 0003 is OK, so applied now.

0004 looks fine as well, be it for the tests (I am hesitating to tweak
things a bit here actually for the role names), the code or the docs,
still I am planning a second lookup.
--
Michael

0004 looks fine as well, be it for the tests (I am hesitating to tweak
things a bit here actually for the role names), the code or the docs,

Anything I can do to help with this? Or will you do that yourself?

On Wed, Jan 18, 2023 at 10:35:29AM +0100, Jelte Fennema wrote:

Anything I can do to help with this? Or will you do that yourself?

Nope. I just need some time to finish wrapping it, that's all.
--
Michael

On Wed, Jan 18, 2023 at 10:35:29AM +0100, Jelte Fennema wrote:

Anything I can do to help with this? Or will you do that yourself?

So, I have done a second lookup, and tweaked a few things:
- Addition of a macro for pg_strcasecmp(), to match with
token_matches().
- Fixed a bit the documentation.
- Tweaked some comments and descriptions in the tests, I was rather
fine with the role and group names.

Jelte, do you like this version?
--
Michael

Looks good to me. One tiny typo in a comment that I noticed when going
over the diff:

+            * Mark the token as quoted, so it will only be compared literally
+            * and not for some special meaning, such as "all" or a group
+            * membership checks.

should be either:
1. a group membership check
2. group membership checks

Now it's mixed singular and plural.

On Thu, Jan 19, 2023 at 12:23:16PM +0100, Jelte Fennema wrote:

should be either:
1. a group membership check
2. group membership checks

Now it's mixed singular and plural.

Thanks, fixed. And now applied the last patch.
--
Michael