Обсуждение: Re: [EXTERNAL] Re: [PATCH] Support using "all" for the db user in pg_ident.conf

> 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, 11 Jan 2023 at 14:34, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

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

Hello,

Playing with this patch, I did not see descriptive comments in 
pg_ident.conf.

Does it make sense to reflect changes to the PG-USERNAME field in the 
pg_ident.conf.sample file?

The same relates to the regexp supportin the DATABASE and USER fieldsof 
the pg_hba.conf.sample file(8fea8683).

-----
Pavel Luzanov

On Mon, 13 Feb 2023 at 15:06, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
> Does it make sense to reflect changes to the PG-USERNAME field in the
> pg_ident.conf.sample file?
>
> The same relates to the regexp supportin the DATABASE and USER fieldsof
> the pg_hba.conf.sample file(8fea8683).

That definitely makes sense to me. When writing the patch I didn't
realise that there was also documentation in those files.

I think it also makes sense to include usage of (some of) the features
in the example files here:
https://www.postgresql.org/docs/devel/auth-username-maps.html

On Mon, Feb 13, 2023 at 03:13:04PM +0100, Jelte Fennema wrote:
> On Mon, 13 Feb 2023 at 15:06, Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
>> Does it make sense to reflect changes to the PG-USERNAME field in the
>> pg_ident.conf.sample file?
>>
>> The same relates to the regexp supportin the DATABASE and USER fieldsof
>> the pg_hba.conf.sample file(8fea8683).

Which comes down to blame me for both of them.

> That definitely makes sense to me. When writing the patch I didn't
> realise that there was also documentation in those files.
>
> I think it also makes sense to include usage of (some of) the features
> in the example files here:
> https://www.postgresql.org/docs/devel/auth-username-maps.html

Hmm, I am not sure that adding more examples in the sample files is
worth the duplication with the docs.

So, please find attached a patch to close the gap the sample files,
for both things, with descriptions of all the field values they can
use.

What do you think?
--
Michael

On 15.02.2023 10:11, Michael Paquier wrote:

Which comes down to blame me for both of them.

My only intention was to make postgres better.
I'm sorry you took it that way. 

So, please find attached a patch to close the gap the sample files,
for both things, with descriptions of all the field values they can
use.

A short and precise description. Nothing to add.
Next time I will try to offer a patch at once.

 
-----
Pavel Luzanov

On Wed, Feb 15, 2023 at 01:05:04PM +0300, Pavel Luzanov wrote:
> On 15.02.2023 10:11, Michael Paquier wrote:
>> Which comes down to blame me for both of them.
>
> My only intention was to make postgres better.I'm sorry you took it that
> way.

You have no need to feel sorry about that.  I really appreciate that
you took the time to report this issue, so don't worry.  My point is
that I have committed this code, so basically it is my responsibility
to take care of its maintenance.

>> So, please find attached a patch to close the gap the sample files,
>> for both things, with descriptions of all the field values they can
>> use.
>
> A short and precise description. Nothing to add.Next time I will try to
> offer a patch at once.

If you have a proposal of patch, that's always nice to have, but you
should not feel obliged to do so, either.

Thanks a lot for the report, Pavel!
--
Michael

On Wed, 15 Feb 2023 at 08:11, Michael Paquier <michael@paquier.xyz> wrote:
> Hmm, I am not sure that adding more examples in the sample files is
> worth the duplication with the docs.

I think you misunderstood what I meant (because I admittedly didn't
write it down clearly). I meant the docs for pg_ident don't include
any examples (only descriptions of the new patterns). Attached is a
patch that addresses that.

> So, please find attached a patch to close the gap the sample files,
> for both things, with descriptions of all the field values they can
> use.

LGTM

On Wed, Feb 15, 2023 at 03:40:26PM +0100, Jelte Fennema wrote:
> On Wed, 15 Feb 2023 at 08:11, Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm, I am not sure that adding more examples in the sample files is
>> worth the duplication with the docs.
>
> I think you misunderstood what I meant (because I admittedly didn't
> write it down clearly). I meant the docs for pg_ident don't include
> any examples (only descriptions of the new patterns). Attached is a
> patch that addresses that.

Shouldn't the paragraph above the example file of pg_ident.conf be
updated to reflect the changes you have added?  An idea would be
cleaner to split that into two sections.  For example, we could keep
the current example with bryanh, ann and bob as it is (splitting it
into its own <para>), and add a second example with all the new
patterns?

>> So, please find attached a patch to close the gap the sample files,
>> for both things, with descriptions of all the field values they can
>> use.
>
> LGTM

Thanks for the review, applied this part.
--
Michael