Thread: Re: 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,
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
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
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: 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
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: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