Message-ID: <5000f886-e640-b284-fa64-5cef28b6d272@gmail.com>
Date: Tue, 18 Oct 2022 09:14:21 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.3
Subject: Re: Patch proposal: make use of regular expressions for the username
 in pg_hba.conf
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Jacob Champion <jchampion@timescale.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <Yyk5rfYUTylD8XRg@paquier.xyz> <572017.1663646973@sss.pgh.pa.us>
 <YylB+vmF9dti1Alq@paquier.xyz>
 <e3091081-dc98-7f2b-829f-c247b9337653@gmail.com>
 <Yz0xO0emJ+mxtj2a@paquier.xyz>
 <e346f82f-4a6b-22f7-d6c1-cef16d472de9@gmail.com>
 <Y0UNP4FJyOBn6WX5@paquier.xyz>
 <607af3c9-e45d-3941-41a2-d21065d6594b@gmail.com>
 <Y0jz8QeyeEsnXKlm@paquier.xyz>
 <b5e61748-a38e-60a0-a208-e813269661dc@gmail.com>
 <Y04+/KMwnQszTSK7@paquier.xyz>
Content-Language: en-US
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <Y04+/KMwnQszTSK7@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/5000f886-e640-b284-fa64-5cef28b6d272%40gmail.com>
Precedence: bulk

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