vncpasswd add pwquality complexity rule check #1762

prownd · 2024-06-02T09:46:57Z

Add a password complexity strategy for VNC user authentication
enhance password quality verification checks, and enhance security

Additionally, Chinese translation has been added

CendioOssman · 2024-06-03T14:38:27Z

Thank you for your contribution!

I'm concerned how well a pwquality will work for VNC passwords, given that they are limited to 8 characters. #370 would probably need to be implemented first. Otherwise the user might come up with a long and secure to keep pwquality happy, but overlook the fact that the actual used password is much shorter.

It should probably also just be a warning when pwquality fails, not an error, given that it is difficult to get a good password with so few characters.

prownd · 2024-06-03T16:25:34Z

Yes, due to the VNC protocol, the actual length of the VNC password used is 8 characters.
The first version I modified did indeed limit the maximum length to 8 characters. However, considering that there has always been no limit on the maximum length when setting passwords, so I removed the limit of 8 characters.
According to your suggestion, when pwquality fails, there will only be an alarm prompt. Instead of terminating password settings

prownd · 2024-06-03T16:29:28Z

Thank you for your contribution!

I'm concerned how well a pwquality will work for VNC passwords, given that they are limited to 8 characters. #370 would probably need to be implemented first. Otherwise the user might come up with a long and secure to keep pwquality happy, but overlook the fact that the actual used password is much shorter.

It should probably also just be a warning when pwquality fails, not an error, given that it is difficult to get a good password with so few characters.

According to your opinion, when setting a password, add an 8-character limit.
Can I modify and submit it again or not

prownd · 2024-06-07T17:54:59Z

Use libpwquality to check password quality and complexity, improving security.
The validation rules for libpwquality are as follows:
- The minimum password length is 6 and the maximum is 8
- Password characters repeat once
- At least 3 types of characters are required
Add i18n internationalization support and Chinese localization translation.

prownd · 2024-06-07T17:58:17Z

Use libpwquality to check password quality and complexity, improving security.
The validation rules for libpwquality are as follows:

The minimum password length is 6 and the maximum is 8

Password characters repeat once

At least 3 types of characters are required

Add i18n internationalization support and Chinese localization translation.

@CendioOssman Could you please take a look and see if there is anything else that needs to be added.

CendioOssman

Looks really good! There are some tweaks that are needed, though. Please have a look at my comments.

po/zh_CN.po

unix/vncpasswd/CMakeLists.txt

unix/vncpasswd/vncpasswd.cxx

CendioOssman · 2024-06-14T14:19:29Z

unix/vncpasswd/vncpasswd.cxx

+	pwquality_set_int_value(pwq, PWQ_SETTING_MIN_LENGTH, 6);
+	pwquality_set_int_value(pwq, PWQ_SETTING_MAX_SEQUENCE, 8);
+	pwquality_set_int_value(pwq, PWQ_SETTING_MAX_REPEAT, 1);
+	pwquality_set_int_value(pwq, PWQ_SETTING_MIN_CLASS, 3);


This doesn't seem proper to override. Isn't this configured by the system administrator in /etc/security/pwquality.conf?

Using the rules specified in the code for the libpwquality library.
Instead of using the libpwquality.cn configuration file.
I think these two are independent, so there should be no rewriting or conflict.

The man page for pwquality.conf states:

pwquality.conf provides a way to configure the default password quality requirements for the system passwords. This file is read by the libpwquality library and utilities that use this library for checking and generating passwords.

So I think it would be very surprising for a sysadmin if those settings aren't respected by vncpasswd.

Please remove all the overrides and we can use the settings that libpwquality has loaded from the system configuration.

CendioOssman · 2024-06-14T14:22:31Z

unix/vncpasswd/vncpasswd.cxx

        exit(1);
      }
-      fprintf(stderr,"Password must be at least 6 characters - try again\n");
+      fprintf(stderr,_("Password must be at least 6 characters - try again\n"));


This is now policy that could conflict with what pwquality wants. It should be removed if we always use pwquality, or used as a fallback if pwquality ends up being optional.

unix/vncpasswd/vncpasswd.cxx

CendioOssman · 2024-06-14T14:24:05Z

unix/vncpasswd/vncpasswd.cxx

+      continue;
+    }
+
+    if (first.size() > 8) {


Nice! But could you split this to a separate commit? It's independent of using pwquality.

Perhaps they should be submitted separately

A separate commit, yes. But you don't need to open a separate PR for such a small fix.

unix/vncpasswd/vncpasswd.cxx

prownd · 2024-06-24T02:56:58Z

@CendioOssman
I have made modifications according to your suggestions.
Could you please review it to see if it is correct.

CendioOssman · 2024-06-24T09:29:18Z

unix/vncpasswd/CMakeLists.txt

+# check for pwquality password check support
+option(ENABLE_PWQUALITY "Enable pwquality password check" ON)
+if(ENABLE_PWQUALITY)
+  if(UNIX)


Perhaps this if() should be around everything, so we don't annoy Windows users with this setting?

unix/vncpasswd/CMakeLists.txt

prownd · 2024-06-25T09:28:18Z

Could you please take a moment to review again . @CendioOssman

prownd · 2024-07-11T12:47:54Z

@CendioOssman Please review again.
Is your suggestion to use the default/etc/security/pwquality.conf configuration rules?
But as mentioned earlier, the actual maximum length of a password is only 8, which may have generated a very long and complex password, but only 8 lengths were actually used.
So it is very necessary to specify some rules in the code.

Looking forward to your reply and suggestions.

CendioOssman · 2024-08-06T11:38:21Z

Sorry for the late response. I've been away for a few weeks.

There are still some open requests that haven't been addressed. Please have a look at them and see if we can get the last bits fixed.

CendioOssman · 2024-08-06T11:40:48Z

Is your suggestion to use the default/etc/security/pwquality.conf configuration rules?

Yes, that is correct.

But as mentioned earlier, the actual maximum length of a password is only 8, which may have generated a very long and complex password, but only 8 lengths were actually used.
So it is very necessary to specify some rules in the code.

If the system policy doesn't allow passwords that are compatible with VNC, then the system should refuse to continue. It shouldn't silently change the policy and allow things that the user thinks have been blocked.

As for only 8 characters being used, your previous suggestion had a check for that. But it seems to have been dropped?

prownd · 2024-08-07T18:04:23Z

Is your suggestion to use the default/etc/security/pwquality.conf configuration rules?

Yes, that is correct.

Okay, then use the default pwquality.conf rule

But as mentioned earlier, the actual maximum length of a password is only 8, which may have generated a very long and complex password, but only 8 lengths were actually used.
So it is very necessary to specify some rules in the code.

If the system policy doesn't allow passwords that are compatible with VNC, then the system should refuse to continue. It shouldn't silently change the policy and allow things that the user thinks have been blocked.

As for only 8 characters being used, your previous suggestion had a check for that. But it seems to have been dropped?

Because it's independent of using pwquality
Your suggestion is to submit a separate commit

CendioOssman · 2024-08-13T13:00:09Z

Your suggestion is to submit a separate commit

Yes, but the commit can still be part of this pull request.

Use the library pwquality to check password complexity and improve security. Additionally, optional enable support is also set in CMake.

Password should not be greater than 8 characters. Because only 8 valid characters are used.

prownd · 2024-08-13T17:03:29Z

@CendioOssman According to your suggestion, it has been modified.
Could you please review it again

CendioOssman

Looks good! Thank you for getting this done.

prownd force-pushed the master branch from 883527c to 3d71e75 Compare June 7, 2024 17:40

CendioOssman requested changes Jun 14, 2024

View reviewed changes

prownd force-pushed the master branch from 3d71e75 to c3d6dee Compare June 24, 2024 02:46

prownd requested a review from CendioOssman June 24, 2024 02:52

prownd changed the title ~~vncpasswd add pwquality complexity rule check and translation~~ vncpasswd add pwquality complexity rule check Jun 24, 2024

CendioOssman requested changes Jun 24, 2024

View reviewed changes

prownd force-pushed the master branch from c3d6dee to 09fa7a7 Compare June 24, 2024 13:57

prownd requested a review from CendioOssman June 24, 2024 14:04

prownd closed this Aug 7, 2024

prownd reopened this Aug 7, 2024

vncpasswd add password complexity rule check to enhance security

6e3b382

Use the library pwquality to check password complexity and improve security. Additionally, optional enable support is also set in CMake.

prownd force-pushed the master branch from 09fa7a7 to 6e3b382 Compare August 13, 2024 16:32

Limit the maximum length of a password to 8 characters

73e6e18

Password should not be greater than 8 characters. Because only 8 valid characters are used.

CendioOssman approved these changes Aug 15, 2024

View reviewed changes

CendioOssman merged commit 3aca8be into TigerVNC:master Aug 15, 2024
11 checks passed

CendioOssman mentioned this pull request Aug 15, 2024

warn about maximum VNC password length #370

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vncpasswd add pwquality complexity rule check #1762

vncpasswd add pwquality complexity rule check #1762

prownd commented Jun 2, 2024

CendioOssman commented Jun 3, 2024

prownd commented Jun 3, 2024 •

edited

Loading

prownd commented Jun 3, 2024 •

edited

Loading

prownd commented Jun 7, 2024

prownd commented Jun 7, 2024

CendioOssman left a comment

CendioOssman Jun 14, 2024

prownd Jun 17, 2024 •

edited

Loading

CendioOssman Jun 24, 2024

CendioOssman Jun 14, 2024

CendioOssman Jun 14, 2024

prownd Jun 17, 2024

CendioOssman Jun 24, 2024

prownd commented Jun 24, 2024

CendioOssman Jun 24, 2024

prownd commented Jun 25, 2024 •

edited

Loading

prownd commented Jul 11, 2024

CendioOssman commented Aug 6, 2024

CendioOssman commented Aug 6, 2024

prownd commented Aug 7, 2024

CendioOssman commented Aug 13, 2024

prownd commented Aug 13, 2024

CendioOssman left a comment

vncpasswd add pwquality complexity rule check #1762

vncpasswd add pwquality complexity rule check #1762

Conversation

prownd commented Jun 2, 2024

CendioOssman commented Jun 3, 2024

prownd commented Jun 3, 2024 • edited Loading

prownd commented Jun 3, 2024 • edited Loading

prownd commented Jun 7, 2024

prownd commented Jun 7, 2024

CendioOssman left a comment

Choose a reason for hiding this comment

CendioOssman Jun 14, 2024

Choose a reason for hiding this comment

prownd Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

CendioOssman Jun 24, 2024

Choose a reason for hiding this comment

CendioOssman Jun 14, 2024

Choose a reason for hiding this comment

CendioOssman Jun 14, 2024

Choose a reason for hiding this comment

prownd Jun 17, 2024

Choose a reason for hiding this comment

CendioOssman Jun 24, 2024

Choose a reason for hiding this comment

prownd commented Jun 24, 2024

CendioOssman Jun 24, 2024

Choose a reason for hiding this comment

prownd commented Jun 25, 2024 • edited Loading

prownd commented Jul 11, 2024

CendioOssman commented Aug 6, 2024

CendioOssman commented Aug 6, 2024

prownd commented Aug 7, 2024

CendioOssman commented Aug 13, 2024

prownd commented Aug 13, 2024

CendioOssman left a comment

Choose a reason for hiding this comment

prownd commented Jun 3, 2024 •

edited

Loading

prownd commented Jun 3, 2024 •

edited

Loading

prownd Jun 17, 2024 •

edited

Loading

prownd commented Jun 25, 2024 •

edited

Loading