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

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