Skip to content

Commit

Permalink
Fix password complexity regex for special characters (backport for v1…
Browse files Browse the repository at this point in the history
….10.0) (#8524)

* 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
  • Loading branch information
guillep2k authored and lunny committed Oct 16, 2019
1 parent db0d4ff commit 595033f
Show file tree
Hide file tree
Showing 9 changed files with 140 additions and 72 deletions.
3 changes: 2 additions & 1 deletion custom/conf/app.ini.sample
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,8 @@ IMPORT_LOCAL_PATHS = false
; Set to true to prevent all users (including admin) from creating custom git hooks
DISABLE_GIT_HOOKS = false
;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 Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt"
PASSWORD_HASH_ALGO = pbkdf2
Expand Down
3 changes: 2 additions & 1 deletion docs/content/doc/advanced/config-cheat-sheet.en-us.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,8 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- lower - use one or more lower latin characters
- upper - use one or more upper latin characters
- 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`)

Expand Down
63 changes: 39 additions & 24 deletions modules/password/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,60 @@ package password
import (
"crypto/rand"
"math/big"
"regexp"
"strings"
"sync"

"code.gitea.io/gitea/modules/setting"
)

var matchComplexities = map[string]regexp.Regexp{}
var matchComplexityOnce sync.Once
var validChars string
var validComplexities = map[string]string{
"lower": "abcdefghijklmnopqrstuvwxyz",
"upper": "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
"digit": "0123456789",
"spec": `][ !"#$%&'()*+,./:;<=>?@\^_{|}~` + "`-",
}
var (
matchComplexityOnce sync.Once
validChars string
requiredChars []string

charComplexities = map[string]string{
"lower": `abcdefghijklmnopqrstuvwxyz`,
"upper": `ABCDEFGHIJKLMNOPQRSTUVWXYZ`,
"digit": `0123456789`,
"spec": ` !"#$%&'()*+,-./:;<=>?@[\]^_{|}~` + "`",
}
)

// NewComplexity for preparation
func NewComplexity() {
matchComplexityOnce.Do(func() {
if len(setting.PasswordComplexity) > 0 {
for key, val := range setting.PasswordComplexity {
matchComplexity := regexp.MustCompile(val)
matchComplexities[key] = *matchComplexity
validChars += validComplexities[key]
setupComplexity(setting.PasswordComplexity)
})
}

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)
}
} else {
for _, val := range validComplexities {
validChars += val
}
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 is Complexity
// IsComplexEnough return True if password meets complexity settings
func IsComplexEnough(pwd string) bool {
if len(setting.PasswordComplexity) > 0 {
NewComplexity()
for _, val := range matchComplexities {
if !val.MatchString(pwd) {
NewComplexity()
if len(validChars) > 0 {
for _, req := range requiredChars {
if !strings.ContainsAny(req, pwd) {
return false
}
}
Expand Down
75 changes: 75 additions & 0 deletions modules/password/password_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
24 changes: 6 additions & 18 deletions modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ var (
MinPasswordLength int
ImportLocalPaths bool
DisableGitHooks bool
PasswordComplexity map[string]string
PasswordComplexity []string
PasswordHashAlgo string

// UI settings
Expand Down Expand Up @@ -775,26 +775,14 @@ func NewContext() {

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(",")
for _, y := range cfgdata {
ts := strings.TrimSpace(y)
for a := range dictPC {
if strings.ToLower(ts) == a {
PasswordComplexity[ts] = dictPC[ts]
break
}
PasswordComplexity = make([]string, 0, len(cfgdata))
for _, name := range cfgdata {
name := strings.ToLower(strings.Trim(name, `"`))
if name != "" {
PasswordComplexity = append(PasswordComplexity, name)
}
}
if len(PasswordComplexity) == 0 {
PasswordComplexity = dictPC
}

sec = Cfg.Section("attachment")
AttachmentPath = sec.Key("PATH").MustString(path.Join(AppDataPath, "attachments"))
Expand Down
2 changes: 1 addition & 1 deletion options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ team_no_units_error = Allow access to at least one repository section.
email_been_used = The email address is already used.
openid_been_used = The OpenID address '%s' is already used.
username_password_incorrect = Username or password is incorrect.
password_complexity = Password does not pass complexity requirements.
password_complexity = Password does not pass complexity requirements.
enterred_invalid_repo_name = The repository name you entered is incorrect.
enterred_invalid_owner_name = The new owner name is not valid.
enterred_invalid_password = The password you entered is incorrect.
Expand Down
4 changes: 2 additions & 2 deletions routers/admin/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestNewUserPost_MustChangePassword(t *testing.T) {
LoginName: "local",
UserName: username,
Email: email,
Password: "xxxxxxxx",
Password: "abc123ABC!=$",
SendNotify: false,
MustChangePassword: true,
}
Expand Down Expand Up @@ -71,7 +71,7 @@ func TestNewUserPost_MustChangePasswordFalse(t *testing.T) {
LoginName: "local",
UserName: username,
Email: email,
Password: "xxxxxxxx",
Password: "abc123ABC!=$",
SendNotify: false,
MustChangePassword: false,
}
Expand Down
2 changes: 1 addition & 1 deletion routers/user/setting/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) {
} else if form.Password != form.Retype {
ctx.Flash.Error(ctx.Tr("form.password_not_match"))
} else if !password.IsComplexEnough(form.Password) {
ctx.Flash.Error(ctx.Tr("settings.password_complexity"))
ctx.Flash.Error(ctx.Tr("form.password_complexity"))
} else {
var err error
if ctx.User.Salt, err = models.GetUserSalt(); err != nil {
Expand Down
36 changes: 12 additions & 24 deletions routers/user/setting/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,76 +19,64 @@ import (
func TestChangePassword(t *testing.T) {
oldPassword := "password"
setting.MinPasswordLength = 6
setting.PasswordComplexity = map[string]string{
"lower": "[a-z]+",
"upper": "[A-Z]+",
"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]+",
}
var pcALL = []string{"lower", "upper", "digit", "spec"}
var pcLUN = []string{"lower", "upper", "digit"}
var pcLU = []string{"lower", "upper"}

for _, req := range []struct {
OldPassword string
NewPassword string
Retype string
Message string
PasswordComplexity map[string]string
PasswordComplexity []string
}{
{
OldPassword: oldPassword,
NewPassword: "Qwerty123456-",
Retype: "Qwerty123456-",
Message: "",
PasswordComplexity: setting.PasswordComplexity,
PasswordComplexity: pcALL,
},
{
OldPassword: oldPassword,
NewPassword: "12345",
Retype: "12345",
Message: "auth.password_too_short",
PasswordComplexity: setting.PasswordComplexity,
PasswordComplexity: pcALL,
},
{
OldPassword: "12334",
NewPassword: "123456",
Retype: "123456",
Message: "settings.password_incorrect",
PasswordComplexity: setting.PasswordComplexity,
PasswordComplexity: pcALL,
},
{
OldPassword: oldPassword,
NewPassword: "123456",
Retype: "12345",
Message: "form.password_not_match",
PasswordComplexity: setting.PasswordComplexity,
PasswordComplexity: pcALL,
},
{
OldPassword: oldPassword,
NewPassword: "Qwerty",
Retype: "Qwerty",
Message: "settings.password_complexity",
PasswordComplexity: setting.PasswordComplexity,
Message: "form.password_complexity",
PasswordComplexity: pcALL,
},
{
OldPassword: oldPassword,
NewPassword: "Qwerty",
Retype: "Qwerty",
Message: "settings.password_complexity",
Message: "form.password_complexity",
PasswordComplexity: pcLUN,
},
{
OldPassword: oldPassword,
NewPassword: "QWERTY",
Retype: "QWERTY",
Message: "settings.password_complexity",
Message: "form.password_complexity",
PasswordComplexity: pcLU,
},
} {
Expand Down

0 comments on commit 595033f

Please sign in to comment.