Parametrization minimum password lenght

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

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 <tomas@vondra.me> ha
scritto:

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 <
emamuse86@gmail.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@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

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.pg@gmail.com> ha scritto:

We notice some errors on CFBot results.

In attached the errors fixed

Il giorno lun 18 nov 2024 alle ore 16:59 Emanuele Musella <
emamuse86@gmail.com> ha scritto:

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]https://github.com/postgres/postgres/blob/master/src/tools/ci/README).

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]/messages/by-id/ZzsZZY3YrO6hinnT@ip-10-97-1-34.eu-west-3.compute.internal.

=== 3

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

Sorry if I was not clear in [2]/messages/by-id/ZzsZZY3YrO6hinnT@ip-10-97-1-34.eu-west-3.compute.internal, 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]: /messages/by-id/ZzsZZY3YrO6hinnT@ip-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

On Tue, Nov 12, 2024 at 02:48:28PM +0100, Tomas Vondra wrote:

Thanks for the patch, seems like a useful feature. Please add the patch
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

On Wed, Nov 20, 2024 at 07:45:40AM +0900, Michael Paquier wrote:

On Tue, Nov 12, 2024 at 02:48:28PM +0100, Tomas Vondra wrote:

Thanks for the patch, seems like a useful feature. Please add the patch
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

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

/messages/by-id/D960CB61B694CF459DCFB4B0128514C203937F49@exadv11.host.magwien.gv.at
/messages/by-id/AC785D69-41EC-4D0A-AC37-1F9FF55C9E34@amazon.com

--
nathan

Thank you Bertrand for your feedbacks. We are looking for CFbot part so we
can compile it like CFbot.

For now we have fixed all points.

Thank you

Il giorno mar 19 nov 2024 alle ore 20:28 Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> ha scritto:

Sorry here the right attachment

Il giorno lun 25 nov 2024 alle ore 11:15 Emanuele Musella <
emamuse86@gmail.com> ha scritto:

Here is what I have staged for commit.

--
nathan

Hi,

On Wed, Dec 18, 2024 at 02:56:24PM -0600, Nathan Bossart wrote:

Here is what I have staged for commit.

Thanks!

A few comments:

=== 1

+ if (pwdlen < min_password_length)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password is too short")));

Now that the minimum password length is not "hardcoded" anymore, I wonder if it
wouldn't be better to provide more details here (pwdlen and min_password_length).

Suggestion in on_top_of_0001.txt attached.

=== 2

+       /* Define custom GUC variables. */
+       DefineCustomIntVariable("passwordcheck.min_password_length",
+                                                       "Minimum allowed password length.",
+                                                       NULL,
+                                                       &min_password_length,
+                                                       8,
+                                                       0, INT_MAX,

Since password must contain both letters and nonletters, 0 seems too low. I
wonder if 2 is not a better value (done in on_top_of_0001.txt attached).

Also, it seems to me that INT_MAX is too large (as mentioned in [1]/messages/by-id/Zzzmw4IAvrypmFO4@ip-10-97-1-34.eu-west-3.compute.internal), but that's
probably a nit.

[1]: /messages/by-id/Zzzmw4IAvrypmFO4@ip-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

Hi Nathan,

thank you.

It seems to me you are working on an older version of patch.

In attached the latest one for your reference.

Thanks for your support

Best regards

Emanuele Musella

Il giorno mer 18 dic 2024 alle ore 21:56 Nathan Bossart <
nathandbossart@gmail.com> ha scritto:

On Thu, Dec 19, 2024 at 07:25:30AM +0000, Bertrand Drouvot wrote:

+ if (pwdlen < min_password_length)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("password is too short")));

Now that the minimum password length is not "hardcoded" anymore, I wonder if it
wouldn't be better to provide more details here (pwdlen and min_password_length).

Suggestion in on_top_of_0001.txt attached.

Yeah, good point.

+       /* Define custom GUC variables. */
+       DefineCustomIntVariable("passwordcheck.min_password_length",
+                                                       "Minimum allowed password length.",
+                                                       NULL,
+                                                       &min_password_length,
+                                                       8,
+                                                       0, INT_MAX,

Since password must contain both letters and nonletters, 0 seems too low. I
wonder if 2 is not a better value (done in on_top_of_0001.txt attached).

Also, it seems to me that INT_MAX is too large (as mentioned in [1]), but that's
probably a nit.

I don't think we need to restrict the allowed values here. It's true that
the lower bound will be effectively 2 due to the other checks, but that's
not the responsibility of this one to enforce. Those other checks will
likely become configurable at some point, too.

-					 errmsg("password is too short")));
+					 errmsg("password is too short: %d (< %d)", pwdlen, min_password_length)));

I think we should explicitly point to the parameter and note its current
value.

--
nathan

On Thu, Dec 19, 2024 at 11:22:02AM +0100, Emanuele Musella wrote:

It seems to me you are working on an older version of patch.

In attached the latest one for your reference.

I used the v9 patch as the basis for v10. There are a few small edits,
such as removing the upper bound for the parameter and expanding the
documentation, but the feature itself remains intact.

--
nathan

On Thu, Dec 19, 2024 at 09:36:17AM -0600, Nathan Bossart wrote:

On Thu, Dec 19, 2024 at 07:25:30AM +0000, Bertrand Drouvot wrote:

-					 errmsg("password is too short")));
+					 errmsg("password is too short: %d (< %d)", pwdlen, min_password_length)));

I think we should explicitly point to the parameter and note its current
value.

Like so.

--
nathan

On Thu, Dec 19, 2024 at 09:57:31AM -0600, Nathan Bossart wrote:

On Thu, Dec 19, 2024 at 09:36:17AM -0600, Nathan Bossart wrote:

On Thu, Dec 19, 2024 at 07:25:30AM +0000, Bertrand Drouvot wrote:

-					 errmsg("password is too short")));
+					 errmsg("password is too short: %d (< %d)", pwdlen, min_password_length)));

I think we should explicitly point to the parameter and note its current
value.

Like so.

LGTM.

Regards,

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

On Thu, 19 Dec 2024 at 09:57, Nathan Bossart <nathandbossart@gmail.com> wrote:

On Thu, Dec 19, 2024 at 09:36:17AM -0600, Nathan Bossart wrote:

On Thu, Dec 19, 2024 at 07:25:30AM +0000, Bertrand Drouvot wrote:

-					 errmsg("password is too short")));
+					 errmsg("password is too short: %d (< %d)", pwdlen, min_password_length)));

I think we should explicitly point to the parameter and note its current
value.

Like so.

It seems that the v11 cannot apply on master since 7f97b4734, and rebase is needed.

$ git am ~/v11-0001-Add-passwordcheck.min_password_length.patch
Applying: Add passwordcheck.min_password_length.
error: patch failed: contrib/passwordcheck/passwordcheck.c:26
error: contrib/passwordcheck/passwordcheck.c: patch does not apply
Patch failed at 0001 Add passwordcheck.min_password_length.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

The comment of prev_check_password_hook has been changed which case the apply failed.

--
Regrads,
Japin Li