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

Hi,

On 9/12/22 9:55 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 9/10/22 1:21 AM, Jacob Champion wrote:
>> On 8/19/22 01:12, Drouvot, Bertrand wrote:
>>> + wstr = palloc((strlen(tok->string + 1) + 1) * sizeof(pg_wchar));
>>> + wlen = pg_mb2wchar_with_len(tok->string + 1,
>>> + wstr, strlen(tok->string + 1));
>> The (tok->string + 1) construction comes up often enough that I think it
>> should be put in a `regex` variable or similar. That would help my eyes
>> with the (strlen(tok->string + 1) + 1) construction, especially.
>>
>> I noticed that for pg_ident, we precompile the regexes per-line and
>> reuse those in child processes. Whereas here we're compiling, using, and
>> then discarding the regex for each check. I think the example set by the
>> pg_ident code is probably the one to follow, unless you have a good
>> reason not to.
>
> Thanks for the feedback.
>
> Yeah fully agree. I'll provide a new version that follow the same logic
> as the pg_ident code.
>
>>> +# Testing with regular expression for username
>>> +reset_pg_hba($node, '/^.*md.*$', 'password');
>>> +test_role($node, 'md5_role', 'password from pgpass and regular expression for username', 0);
>>> +
>> IMO the coverage for this patch needs to be filled out. Negative test
>> cases are more important than positive ones for security-related code.
>
> Agree, will do.
>
>> Other than that, and Tom's note on potentially expanding this to other
>> areas,
>
> I'll add regexp usage for the database column and also the for the
> address one when non CIDR is provided (so host name(s)) (I think it also
> makes sense specially as we don't allow multiple values for this column).
>

Please find attached v2 addressing the comments mentioned above.

v2 also provides regular expression usage for the database and the
address columns (when a host name is being used).

Remark:

The CF bot is failing for Windows (all other tests are green) and only
for the new tap test related to the regular expression on the host name
(the ones on database and role are fine).

The issue is not related to the patch. The issue is that the Windows
Cirrus test does not like when a host name is provided for a "host"
entry in pg_hba.conf (while it works fine when a CIDR is provided).

You can see an example in [1] where the only change is to replace the
CIDR by "localhost" in 002_scram.pl. As you can see the Cirrus tests are
failing on Windows only (its log file is here [2]).

I'll look at this "Windows" related issue but would appreciate any
guidance/help if someone has experience in this area on windows.

[1]: https://github.com/bdrouvot/postgres/branches on branch “host_non_cidr”

[2]:
https://api.cirrus-ci.com/v1/artifact/task/6507279833890816/log/src/test/ssl/tmp_check/log/002_scram_primary.log

Regards,

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