Date: Mon, 18 Nov 2024 10:39:33 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Emanuele Musella <emamuse86@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas@vondra.me>
Subject: Re: Parametrization minimum password lenght
Message-ID: <ZzsZZY3YrO6hinnT@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>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+ugDNz7nGan_zjX__ZzDMxnPviaRWrtrNicTZseF4wskbu2hw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZzsZZY3YrO6hinnT%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Mon, Nov 18, 2024 at 10:01:41AM +0100, Emanuele Musella wrote:
> Hi all,
> 
> we have fixed the following warning from the CFbot:
> 
> [07:14:29.637] passwordcheck.c:37:5: error: no previous extern declaration
> for non-static variable 'min_password_length'
> [-Werror,-Wmissing-variable-declarations]
> 
> 
> In attach the fixed patch.

Thanks for the patch, that looks like a useful feature!

A few random comments:

=== 1

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

I'm not sure we do that usually. For example, in cb3384a0cb, Fabien Coelho has
not been mentioned as an author in contrib/isn/isn.c.

=== 2

 #include "commands/user.h"
 #include "fmgr.h"
 #include "libpq/crypt.h"
+#include "commands/explain.h"
+#include "utils/guc.h"

The "includes" should be in the order based on their ASCII value (see 7e735035f2).

=== 3

+/* min_password_length minimum password length */

I think "/* Minimum password length */ is "enough" for the comment.

Also what about adding "/* GUC variables */" on top of it?

=== 4 (Nit)

+static int min_password_length;

What about "min_pwd_len" to make it consistent with pwd_has_letter and pwd_has_nonletter
for example?

=== 5

        prev_check_password_hook = check_password_hook;
        check_password_hook = check_password;
+
+       /* Define custom GUC variables. */
+       DefineCustomIntVariable("passwordcheck.min_password_length",

What about moving the DefineCustomIntVariable before the hooks? (that would be
consistent with auto_explain.c, auth_delay.c, autoprewarm.c and pg_stat_statements.c
to name a few).

=== 6

+                               "8 is default.",

I don't think that's needed (that would be visible from pg_settings.extra_desc
 while it already provides the information through the boot_val field).

=== 7

+                               GUC_UNIT_MS,

Not the right unit, that should be GUC_UNIT_BYTE because...

=== 8

postgres=# SET passwordcheck.min_password_length = 4;
SET
postgres=# create user test with password 'ab1';
ERROR:  password is too short

But with multibyte in play:

postgres=# create user test with password 'äb1';
CREATE ROLE.

That's because "strlen(password);" returns the number of bytes.

We could make it based on the characters instead by using "pg_mbstrlen()"
but that would break the behavior as compared to now. So, I think we
just need to make it clear that's bytes instead.

=== 9

I think that a call to MarkGUCPrefixReserved() is missing (see auto_explain.c
for example).

=== 10

+ <para>
+  In <filename>postgresql.conf</filename> you may set the minimum password length
+  by setting passwordcheck.min_password_length = INT.

I think that would make sense to add a "Configuration Parameters" section and
follow the format done in auto-explain or pg_prewarm for example.

=== 11

+  The default minimum password length if not setted passwordcheck.min_password_length is 8 chars.

s/chars/bytes/ (as per === 8 above)

Regards,

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