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