Fix password complexity regex for special characters (on master) (#8525)

* Fix extra space

* Fix regular expression

* Fix error template name

* Simplify check code, fix default values, add test

* Fix router tests

* Fix fmt

* Fix setting and lint

* Move cleaning up code to test, improve comments

* Tidy up variable declaration
This commit is contained in:
guillep2k 2019-10-16 00:09:58 -03:00 committed by Lunny Xiao
parent 66e99d722a
commit 31655aabfc
9 changed files with 142 additions and 74 deletions

View file

@ -345,7 +345,8 @@ IMPORT_LOCAL_PATHS = false
; Set to true to prevent all users (including admin) from creating custom git hooks ; Set to true to prevent all users (including admin) from creating custom git hooks
DISABLE_GIT_HOOKS = false DISABLE_GIT_HOOKS = false
;Comma separated list of character classes required to pass minimum complexity. ;Comma separated list of character classes required to pass minimum complexity.
;If left empty or no valid values are specified, the default values (`lower,upper,digit,spec`) will be used. ;If left empty or no valid values are specified, the default values ("lower,upper,digit,spec") will be used.
;Use "off" to disable checking.
PASSWORD_COMPLEXITY = lower,upper,digit,spec PASSWORD_COMPLEXITY = lower,upper,digit,spec
; Password Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt" ; Password Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt"
PASSWORD_HASH_ALGO = pbkdf2 PASSWORD_HASH_ALGO = pbkdf2

View file

@ -219,7 +219,8 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- lower - use one or more lower latin characters - lower - use one or more lower latin characters
- upper - use one or more upper latin characters - upper - use one or more upper latin characters
- digit - use one or more digits - digit - use one or more digits
- spec - use one or more special characters as ``][!"#$%&'()*+,./:;<=>?@\^_{|}~`-`` and space symbol. - spec - use one or more special characters as ``!"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~``
- off - do not check password complexity
## OpenID (`openid`) ## OpenID (`openid`)

View file

@ -7,45 +7,60 @@ package password
import ( import (
"crypto/rand" "crypto/rand"
"math/big" "math/big"
"regexp" "strings"
"sync" "sync"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
) )
var matchComplexities = map[string]regexp.Regexp{} var (
var matchComplexityOnce sync.Once matchComplexityOnce sync.Once
var validChars string validChars string
var validComplexities = map[string]string{ requiredChars []string
"lower": "abcdefghijklmnopqrstuvwxyz",
"upper": "ABCDEFGHIJKLMNOPQRSTUVWXYZ", charComplexities = map[string]string{
"digit": "0123456789", "lower": `abcdefghijklmnopqrstuvwxyz`,
"spec": `][ !"#$%&'()*+,./:;<=>?@\^_{|}~` + "`-", "upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`,
"digit": `0123456789`,
"spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`",
} }
)
// NewComplexity for preparation // NewComplexity for preparation
func NewComplexity() { func NewComplexity() {
matchComplexityOnce.Do(func() { matchComplexityOnce.Do(func() {
if len(setting.PasswordComplexity) > 0 { setupComplexity(setting.PasswordComplexity)
for key, val := range setting.PasswordComplexity {
matchComplexity := regexp.MustCompile(val)
matchComplexities[key] = *matchComplexity
validChars += validComplexities[key]
}
} else {
for _, val := range validComplexities {
validChars += val
}
}
}) })
} }
// IsComplexEnough return True if password is Complexity func setupComplexity(values []string) {
if len(values) != 1 || values[0] != "off" {
for _, val := range values {
if chars, ok := charComplexities[val]; ok {
validChars += chars
requiredChars = append(requiredChars, chars)
}
}
if len(requiredChars) == 0 {
// No valid character classes found; use all classes as default
for _, chars := range charComplexities {
validChars += chars
requiredChars = append(requiredChars, chars)
}
}
}
if validChars == "" {
// No complexities to check; provide a sensible default for password generation
validChars = charComplexities["lower"] + charComplexities["upper"] + charComplexities["digit"]
}
}
// IsComplexEnough return True if password meets complexity settings
func IsComplexEnough(pwd string) bool { func IsComplexEnough(pwd string) bool {
if len(setting.PasswordComplexity) > 0 {
NewComplexity() NewComplexity()
for _, val := range matchComplexities { if len(validChars) > 0 {
if !val.MatchString(pwd) { for _, req := range requiredChars {
if !strings.ContainsAny(req, pwd) {
return false return false
} }
} }

View file

@ -0,0 +1,75 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package password
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestComplexity_IsComplexEnough(t *testing.T) {
matchComplexityOnce.Do(func() {})
testlist := []struct {
complexity []string
truevalues []string
falsevalues []string
}{
{[]string{"lower"}, []string{"abc", "abc!"}, []string{"ABC", "123", "=!$", ""}},
{[]string{"upper"}, []string{"ABC"}, []string{"abc", "123", "=!$", "abc!", ""}},
{[]string{"digit"}, []string{"123"}, []string{"abc", "ABC", "=!$", "abc!", ""}},
{[]string{"spec"}, []string{"=!$", "abc!"}, []string{"abc", "ABC", "123", ""}},
{[]string{"off"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}, nil},
{[]string{"lower", "spec"}, []string{"abc!"}, []string{"abc", "ABC", "123", "=!$", "abcABC123", ""}},
{[]string{"lower", "upper", "digit"}, []string{"abcABC123"}, []string{"abc", "ABC", "123", "=!$", "abc!", ""}},
}
for _, test := range testlist {
testComplextity(test.complexity)
for _, val := range test.truevalues {
assert.True(t, IsComplexEnough(val))
}
for _, val := range test.falsevalues {
assert.False(t, IsComplexEnough(val))
}
}
// Remove settings for other tests
testComplextity([]string{"off"})
}
func TestComplexity_Generate(t *testing.T) {
matchComplexityOnce.Do(func() {})
const maxCount = 50
const pwdLen = 50
test := func(t *testing.T, modes []string) {
testComplextity(modes)
for i := 0; i < maxCount; i++ {
pwd, err := Generate(pwdLen)
assert.NoError(t, err)
assert.Equal(t, pwdLen, len(pwd))
assert.True(t, IsComplexEnough(pwd), "Failed complexities with modes %+v for generated: %s", modes, pwd)
}
}
test(t, []string{"lower"})
test(t, []string{"upper"})
test(t, []string{"lower", "upper", "spec"})
test(t, []string{"off"})
test(t, []string{""})
// Remove settings for other tests
testComplextity([]string{"off"})
}
func testComplextity(values []string) {
// Cleanup previous values
validChars = ""
requiredChars = make([]string, 0, len(values))
setupComplexity(values)
}

View file

@ -149,7 +149,7 @@ var (
MinPasswordLength int MinPasswordLength int
ImportLocalPaths bool ImportLocalPaths bool
DisableGitHooks bool DisableGitHooks bool
PasswordComplexity map[string]string PasswordComplexity []string
PasswordHashAlgo string PasswordHashAlgo string
// UI settings // UI settings
@ -781,26 +781,14 @@ func NewContext() {
InternalToken = loadInternalToken(sec) InternalToken = loadInternalToken(sec)
var dictPC = map[string]string{
"lower": "[a-z]+",
"upper": "[A-Z]+",
"digit": "[0-9]+",
"spec": `][ !"#$%&'()*+,./:;<=>?@\\^_{|}~` + "`-",
}
PasswordComplexity = make(map[string]string)
cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",")
for _, y := range cfgdata { PasswordComplexity = make([]string, 0, len(cfgdata))
ts := strings.TrimSpace(y) for _, name := range cfgdata {
for a := range dictPC { name := strings.ToLower(strings.Trim(name, `"`))
if strings.ToLower(ts) == a { if name != "" {
PasswordComplexity[ts] = dictPC[ts] PasswordComplexity = append(PasswordComplexity, name)
break
} }
} }
}
if len(PasswordComplexity) == 0 {
PasswordComplexity = dictPC
}
sec = Cfg.Section("attachment") sec = Cfg.Section("attachment")
AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments")) AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments"))

View file

@ -34,7 +34,7 @@ func TestNewUserPost_MustChangePassword(t *testing.T) {
LoginName: "local", LoginName: "local",
UserName: username, UserName: username,
Email: email, Email: email,
Password: "xxxxxxxx", Password: "abc123ABC!=$",
SendNotify: false, SendNotify: false,
MustChangePassword: true, MustChangePassword: true,
} }
@ -71,7 +71,7 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) {
LoginName: "local", LoginName: "local",
UserName: username, UserName: username,
Email: email, Email: email,
Password: "xxxxxxxx", Password: "abc123ABC!=$",
SendNotify: false, SendNotify: false,
MustChangePassword: false, MustChangePassword: false,
} }

View file

@ -54,7 +54,7 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) {
} else if form.Password != form.Retype { } else if form.Password != form.Retype {
ctx.Flash.Error(ctx.Tr("form.password_not_match")) ctx.Flash.Error(ctx.Tr("form.password_not_match"))
} else if !password.IsComplexEnough(form.Password) { } else if !password.IsComplexEnough(form.Password) {
ctx.Flash.Error(ctx.Tr("settings.password_complexity")) ctx.Flash.Error(ctx.Tr("form.password_complexity"))
} else { } else {
var err error var err error
if ctx.User.Salt, err = models.GetUserSalt(); err != nil { if ctx.User.Salt, err = models.GetUserSalt(); err != nil {

View file

@ -19,76 +19,64 @@ import (
func TestChangePassword(t *testing.T) { func TestChangePassword(t *testing.T) {
oldPassword := "password" oldPassword := "password"
setting.MinPasswordLength = 6 setting.MinPasswordLength = 6
setting.PasswordComplexity = map[string]string{ var pcALL = []string{"lower", "upper", "digit", "spec"}
"lower": "[a-z]+", var pcLUN = []string{"lower", "upper", "digit"}
"upper": "[A-Z]+", var pcLU = []string{"lower", "upper"}
"digit": "[0-9]+",
"spec": "[-_]+",
}
var pcLUN = map[string]string{
"lower": "[a-z]+",
"upper": "[A-Z]+",
"digit": "[0-9]+",
}
var pcLU = map[string]string{
"lower": "[a-z]+",
"upper": "[A-Z]+",
}
for _, req := range []struct { for _, req := range []struct {
OldPassword string OldPassword string
NewPassword string NewPassword string
Retype string Retype string
Message string Message string
PasswordComplexity map[string]string PasswordComplexity []string
}{ }{
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "Qwerty123456-", NewPassword: "Qwerty123456-",
Retype: "Qwerty123456-", Retype: "Qwerty123456-",
Message: "", Message: "",
PasswordComplexity: setting.PasswordComplexity, PasswordComplexity: pcALL,
}, },
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "12345", NewPassword: "12345",
Retype: "12345", Retype: "12345",
Message: "auth.password_too_short", Message: "auth.password_too_short",
PasswordComplexity: setting.PasswordComplexity, PasswordComplexity: pcALL,
}, },
{ {
OldPassword: "12334", OldPassword: "12334",
NewPassword: "123456", NewPassword: "123456",
Retype: "123456", Retype: "123456",
Message: "settings.password_incorrect", Message: "settings.password_incorrect",
PasswordComplexity: setting.PasswordComplexity, PasswordComplexity: pcALL,
}, },
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "123456", NewPassword: "123456",
Retype: "12345", Retype: "12345",
Message: "form.password_not_match", Message: "form.password_not_match",
PasswordComplexity: setting.PasswordComplexity, PasswordComplexity: pcALL,
}, },
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "Qwerty", NewPassword: "Qwerty",
Retype: "Qwerty", Retype: "Qwerty",
Message: "settings.password_complexity", Message: "form.password_complexity",
PasswordComplexity: setting.PasswordComplexity, PasswordComplexity: pcALL,
}, },
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "Qwerty", NewPassword: "Qwerty",
Retype: "Qwerty", Retype: "Qwerty",
Message: "settings.password_complexity", Message: "form.password_complexity",
PasswordComplexity: pcLUN, PasswordComplexity: pcLUN,
}, },
{ {
OldPassword: oldPassword, OldPassword: oldPassword,
NewPassword: "QWERTY", NewPassword: "QWERTY",
Retype: "QWERTY", Retype: "QWERTY",
Message: "settings.password_complexity", Message: "form.password_complexity",
PasswordComplexity: pcLU, PasswordComplexity: pcLU,
}, },
} { } {