On Mon, Oct 17, 2022 at 07:56:02PM +0200, Drouvot, Bertrand wrote:
> On 10/14/22 7:30 AM, Michael Paquier wrote:
>> This approach would not stick with
>> pg_ident.conf though, as we validate the fields in each line when we
>> put our hands on ident_user and after the base validation of a line
>> (number of fields, etc.).
>
> I'm not sure to get the issue here with the proposed approach and
> pg_ident.conf.
My point is about parse_ident_line(), where we need to be careful in
the order of the operations. The macros IDENT_MULTI_VALUE() and
IDENT_FIELD_ABSENT() need to be applied on all the fields first, and
the regex computation needs to be last. Your patch had a subtile
issue here, as users may get errors on the computed regex before the
ordering of the fields as the computation was used *before* the "Get
the PG rolename token" part of the logic.
>> About this last point, token_regexec() does not include
>> check_ident_usermap() in its logic, and it seems to me that it should.
>> The difference is with the expected regmatch_t structures, so
>> shouldn't token_regexec be extended with two arguments as of an array
>> of regmatch_t and the number of elements in the array?
>
> You are right, not using token_regexec() in check_ident_usermap() in the
> previous patch versions was not right. That's fixed in the attached, though
> the substitution (if any) is still outside of token_regexec(), do you think
> it should be part of it? (I think that makes sense to keep it outside of it
> as we wont use the substitution logic for roles, databases and hostname)
Keeping the substition done with the IdentLine's Authtokens outside of
the internal execution routine is fine by me.
While putting my hands on that, I was also wondering whether we should
have the error string generated after compilation within the internal
regcomp() routine, but that would require more arguments to
pg_regcomp() (as of file name, line number, **err_string), and that
looks more invasive than necessary. Perhaps the follow-up steps will
prove me wrong, though :)
A last thing is the introduction of a free() routine for AuthTokens,
to minimize the number of places where we haev pg_regfree(). The gain
is minimal, but that looks more consistent with the execution and
compilation paths.
--
Michael
