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

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

Merged
merged 10 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion custom/conf/app.ini.sample
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,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 @@ -219,7 +219,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
55 changes: 34 additions & 21 deletions modules/password/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,58 @@ 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
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
var validChars string
var validComplexities = map[string]string{
"lower": "abcdefghijklmnopqrstuvwxyz",
"upper": "ABCDEFGHIJKLMNOPQRSTUVWXYZ",
"digit": "0123456789",
"spec": `][ !"#$%&'()*+,./:;<=>?@\^_{|}~` + "`-",
var requiredChars []string

var 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 {
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -149,7 +149,7 @@ var (
MinPasswordLength int
ImportLocalPaths bool
DisableGitHooks bool
PasswordComplexity map[string]string
PasswordComplexity []string
PasswordHashAlgo string

// UI settings
Expand Down Expand Up @@ -781,26 +781,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