Re: Parametrization minimum password lenght

Hello Bertrand,

thank you for your feedbacks.

We have done every fixes required.

In attach the last version.

Best regards

Il giorno lun 18 nov 2024 alle ore 11:39 Bertrand Drouvot <
bertranddrouvot(dot)pg(at)gmail(dot)com> ha scritto:

> 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(at)boriani(dot)cloud>
> + * Author: Emanuele Musella <emamuse86(at)gmail(dot)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
>