558803 - Add admin options for specifying/requiring password complexity in various forms

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Michael Coates [:mcoates] (acct no longer active)]

Issue:

Admins are unable to specify a complexity requirement for user passwords.

Recommended Remediation:

Provide an option that allows an admin to specify password complexity requirements for all passwords. A possible example is as follows:
Require passwords to contain:
[ ] lowercase letters
[ ] uppercase letters
[ ] numbers
[ ] special characters

Note: This bug is filed in addition to bug 330846 which is requesting an increase of the minimum password length requirement.

Comment 1

[Reed Loden [:reed]]

Not an issue that needs to be private.

Comment 2

[Michael Coates [:mcoates] (acct no longer active)]

Yea, I always debate (In reply to comment #1)
> Not an issue that needs to be private.

Yea, I debate that and generally default to marking the security flag for issues related to security. But I tend to agree with you.

Comment 3

[rojanu]

Attachment: Complex Passwords

We have been using the this patch and works fine.
I can't claim the credit for the patch but can't remember where I got it from either.

Comment 4

[Frédéric Buclin]

Comment on attachment 496125 [details] [diff] [review]
Complex Passwords

Wrong flag status.

Comment 5

[rojanu]

Attachment: ComplexPassword 4_RC1

I have applied the same patch to the Bugzilla 4 RC_1

Comment 6

[Frédéric Buclin]

Comment on attachment 497274 [details] [diff] [review]
ComplexPassword 4_RC1

We won't take this feature on the 4.0 branch. The code is feature-frozen for a long time.

Comment 7

[Frédéric Buclin]

Comment on attachment 496125 [details] [diff] [review]
Complex Passwords

>Index: Bugzilla/Config/Auth.pm

>+   name => 'passwordregexp',
>+   type => 't',
>+   default => q:[1-9],[A-Za-z]:,
>+   checker => \&check_regexp
>+  },
>+
>+  {
>+   name => 'passwordregexpfailmessage',
>+   type => 't',
>+   default => "Passwords must contain both letters and numbers.",
>+  },

We shouldn't use regexps to configure password complexity. They are a pain to write correctly to do what you want, and only advanced Perl hackers would know regexps well enough to do it correctly. Better is to offer a select list, following the idea in comment 0.

Also, the error message cannot be translated this way; it would be hardcoded. Using the select list above, it would be trivial to make the error message localizable.

Comment 8

[rojanu]

Attachment: ComplexPassword v3

Made regexs into <select> options and error message is in user-error.html.tmpl

Comment 9

[Frédéric Buclin]

Comment on attachment 497494 [details] [diff] [review]
ComplexPassword v3

>=== modified file 'Bugzilla/Config/Auth.pm'

>+   name => 'password_complexity',
>+   type => 'o',
>+   choices => [ '[1-9]', '[A-Z]', '[a-z]', '[@#$%^&+=]' ],

As discussed on IRC, this is pretty meaningless to most admins. Regexps used by Bugzilla should never be displayed, but used in the backend. Put real text to admins. As I suggested yesterday, we should probably have the following options:

- No constraints
- Mixed letters
- Letters and numbers
- Letters, numbers and special characters

Your regexps means that a user could have a pure numerical password, or a pure lowercase (respectively uppercase) password, which do not imply any complexity.

Comment 10

[rojanu]

Attachment: ComplexPassword v3.1

Comment 11

[Frédéric Buclin]

Comment on attachment 498551 [details] [diff] [review]
ComplexPassword v3.1

>=== modified file 'Bugzilla/User.pm'

>+sub test_password {

>+    my $password_regexp = '.*';

Use qr// instead of single quotes for regexps.


>+    warn "PASS: ".$password ." REGEX: ". $password_regexp;

Debug code.


>+    if ($password !~ $password_regexp) {
>+        return 0
>+    }

An error should be thrown instead. Also, this check should be done as part of $user->set_password(), which means this patch will conflict with the other one you are working on, see bug 284570.



>=== modified file 'editusers.cgi'

>+    if ($cgi->param('password')) {
>+        $valid_password = Bugzilla::User::test_password($cgi->param('password'));
>+    }

Should probably be part of $user->set_password().


I didn't look further.

Comment 12

[rojanu]

Attachment: Calidated Complex Passwords

I made the test password part of validate_password as a password needs to be validated before set but this way Admins have to meet the complexity levels as well, which I think would be fair for a system with complex passwords.

Comment 13

[Frédéric Buclin]

Comment on attachment 499017 [details] [diff] [review]
Calidated Complex Passwords

>=== modified file 'Bugzilla/User.pm'

>+    my $password_regexp = qr/.*/;
>+    if ($complexity_level eq 'no_constraints') {
>+        $password_regexp = qr/.*/;

This IF block is useless as .* is already set above.


>+    } elsif ($complexity_level eq 'mixed_letters') {
>+        $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z]).*/;
>+    } elsif ($complexity_level eq 'letters_numbers') {
>+        $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z])(?=.*\d).*/;
>+    } elsif ($complexity_level eq 'letters_numbers_specialchars') {
>+        $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@#\$%\^&+=]).*/;
>+    }

Using the (?=.*[...]) syntax is clever, however http://docstore.mik.ua/orelly/perl/cookbook/ch06_18.htm#ch06-19434 says that it may require more scans of the string than the usual $foo =~ /.../ && $foo =~ /.../ syntax. This shouldn't be a big deal, though.

A few more things:

1) http://perldoc.perl.org/POSIX.html#islower recommends to use [[:lower:]] instead of [a-z] and [[:upper:]] instead of [A-Z]. I think this makes sense for languages using non-ASCII characters. Note that "é" is not seen as lowercase by [[:lower:]], \p{Ll} and [a-z]. Not sure how to fix this, nor am I sure how they would behave with languages like russian or japanese. Anyway, I think the [[:foo:]] syntax should be used.

2) We should use [[:punct:]] instead of [@#\$%\^&+=], see http://perldoc.perl.org/perlrecharclass.html#[5] ([5] is part of the URL).

3) To follow the examples given at http://docstore.mik.ua/orelly/perl/cookbook/ch06_18.htm#ch06-19434, we could remove the leading and trailing .* and replace them by ^ at the beginning, i.e. qr/.*(...).*/ would become qr/^(...)/. Not sure if this makes any difference, though.

4) For letters_numbers_specialchars, maybe it's enough to just look for \w instead of [a-z][A-Z]. I would say that ZT8ç%W/ is complex enough, even without having any lowercase character. This would also save us from the problem described above about accented characters.



>=== modified file 'editusers.cgi'

>+    validate_password($password);

>+        if ($cgi->param('password')) {
>+            validate_password($cgi->param('password'));
>+            $otherUser->set_password($cgi->param('password'));
>+        }

Both changes are useless. set_password() already calls validate_password() via _check_password(). So you don't need to edit this file at all.



>=== modified file 'template/en/default/admin/params/auth.html.tmpl'

>+  password_complexity => "Controls management of complex passwords

Maybe "Set the complexity required for passwords"?


>+                        no_constraints - Passwords are not restricted in anyway

We should make it clearer that the minimum length of 6 characters is still applicable.


>+                        letters_numbers_specialchars - Passwords must contain at least one 
>+                        UPPER and one lower case letter, a number and a 
>+                        special character from @#$%^&+=

Per my comment above about [[:punct:]], we shouldn't restrict the list of special characters, but maybe simply enumerate one or two of them.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+     [% IF passregex.search('specialchars') %]
>+         <li>special character from @,#,$,%,^,&,+,=</li>

Same here.


Otherwise, your patch looks good, and the comments above are easy to address. We are very close. :)

Comment 14

[rojanu]

Attachment: Complex Passwords w/ nit RegEx

I am not totally happy about nitness of complexity if statement but couldn't come up with something nitter

Comment 15

[Frédéric Buclin]

Comment on attachment 501322 [details] [diff] [review]
Complex Passwords w/ nit RegEx

>=== modified file 'Bugzilla/Config/Auth.pm'

>+   choices => [ 'no_constraints', 'mixed_letters', 'letters_numbers', 'letters_numbers_specialchars' ],

This line is a bit too long.



>=== modified file 'Bugzilla/User.pm'

>+    if ($complexity_level eq 'letters_numbers_specialchars') {
>+        ThrowUserError('password_not_complex') if ($password !~ /\w/ ||
>+                                                   $password !~ /\d/ ||
>+                                                   $password !~ /[[:punct:]]/ );

I agree with you that your previous regexpes were much nicer. :)

Note that in Bugzilla, we usually prefer the

 $foo
 || $bar

syntax.



>=== modified file 'template/en/default/admin/params/auth.html.tmpl'

>+                        no_constraints - Passwords restriction off. ${terms.Bugzilla} passwords
>+                        must be at least ${constants.USER_PASSWORD_MIN_LENGTH} characters long

I think we should be clearer that the min length applies in all cases.


>+                        letters_numbers_specialchars - Passwords must contain at least one 
>+                        UPPER and one lower case letter, a number and a 

It should rather be "one UPPER *or* one lower", not *and*.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+     <ul>
>+     [% IF passregex.search('letters') %]

The indentation is incorrect. It should be

  <ul>
    [% IF ... %]

Note that there is a missing </ul> at the end of the list.


All these comments are minor and easy to fix. I'm going to fix them myself on checkin. Thank you for your patch! :) r=LpSolit

Comment 16

[Frédéric Buclin]

Attachment: checked in patch

Here is the patch I'm going to check in. It simply fixes the comments made above.

Comment 17

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/User.pm
modified Bugzilla/Config/Auth.pm
modified template/en/default/admin/params/auth.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7650.

Comment 18

[Frédéric Buclin]

Added to relnotes in bug 713346.

Comment 19

[Sunil Joshi [:sjoshi]]

Attachment: TestCasev1.diff

Proposed Test case

Comment 20

[Frédéric Buclin]

Comment on attachment 8369509 [details] [diff] [review]
TestCasev1.diff

I improved your testcase a bit to decrease the execution time by a factor of 2 (42 seconds instead of 85 seconds on my machine) and refactored it a bit to avoid lots of duplicated code. I also added tests to make sure that valid passwords are accepted. Thanks for the testcase! :)

Comment 21

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.4/
added t/test_password_complexity.t
Committed revision 246.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
added t/test_password_complexity.t
Committed revision 231.