Content-Type: multipart/alternative;
	boundary="------------D1upOor4jK6ib39PqL7s0AIZ"
Message-ID: <808019e5-5b28-4771-72fb-99e8013fc05e@amazon.com>
Date: Mon, 12 Sep 2022 09:55:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.2
Subject: Re: Patch proposal: make use of regular expressions for the username
 in pg_hba.conf
To: Jacob Champion <jchampion@timescale.com>, PostgreSQL Hackers
	<pgsql-hackers@lists.postgresql.org>
References: <fff0d7c1-8ad4-76a1-9db3-0ab6ec338bf7@amazon.com>
 <42ec83af-21e4-320a-a973-db4ac1ed053e@timescale.com>
Content-Language: en-US
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
In-Reply-To: <42ec83af-21e4-320a-a973-db4ac1ed053e@timescale.com>
Archived-At: 
 <https://www.postgresql.org/message-id/808019e5-5b28-4771-72fb-99e8013fc05e%40amazon.com>
Precedence: bulk

--------------D1upOor4jK6ib39PqL7s0AIZ
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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

Regards,

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

--------------D1upOor4jK6ib39PqL7s0AIZ
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#415ce2" bgcolor="#FFFFFF">
    <p>Hi,<br>
    </p>
    <div class="moz-cite-prefix">On 9/10/22 1:21 AM, Jacob Champion
      wrote:
    </div>
    <blockquote type="cite"
      cite="mid:42ec83af-21e4-320a-a973-db4ac1ed053e@timescale.com">
      <pre class="moz-quote-pre" wrap="">
On 8/19/22 01:12, Drouvot, Bertrand wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+           wstr = palloc((strlen(tok-&gt;string + 1) + 1) * sizeof(pg_wchar));
+           wlen = pg_mb2wchar_with_len(tok-&gt;string + 1,
+                                       wstr, strlen(tok-&gt;string + 1));
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
The (tok-&gt;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-&gt;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.
</pre>
    </blockquote>
    <p>Thanks for the feedback.<br>
    </p>
    <p>Yeah fully agree. I'll provide a new version that follow the same
      logic as the pg_ident code.<br>
    </p>
    <blockquote type="cite"
      cite="mid:42ec83af-21e4-320a-a973-db4ac1ed053e@timescale.com">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+# 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);
+
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
IMO the coverage for this patch needs to be filled out. Negative test
cases are more important than positive ones for security-related code.</pre>
    </blockquote>
    <p>Agree, will do.<br>
    </p>
    <blockquote type="cite"
      cite="mid:42ec83af-21e4-320a-a973-db4ac1ed053e@timescale.com">
      <pre class="moz-quote-pre" wrap="">

Other than that, and Tom's note on potentially expanding this to other
areas, </pre>
    </blockquote>
    <p>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).</p>
    <p>Regards,<br>
    </p>
    <pre class="moz-signature" cols="72">-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: <a class="moz-txt-link-freetext" href="https://aws.amazon.com">https://aws.amazon.com</a></pre>
  </body>
</html>

--------------D1upOor4jK6ib39PqL7s0AIZ--