Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

Hi,

On 10/18/22 7:51 AM, Michael Paquier wrote:
> 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.

Gotcha, thanks! I was wondering if we shouldn't add a comment about that 
and I see that you've added one in v2, thanks!

BTW, what about adding a new TAP test (dedicated patch) to test the 
behavior in case of errors during the regexes compilation in 
pg_ident.conf and pg_hba.conf (not done yet)? (we could add it once this 
  patch series is done).

> 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 :)

I've had the same thought (and that was what the previous global patch 
was doing). I'm tempted to think that the follow-steps will prove you 
right ;-) (specially if at the end those will be the same error messages 
for databases and roles).

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

Agree, that looks better.

I had a look at your v2, did a few tests and it looks good to me.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com