Subject: Re: Parametrization minimum password lenght

From: Tomas Vondra <[email-masked]>
Date: Tue, 12 Nov 2024 14:48:28 +0100

Hi,

On 11/12/24 14:41, Emanuele Musella wrote:
> The goal about this patch is to parameterize the minimum password lenght
> on users database and apply it on the general code.
> The patch is applicable to the master branch.
> We already tested it: it build and works as expected and nothing is
> found broken,
> 
> Settings in postgresql.conf parametrization like following:
> 
> shared_preload_libraries = 'passwordcheck'
> min_password_lenght = 12
> 
> example:
> 
> postgres=# create user prova with password 'eftghaki';
> ERROR:  password is too short
> postgres=# create user prova with password 'eftghaki1234';
> CREATE ROLE
> 
> 
> In attach the file patch.
> 

Thanks for the patch, seems like a useful feature. Please add the patch
to the next commitfest (2025-01) at https://commitfest.postgresql.org/

A couple comments:

1) The proper spelling is "length" (not "lenght").

2) The GUC should be added to the "passwordcheck" extension, not to the
core GUC file. See how auto_explain defines options in _PG_init() using
DefineCustomIntVariable.

3) It might be a good idea to add a test to passwordcheck.sql.

regards

-- 
Tomas Vondra

---

From: Emanuele Musella <[email-masked]>
Date: Wed, 13 Nov 2024 17:54:56 +0100

Hi Tomas,

we have done the fixes that you have suggested us in the last mail.

1) Modified passwordcheck.sql with a_nice_long_password and also
passwordcheck.out

2) we added the documentation in the doc/src/sgml/passwordcheck.sgml.

3) we have done make check successfully.

If you have other suggestions to share with us will be welcommed.

Thank you

Best regards

Il giorno mar 12 nov 2024 alle ore 16:49 Tomas Vondra <[email-masked]> ha
scritto:

> On 11/12/24 16:36, Emanuele Musella wrote:
> > Hi Tomas,
> >
> > thank you for you feedback.
> >
> > We have implemented you suggestion in the patch. I attached you the
> > latest version.
> >
>
> Please always reply to the mailing list, not just directly to individual
> people. Otherwise others won't see your new messages, it won't get into
> mailing list archives, etc.
>
> > We are waiting for your last feedback to upload the patch on Commitfest.
> >
>
> Not sure I follow. You can add the patch to the commitfest on your own,
> and there's no need to wait for my feedback before doing so.
>
> A couple quick comments for the patch, though:
>
> 1) You modified just the SQL script for the test, not the expected
> output in expected/passwordcheck.out. This means running "make check"
> for this extension will fail.
>
> 2) It's not clear to me why you removed  the first ALTER TABLE with
> a_nice_long_password. Seems wrong.
>
> 3) The test doesn't change the new GUC at all, so how would this show
> the setting actually affects anything? And you only test the "positive"
> case, maybe it'd be good to change the GUC and then test that it rejects
> a password.
>
> 4) The new GUC needs to be added to doc/src/sgml/passwordcheck.sgml.
>
>
> regards
>
> --
> Tomas Vondra
>
>

---

From: Emanuele Musella <[email-masked]>
Date: Mon, 18 Nov 2024 10:01:41 +0100

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.

Best regards

Il giorno mer 13 nov 2024 alle ore 17:54 Emanuele Musella <
[email-masked]> ha scritto:

> Il giorno mar 12 nov 2024 alle ore 16:49 Tomas Vondra <[email-masked]>
> ha scritto:
>

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 18 Nov 2024 10:39:33 +0000

Hi,

> In attach the fixed patch.

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

A few random comments:

=== 1

+ * Author: Maurizio Boriani <[email-masked]>
+ * Author: Emanuele Musella <[email-masked]>

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

---

From: Emanuele Musella <[email-masked]>
Date: Mon, 18 Nov 2024 16:59:53 +0100

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 <
[email-masked]> ha scritto:

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

---

From: Emanuele Musella <[email-masked]>
Date: Mon, 18 Nov 2024 17:21:18 +0100

We notice some errors on CFBot results.

In attached the errors fixed

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 19 Nov 2024 19:28:03 +0000

Hi,

> 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

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,

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Nov 2024 07:45:40 +0900

> to the next commitfest (2025-01) at https://commitfest.postgresql.org/

FYI, I have a large set of such things in my own repo with a clone of
passwordcheck:
https://github.com/michaelpq/pg_plugins/tree/main/passwordcheck_extra

Not all of them may be useful, still the minimal length is one of
them:
- passwordcheck_extra.special_chars, to define a list of special characters
  with the password needing at least one. Default is "!@#$%^&*()_+{}|<>?=".
- passwordcheck_extra.restrict_lower, to enforce the use of at least one
  lower-case character.
- passwordcheck_extra.restrict_upper, to enforce the use of at least one
  upper-case character.
- passwordcheck_extra.restrict_numbers, to enforce the use of at least
  one number.
- passwordcheck_extra.restrict_special, to enforce the use of at least
  one special character listed in \"passwordcheck_extra.special_chars\".
- passwordcheck_extra.minimum_length, minimum length of password allowed.
  Default is 8, which likely sucks.
- passwordcheck_extra.maximum_length, maximum length of password allowed.
  Default is 15, which definitely sucks, but it is useful for tests.

I would not mind if you just copy-paste some of this stuff, and the
code is released under the PostgreSQL license.  Feel free to not worry
about the authorship part as well, I'm fine even if my name is
discarded.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 21 Nov 2024 16:25:38 -0600

> https://github.com/michaelpq/pg_plugins/tree/main/passwordcheck_extra

Here is some previous discussion about passwordcheck that may be of
interest:

	https://www.postgresql.org/message-id/flat/D960CB61B694CF459DCFB4B0128514C203937F49%40exadv11.host.magwien.gv.at
	https://www.postgresql.org/message-id/flat/AC785D69-41EC-4D0A-AC37-1F9FF55C9E34%40amazon.com

-- 
nathan