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

Hi,

On 10/11/22 8:29 AM, Michael Paquier wrote:
> 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?

Oh right, good catch, thanks! Please find attached v6 fixing it.

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

Thanks for the explanation!

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

Agree that both struct are not necessary. In v6, AuthTokenOrRegex has
been removed and the regex has been moved to AuthToken. There is no
is_regex bool anymore, as it's enough to test whether regex is NULL or not.

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

The patch does allow that. For example it happens for the test where we
add a comma in the role name. As we don't rely on a dedicated char to
mark the end of a reg exp (we only rely on / to mark its start) then
allowing (regex_t != NULL && quoted) seems reasonable to me.

>> +# 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 ;)
>

Indeed, ;-)

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

Right.

Regards,

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