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