Date: Tue, 19 Nov 2024 19:28:03 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Emanuele Musella <emamuse86@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Parametrization minimum password lenght
Message-ID: <Zzzmw4IAvrypmFO4@ip-10-97-1-34.eu-west-3.compute.internal>
References: <CA+ugDNyYtHOtWCqVD3YkSVYDWD_1fO8Jm_ahsDGA5dXhbDPwrQ@mail.gmail.com>
 <5702ea42-3599-4062-bd15-3e3c7cc67e6e@vondra.me>
 <CA+ugDNwOCK1hNqehay2ycwKXBJQL2A2RXoFEWTwT5TTETPJpTw@mail.gmail.com>
 <9bd48f6d-dec0-48be-8db3-c3b29be588e7@vondra.me>
 <CA+ugDNyPNMqFADM20J6+qeAqmc1=Kxiuff29xz93-3k2zWFu3w@mail.gmail.com>
 <CA+ugDNz7nGan_zjX__ZzDMxnPviaRWrtrNicTZseF4wskbu2hw@mail.gmail.com>
 <ZzsZZY3YrO6hinnT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+ugDNw4KR+j98Lp=GX6gnra=1cshQQuDp5shQw=gBVKZiebHw@mail.gmail.com>
 <CA+ugDNzcEEgcLvC2w-LMjfWXKSb9btEBcRnDTkfgviT_eVEkAA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+ugDNzcEEgcLvC2w-LMjfWXKSb9btEBcRnDTkfgviT_eVEkAA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zzzmw4IAvrypmFO4%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Mon, Nov 18, 2024 at 05:21:18PM +0100, Emanuele Musella wrote:
> We notice some errors on CFBot results.

FWIW, you can run "cfbot like" tests on your own repo (see [1]).

> In attached the errors fixed

Thanks for the updated version!

A few random comments:

=== 1

trailing whitespace:

$ git apply min_password_length_v7.patch
min_password_length_v7.patch:130: trailing whitespace.
  There is a configuration parameter that control the behavior
warning: 1 line adds whitespace errors.

=== 2

+ * Author: Maurizio Boriani <maurizio@boriani.cloud>
+ * Author: Emanuele Musella <emamuse86@gmail.com>

Same comment as in [2].

=== 3

-               int                     pwdlen = strlen(password);
+               int                     pwdlen = pg_mbstrlen(password);

Sorry if I was not clear in [2], but I meant to say to keep using strlen() to be
consistent with the current behavior.

=== 4

+                               GUC_UNIT_BYTE,

this is correct if strlen() is used (see above comment).

=== 5

+                               0, INT_MAX,

INT_MAX seems too large and 0 too low. Maybe we should not allow less than it
was before the patch (8). For the max, maybe something like PG_MAX_AUTH_TOKEN_LENGTH?
(see the comment in src/backend/libpq/auth.c)

=== 6

+  There is a configuration parameter that control the behavior
+  <filename>passwordcheck</filename>

s/behavior/behavior of/?

=== 7

+      <varname>passwordcheck.min_password_length</varname> is the minimum length
+      of accepted password on database users.
+      If not setted the default is 8 bytes.

What about? "is the minimum password length in bytes. The default is 8."

=== 7

+
+<programlisting>
+# postgresql.conf
+session_preload_libraries = 'passwordcheck'
+passwordcheck.min_password_length = 12
+
+</programlisting>

What about a sentence before? Something like for auto_explain means "In ordinary
usage, these parameters are set in postgresql.conf,............"

[1]: https://github.com/postgres/postgres/blob/master/src/tools/ci/README
[2]: https://www.postgresql.org/message-id/ZzsZZY3YrO6hinnT%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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