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

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