Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf - Mailing list pgsql-hackers
From | Drouvot, Bertrand |
---|---|
Subject | Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf |
Date | |
Msg-id | 72e14f66-a454-36e4-9e00-01b77e0b46c4@gmail.com Whole thread Raw |
In response to | Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf ("Drouvot, Bertrand" <bdrouvot@amazon.com>) |
Responses |
Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: