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

Hi,

On 10/14/22 7:30 AM, Michael Paquier wrote:
> On Wed, Oct 12, 2022 at 08:17:14AM +0200, Drouvot, Bertrand wrote:
>> Indeed, ;-)
>
>
> I have also looked
> at make_auth_token(), and wondered if it could be possible to have this
> routine compile the regexes.

I think that it makes sense.

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

The new attached patch proposal is making use of make_auth_token()
(through copy_auth_token()) in parse_ident_line(), do you see any issue?

>
> The logic in charge of compiling the regular expressions could be
> consolidated more. The patch refactors the logic with
> token_regcomp(), uses it for the user names (ident_user in
> parse_ident_line() from pg_ident.conf), then extended to the hostnames
> (single item) and the role/database names (list possible in these
> cases). This approach looks right to me. Once we plug in an AuthItem
> to IdentLine, token_regcomp could be changed so as it takes an
> AuthToken in input

Right, did it that way in the attached.

> - Only one code path of hba.c should call pg_regcomp() (the patch does
> that), and only one code path should call pg_regexec() (two code paths
> of hba.c do that with the patch, as of the need to store matching
> expression). This should minimize the areas where we call
> pg_mb2wchar_with_len(), for one.

Right.

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

>
> The code could be split to tackle things step-by-step:
> - One refactoring patch to introduce token_regcomp() and
> token_regexec()

Agree. Please find attached v1-0001-token_reg-functions.patch for this
first step.

Regards,

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