Require password complexity #3205

skofman1 · 2016-08-18T23:47:33Z

Require password complexity: atleast 8 characters, one uppercase letter, one lowercase letter, and a digit.

@maartenba @blowdart @yishaigalatzer @qianjun22

scottbommarito · 2016-08-18T23:49:16Z

src/NuGetGallery/Strings.resx

@@ -395,4 +395,7 @@ The {2} Team</value>
  <data name="PackageCreatedFromApi" xml:space="preserve">
    <value>Package created from API.</value>
  </data>
+  <data name="PasswordValidationFailure" xml:space="preserve">
+    <value>Use atleast 8 characters and include one uppercase letter, one lowercase letter and a digit. </value>


at least (missing the space) #Resolved

scottbommarito · 2016-08-19T00:02:39Z

Correct me if I'm wrong, but this only enforces that the password is secure on registration, right? This doesn't prevent users from resetting their password to something that's not allowed on the accounts page or by following the forgot my password flow, right? #Resolved

maartenba · 2016-08-19T06:11:04Z

src/NuGetGallery/ViewModels/LogOnViewModel.cs

-        internal static readonly Regex UsernameNormalizationRegex =
-            new Regex(@"[^A-Za-z0-9_\.-]");
+        internal const string PasswordValidationRegex =
+            @"^(?=.*[A-Z])(?=.*[a-z])(?=.*[0-9]).{1,64}$";


Comment would indeed be good, the {1,64} confuses me - Is this RegEx allowing passwords of 1 character length? #Resolved

maartenba · 2016-08-19T06:11:50Z

The atleast should be updated, other than that LGTM #Resolved

shishirx34 · 2016-08-19T17:37:16Z

I'd be interested to hear answer to @scottbommarito's question above about password reset flow, other than that LGTM. #Resolved

skofman1 · 2016-08-19T18:55:16Z

Good catch :)

In reply to: 240892895 [](ancestors = 240892895)

skofman1 · 2016-08-19T19:00:41Z

Added hints:

@harikmenon?

#Resolved

harikmenon · 2016-08-19T20:56:13Z

We should change the text to the below. Otherwise looks good.

Your password must be at least 8 characters, should include at least one uppercase letter, one lowercase letter and a digit #Resolved

scottbommarito · 2016-08-19T21:33:24Z

I think you're still missing the ResetPassword flow, I only see Register and Change Password.
https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/ViewModels/PasswordResetViewModel.cs#L14 #Resolved

skofman1 · 2016-08-19T21:52:11Z

Thanks!

In reply to: 241141008 [](ancestors = 241141008)

scottbommarito · 2016-08-19T21:54:30Z

src/NuGetGallery/ViewModels/AccountViewModel.cs

@@ -53,6 +53,7 @@ public class ChangePasswordViewModel

        [Required]
        [Display(Name = "New Password")]
+        [RegularExpression(RegisterViewModel.PasswordValidationRegex, ErrorMessage = RegisterViewModel.PasswordHint)]


Nit, but I think it would better to have a help class somewhere containing these RegEx and hints statically so we don't have ViewModels referring to other ViewModels, but it's fine to have it like that. #Resolved

+1 #Resolved

scottbommarito · 2016-08-19T21:54:38Z

LGTM! #Closed

skofman1 · 2016-09-06T21:57:24Z

@yishaigalatzer @harikmenon , can I merge? #Closed

yishaigalatzer · 2016-09-06T22:27:33Z

This is going to break existing users of the gallery, I would make this configurable. Thoughts?
I don't want this to sneak into a deployment, lets hold off until we can review this change with the directors next week #Closed

skofman1 · 2016-09-06T22:29:54Z

This change won't break users. Simply, the next time they create a password, the requirements will be more severe. #Closed

yishaigalatzer · 2016-09-06T22:33:10Z

tests/NuGetGallery.Facts/PasswordValidationRegexTests.cs

+        [InlineData("aaAAaaAAaaAA")] // No digit
+        [InlineData("12345678a")] // No upperscase letter
+        [InlineData("12345678A")] // No lowercase letter
+        [InlineData("1aA")] // Too short


Add more trivial cases

single letter
single upper case letter
single number
just a number
special chars #Resolved

skofman1 · 2016-09-07T18:41:41Z

We are considering making password complexity configurable. This will allow users deploying gallery on their own servers to control the password complexity they require from their users.
There are 2 options:

Have a turn on/off flag to enforce complexity using regex as suggested in this PR (when turned off only password length will be required).
Have the regex used configurable.

Do you think this feature is important for our users? Should we enforce password complexity by default to make our customers more secure, even if they have their own deployments?
Which of the options above do you prefer?

@yishaigalatzer @robertmuehsig @maartenba @xavierdecoster @dtivel @qianjun22
#Closed

dtivel · 2016-09-07T19:41:22Z

An on/off flag would be useful for toggling the feature. I think a configurable regex can be implemented later based on feedback. #Closed

yishaigalatzer · 2016-09-07T22:36:20Z

The cost of implementing configuration is negligible, compared to a switch. Lets not push it out, either do it or not. #Closed

xavierdecoster · 2016-09-12T10:46:41Z

+1 on configurable regex with default value enforcing the rules outlined in this issue #Closed

skofman1 · 2016-09-12T22:26:21Z

a new version: password regex was made configurable. Please review again #Closed

shishirx34 · 2016-09-12T22:32:21Z

src/NuGetGallery/Configuration/AppConfiguration.cs

@@ -174,6 +174,14 @@ public class AppConfiguration : IAppConfiguration
        /// </summary>
        public string EnforcedAuthProviderForAdmin { get; set; }

+        [Required]
+        [DefaultValue("^(?=.*[A-Z])(?=.*[a-z])(?=.*[0-9]).{8,64}$")]


I think the default should be any string. That way if anyone wants to use it will set it as appropriate, or may be provide a way to disable configuration?

.* ``` #WontFix

I think the default behavior should be as specified above (unless I'm missing something). I would add a comment explaining what this regex supposed to do #Resolved

shishirx34 · 2016-09-12T22:33:32Z

Looks good. #Closed

yishaigalatzer · 2016-09-12T23:19:41Z

src/NuGetGallery/Infrastructure/GalleryPasswordValidationAttribute.cs

+namespace NuGetGallery.Infrastructure
+{
+    [AttributeUsage(AttributeTargets.Property)]
+    public sealed class GalleryPasswordValidationAttribute : ValidationAttribute


nit: I would call this PasswordValidationAttribute the gallery is unnecessary #Resolved

yishaigalatzer · 2016-09-12T23:24:52Z

When nit comments addressed

skofman1 added 2 commits August 18, 2016 16:43

Require password complexity

b7dafeb

Merge branch 'dev' into feature-passwordComplexity

f584b31

skofman1 self-assigned this Aug 18, 2016

dnfclas added the cla-already-signed label Aug 18, 2016

scottbommarito reviewed Aug 18, 2016
View reviewed changes

maartenba reviewed Aug 19, 2016
View reviewed changes

Add password validation for password change + fix hints

954df82

Changed hint message

5883d79

Apply for password reset

fbb5864

scottbommarito reviewed Aug 19, 2016
View reviewed changes

yishaigalatzer reviewed Sep 6, 2016
View reviewed changes

skofman1 added 2 commits September 6, 2016 16:34

Merge branch 'dev' into feature-passwordComplexity

f1dd29f

Move regex and error message to Contants class

fcdaab0

skofman1 added 2 commits September 12, 2016 15:24

Make password regex configurable

e34dce2

Merge branch 'dev' into feature-passwordComplexity

2ea5304

shishirx34 reviewed Sep 12, 2016
View reviewed changes

yishaigalatzer reviewed Sep 12, 2016
View reviewed changes

Fix PR comments

97f5b5d

skofman1 merged commit 93fa02b into dev Sep 12, 2016

skofman1 deleted the feature-passwordComplexity branch September 12, 2016 23:49

maartenba mentioned this pull request Sep 30, 2016

Deployment 2016.08 #3196

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require password complexity #3205

Require password complexity #3205

skofman1 commented Aug 18, 2016 •

edited

Loading

scottbommarito Aug 18, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

maartenba Aug 19, 2016 •

edited by skofman1

Loading

maartenba commented Aug 19, 2016 •

edited by skofman1

Loading

shishirx34 commented Aug 19, 2016 •

edited by skofman1

Loading

skofman1 commented Aug 19, 2016

skofman1 commented Aug 19, 2016 •

edited

Loading

harikmenon commented Aug 19, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

skofman1 commented Aug 19, 2016

scottbommarito Aug 19, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 6, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 6, 2016 •

edited

Loading

yishaigalatzer commented Sep 6, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 6, 2016 •

edited

Loading

yishaigalatzer Sep 6, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 7, 2016 •

edited

Loading

dtivel commented Sep 7, 2016 •

edited by skofman1

Loading

yishaigalatzer commented Sep 7, 2016 •

edited by skofman1

Loading

xavierdecoster commented Sep 12, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 12, 2016 •

edited

Loading

shishirx34 Sep 12, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 12, 2016 •

edited by skofman1

Loading

shishirx34 commented Sep 12, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 12, 2016 •

edited by skofman1

Loading

yishaigalatzer commented Sep 12, 2016

Require password complexity #3205

Require password complexity #3205

Conversation

skofman1 commented Aug 18, 2016 • edited Loading

scottbommarito Aug 18, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

scottbommarito commented Aug 19, 2016 • edited by skofman1 Loading

maartenba Aug 19, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

maartenba commented Aug 19, 2016 • edited by skofman1 Loading

shishirx34 commented Aug 19, 2016 • edited by skofman1 Loading

skofman1 commented Aug 19, 2016

skofman1 commented Aug 19, 2016 • edited Loading

harikmenon commented Aug 19, 2016 • edited by skofman1 Loading

scottbommarito commented Aug 19, 2016 • edited by skofman1 Loading

skofman1 commented Aug 19, 2016

scottbommarito Aug 19, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

yishaigalatzer Sep 6, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

scottbommarito commented Aug 19, 2016 • edited by skofman1 Loading

skofman1 commented Sep 6, 2016 • edited Loading

yishaigalatzer commented Sep 6, 2016 • edited by skofman1 Loading

skofman1 commented Sep 6, 2016 • edited Loading

yishaigalatzer Sep 6, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

skofman1 commented Sep 7, 2016 • edited Loading

dtivel commented Sep 7, 2016 • edited by skofman1 Loading

yishaigalatzer commented Sep 7, 2016 • edited by skofman1 Loading

xavierdecoster commented Sep 12, 2016 • edited by skofman1 Loading

skofman1 commented Sep 12, 2016 • edited Loading

shishirx34 Sep 12, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

yishaigalatzer Sep 12, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

shishirx34 commented Sep 12, 2016 • edited by skofman1 Loading

yishaigalatzer Sep 12, 2016 • edited by skofman1 Loading

Choose a reason for hiding this comment

yishaigalatzer commented Sep 12, 2016

skofman1 commented Aug 18, 2016 •

edited

Loading

scottbommarito Aug 18, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

maartenba Aug 19, 2016 •

edited by skofman1

Loading

maartenba commented Aug 19, 2016 •

edited by skofman1

Loading

shishirx34 commented Aug 19, 2016 •

edited by skofman1

Loading

skofman1 commented Aug 19, 2016 •

edited

Loading

harikmenon commented Aug 19, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

scottbommarito Aug 19, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 6, 2016 •

edited by skofman1

Loading

scottbommarito commented Aug 19, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 6, 2016 •

edited

Loading

yishaigalatzer commented Sep 6, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 6, 2016 •

edited

Loading

yishaigalatzer Sep 6, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 7, 2016 •

edited

Loading

dtivel commented Sep 7, 2016 •

edited by skofman1

Loading

yishaigalatzer commented Sep 7, 2016 •

edited by skofman1

Loading

xavierdecoster commented Sep 12, 2016 •

edited by skofman1

Loading

skofman1 commented Sep 12, 2016 •

edited

Loading

shishirx34 Sep 12, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 12, 2016 •

edited by skofman1

Loading

shishirx34 commented Sep 12, 2016 •

edited by skofman1

Loading

yishaigalatzer Sep 12, 2016 •

edited by skofman1

Loading