Thread
Commits
-
Add passwordcheck.min_password_length.
- f7e1b3828a93 18.0 landed
-
Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-12T13:41:00Z
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. Best regards Emanuele Musella & Maurizio Boriani
-
Re: Parametrization minimum password lenght
Tomas Vondra <tomas@vondra.me> — 2024-11-12T13:48:28Z
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
-
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-13T16:54:56Z
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: > 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 > >
-
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-18T09:01:41Z
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 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: > >> 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 >> >>
-
Re: Parametrization minimum password lenght
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-18T10:39:33Z
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 -
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-18T15:59:53Z
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: > 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 > -
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-18T16:21:18Z
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: > 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: > >> 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 >> > -
Re: Parametrization minimum password lenght
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-19T19:28:03Z
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
-
Re: Parametrization minimum password lenght
Michael Paquier <michael@paquier.xyz> — 2024-11-19T22:45:40Z
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 -
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2024-11-21T22:25:38Z
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
-
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-25T10:15:51Z
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: > 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 >
-
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-11-25T17:35:49Z
Sorry here the right attachment Il giorno lun 25 nov 2024 alle ore 11:15 Emanuele Musella < emamuse86@gmail.com> ha scritto: > 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: > >> 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 >> >
-
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2024-12-18T20:56:24Z
Here is what I have staged for commit. -- nathan
-
Re: Parametrization minimum password lenght
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-19T07:25:30Z
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]), but that's probably a nit. [1]: https://www.postgresql.org/message-id/Zzzmw4IAvrypmFO4%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 -
Re: Parametrization minimum password lenght
Emanuele Musella <emamuse86@gmail.com> — 2024-12-19T10:22:02Z
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: > Here is what I have staged for commit. > > -- > nathan >
-
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2024-12-19T15:36:17Z
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 -
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2024-12-19T15:40:25Z
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
-
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2024-12-19T15:57:31Z
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 -
Re: Parametrization minimum password lenght
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-12-20T09:15:28Z
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 -
Re: Parametrization minimum password lenght
Japin Li <japinli@hotmail.com> — 2024-12-25T05:15:53Z
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 -
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-06T19:32:16Z
On Wed, Dec 25, 2024 at 01:15:53PM +0800, Japin Li wrote: > 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. Here is a rebased version of the patch. Barring objections, I am planning to commit this soon. -- nathan
-
Re: Parametrization minimum password lenght
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-07T21:08:57Z
Committed. -- nathan