Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password Complexity Checks #6230

Open
wants to merge 41 commits into
base: master
from

Conversation

@T-M-A
Copy link

commented Mar 3, 2019

  • Password reset
  • Admin user creation page
  • Admin user creation API
  • CLI user creation
  • add generate.GetRandomPassword

T-M-A added some commits Mar 3, 2019

@T-M-A T-M-A changed the title Password Complexity Checks (fix unit tests) Password Complexity Checks Mar 3, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Mar 3, 2019

Codecov Report

Merging #6230 into master will decrease coverage by 0.05%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6230      +/-   ##
==========================================
- Coverage   41.41%   41.36%   -0.06%     
==========================================
  Files         432      432              
  Lines       59552    59650      +98     
==========================================
+ Hits        24664    24672       +8     
- Misses      31648    31736      +88     
- Partials     3240     3242       +2
Impacted Files Coverage Δ
modules/generate/generate.go 17.52% <0%> (-16.48%) ⬇️
routers/user/auth.go 13.03% <0%> (-0.02%) ⬇️
routers/admin/users.go 30% <0%> (-1.19%) ⬇️
routers/api/v1/admin/user.go 28.22% <0%> (-1.02%) ⬇️
modules/util/util.go 41.66% <0%> (-23.56%) ⬇️
routers/user/setting/account.go 25.65% <0%> (-1.01%) ⬇️
modules/setting/setting.go 48.25% <70.58%> (+0.68%) ⬆️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
routers/repo/view.go 42.02% <0%> (-1.02%) ⬇️
modules/log/file.go 77.62% <0%> (+2.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6db3dc7...224b2db. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label Mar 3, 2019

Show resolved Hide resolved options/locale/locale_ru-RU.ini Outdated
Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
Show resolved Hide resolved options/locale/locale_en-US.ini Outdated
Show resolved Hide resolved routers/user/setting/account.go Outdated
@adelowo

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Will it also be possible to hide this behind a config flag. I use a
password manager and I occasionally generate passwords without upper cased characters.
Just lower case, numbers, and special character.

Sometimes, it is just uppercase and special characters. Looks like this scenario will fail

https://github.com/go-gitea/gitea/pull/6230/files#diff-8a33fa342698bd4ebdbbfdf4f4691be7R52

Not a big deal as I can always generate one that passes this check but it is going to be a
false positive. The password I generate (e.g agxh@b@im66qczv@m6qp%3qvzjs$i7k^wzfuca8fe6^eofen6wp7hjwy4sg!h^f9wun3to4$ye3e&$a ) is going to be complex enough but will fail this check as it doesn't have an upper case character

@T-M-A

This comment has been minimized.

Copy link
Author

commented Mar 3, 2019

I'm sorry I'm new
I will try to remake

Update options/locale/locale_en-US.ini
Co-Authored-By: T-M-A <maxim.tkachenko@gmail.com>
@jolheiser
Copy link
Member

left a comment

Small typo nit

Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
Show resolved Hide resolved modules/util/pwd_complexity.go Outdated
@lunny

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I prefer use a mature golang library to do that but not maintain it ourselves.

@T-M-A

This comment has been minimized.

Copy link
Owner Author

commented on routers/user/setting/account.go in f49a9a1 Mar 4, 2019

if I use as
var matchComplexity = regexp.MustCompile(setting.PasswordComplexity)
password check true any always

@adelowo Help me please

@adelowo

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@T-M-A That's most likely because the value of PasswordComplexity has not been parsed from the config file before matchComplexity = regexp.MustCompile(setting.PasswordComplexity) runs, so it happens to be an empty string. If I remember correctly, setting a package level variable has the same kind of effect as putting code in func init

@adelowo

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Maybe a fix can be


var matchComplexity *regexp.Regexp

var matchComplexityOnce sync.Once

func Account(ctx *context.Context) {
     matchComplexityOnce.Do(func() {
         matchComplexity = regexp.MustCompile(setting.PasswordComplexity)
     })
    // codes
}

@adelowo

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

By this check should be added :

  • Password reset
  • Admin user creation page
  • Admin user creation API
  • CLI user creation

@lafriks lafriks added the kind/feature label Mar 4, 2019

@lafriks lafriks added this to the 1.9.0 milestone Mar 4, 2019

T-M-A added some commits Mar 5, 2019

@@ -985,6 +986,11 @@ func NewContext() {
log.Fatal(4, "Error saving generated JWT Secret to custom config: %v", err)
}
}

PasswordComplexity = sec.Key("PASSWORD_COMPLEXITY").MustString("[a-z]+[A-Z]+[0-9_]+[^A-Za-z0-9_]+")
if len(PasswordComplexity) == 0 {

This comment has been minimized.

Copy link
@adelowo

adelowo Mar 5, 2019

Member

You don't need this again as you have passed the default value to MustString

Show resolved Hide resolved modules/util/util.go Outdated

T-M-A added some commits Mar 7, 2019

@T-M-A

This comment has been minimized.

Copy link
Author

commented Mar 7, 2019

@adelowo , Please, check my last implementation.

Show resolved Hide resolved custom/conf/app.ini.sample
Show resolved Hide resolved modules/util/util.go

T-M-A added some commits Mar 8, 2019

fix
@T-M-A

This comment has been minimized.

Copy link
Author

commented May 3, 2019

@adelowo, check new fix again, please

Show resolved Hide resolved cmd/admin.go Outdated
Show resolved Hide resolved modules/generate/generate.go Outdated
Show resolved Hide resolved modules/setting/setting.go Outdated

T-M-A added some commits May 9, 2019

fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.