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

Hi,

On 10/21/22 2:58 AM, Michael Paquier wrote:
> On Wed, Oct 19, 2022 at 10:45:44AM +0200, Drouvot, Bertrand wrote:
>> Please find attached v1-0001-regex-handling-for-db-and-roles-in-hba.patch to
>> implement regexes for databases and roles in hba.
>>
>> It does also contain new regexes related TAP tests and doc updates.
>
> Thanks for the updated version. This is really easy to look at now.
>
>> It relies on the refactoring made in fc579e11c6 (but changes the
>> regcomp_auth_token() parameters so that it is now responsible for emitting
>> the compilation error message (if any), to avoid code duplication in
>> parse_hba_line() and parse_ident_line() for roles, databases and user name
>> mapping).
>
> @@ -652,13 +670,18 @@ check_role(const char *role, Oid roleid, List *tokens)
> [...]
> - if (!tok->quoted && tok->string[0] == '+')
> + if (!token_has_regexp(tok))
> {
> Hmm. Do we need token_has_regexp() here for all the cases? We know
> that the string can begin with a '+', hence it is no regex. The same
> applies for the case of "all". The remaining case is the one where
> the user name matches exactly the AuthToken string, which should be
> last as we want to treat anything beginning with a '/' as a regex. It
> seems like we could do an order like that? Say:
> if (!tok->quoted && tok->string[0] == '+')
> //do
> else if (token_is_keyword(tok, "all"))
> //do
> else if (token_has_regexp(tok))
> //do regex compilation, handling failures
> else if (token_matches(tok, role))
> //do exact match
>
> The same approach with keywords first, regex, and exact match could be
> applied as well for the databases? Perhaps it is just mainly a matter
> of taste,

Yeah, I think it is.

> and it depends on how much you want to prioritize the place
> of the regex over the rest but that could make the code easier to
> understand in the long-run and this is a very sensitive code area,

And agree that your proposal tastes better ;-): it is easier to
understand, v2 attached has been done that way.

> Compiling the expressions for the user and database lists one-by-one
> in parse_hba_line() as you do is correct. However there is a gotcha
> that you are forgetting here: the memory allocations done for the
> regexp compilations are not linked to the memory context where each
> line is processed (named hbacxt in load_hba()) and need a separate
> cleanup.

Oops, right, thanks for the call out!

> In the same fashion as load_ident(), it seems to me that we
> need two extra things for this patch:
> - if !ok (see where we do MemoryContextDelete(hbacxt)), we need to go
> through new_parsed_lines and free for each line the AuthTokens for the
> database and user lists.
> - if ok and new_parsed_lines != NIL, the same cleanup needs to
> happen.

Right, but I think that should be "parsed_hba_lines != NIL".

> My guess is that you could do both the same way as load_ident() does,
> keeping some symmetry between the two code paths.

Right. To avoid code duplication in the !ok/ok cases, the function
free_hba_line() has been added in v2: it goes through the list of
databases and roles tokens and call free_auth_token() for each of them.

> Unifying both into
> a common routine would be sort of annoying as HbaLines uses lists
> within the lists of parsed lines, and IdentLine can have one at most
> in each line.

I agree, and v2 is not attempting to unify them.

> For now, I have made your last patch a bit shorter by applying the
> refactoring of regcomp_auth_token() separately with a few tweaks to
> the comments.

Thanks! v2 attached does apply on top of that.

Regards,

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