> 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.
v4-0003-Only-expand-1-in-pg_ident.conf-when-not-quoted.patch
Description: Binary data
v4-0001-Add-tests-for-1-in-pg_ident.conf-to-0003_peer.pl.patch
Description: Binary data
v4-0002-Store-pg_user-as-AuthToken-in-IdentLine.patch
Description: Binary data
v4-0004-Support-same-user-patterns-in-pg_ident.conf-as-in.patch
Description: Binary data