Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf - Mailing list pgsql-hackers

On Mon, Oct 10, 2022 at 09:00:06AM +0200, Drouvot, Bertrand wrote:
>      foreach(cell, tokens)
>      {
> [...]
> +        tokreg = lfirst(cell);
> +        if (!token_is_regexp(tokreg))
>          {
> -            if (strcmp(dbname, role) == 0)
> +            if (am_walsender && !am_db_walsender)
> +            {
> +                /*
> +                 * physical replication walsender connections can only match
> +                 * replication keyword
> +                 */
> +                if (token_is_keyword(tokreg->authtoken, "replication"))
> +                    return true;
> +            }
> +            else if (token_is_keyword(tokreg->authtoken, "all"))
>                  return true;

When checking the list of databases in check_db(), physical WAL
senders (aka am_walsender && !am_db_walsender) would be able to accept
regexps, but these should only accept "replication" and never a
regexp, no?  The second check on "replication" placed in the branch
for token_is_regexp() in your patch would be correctly placed, though.
This is kind of special in the HBA logic, coming back to 9.0 where
physical replication and this special role property have been
introduced.  WAL senders have gained an actual database property later
on in 9.4 with logical decoding, keeping "replication" for
compatibility (connection strings can use replication=database to
connect as a non-physical WAL sender and connect to a specific
database).

> +typedef struct AuthToken
> +{
> +    char       *string;
> +    bool        quoted;
> +} AuthToken;
> +
> +/*
> + * Distinguish the case a token has to be treated as a regular
> + * expression or not.
> + */
> +typedef struct AuthTokenOrRegex
> +{
> +    bool        is_regex;
> +
> +    /*
> +     * Not an union as we still need the token string for fill_hba_line().
> +     */
> +    AuthToken  *authtoken;
> +    regex_t    *regex;
> +} AuthTokenOrRegex;

Hmm.  With is_regex to check if a regex_t exists, both structures may
not be necessary.  I have not put my hands on that directly, but if
I guess that I would shape things to have only AuthToken with
(enforcing regex_t in priority if set in the list of elements to check
for a match):
- the string
- quoted
- regex_t
A list member should never have (regex_t != NULL && quoted), right?
Hostnames would never be quoted, as well.

> +# test with a comma in the regular expression
> +reset_pg_hba($node, 'all', '"/^.*5,.*e$"', 'password');
> +test_conn($node, 'user=md5,role', 'password', 'matching regexp for username',
> +    0);

So, we check here that the role includes "5," in its name.  This is
getting fun to parse ;)

>  elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
>  {
> -    plan skip_all => 'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
> +    plan skip_all =>
> +      'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
>  }

Unrelated noise from perltidy.
--
Michael