Skip to content

Commit

Permalink
Merge pull request #13 from swdee/rpc-userpass-checks
Browse files Browse the repository at this point in the history 
Place sanity checks on Username and Password length on RPC keystore.createUser method

closes #6
  • Loading branch information
@StephenButtolph
StephenButtolph authored Mar 18, 2020
2 parents 71c01fd + 0d29aed commit 58e7949
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 15 deletions.
32 changes: 30 additions & 2 deletions api/keystore/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,30 @@ import (
"github.com/ava-labs/gecko/vms/components/codec"

jsoncodec "github.com/ava-labs/gecko/utils/json"
zxcvbn "github.com/nbutton23/zxcvbn-go"
)

const (
// maxUserPassLen is the maximum length of the username or password allowed
maxUserPassLen = 1024

// requiredPassScore defines the score a password must achieve to be accepted
// as a password with strong characteristics by the zxcvbn package
//
// The scoring mechanism defined is as follows;
//
// 0 # too guessable: risky password. (guesses < 10^3)
// 1 # very guessable: protection from throttled online attacks. (guesses < 10^6)
// 2 # somewhat guessable: protection from unthrottled online attacks. (guesses < 10^8)
// 3 # safely unguessable: moderate protection from offline slow-hash scenario. (guesses < 10^10)
// 4 # very unguessable: strong protection from offline slow-hash scenario. (guesses >= 10^10)
requiredPassScore = 2
)

var (
errEmptyUsername = errors.New("username can't be the empty string")
errEmptyUsername = errors.New("username can't be the empty string")
errUserPassMaxLength = fmt.Errorf("CreateUser call rejected due to username or password exceeding maximum length of %d chars", maxUserPassLen)
errWeakPassword = errors.New("Failed to create user as the given password is too weak. A stronger password is one of 8 or more characters containing attributes of upper and lowercase letters, numbers, and/or special characters")
)

// KeyValuePair ...
Expand Down Expand Up @@ -114,7 +134,11 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre
ks.lock.Lock()
defer ks.lock.Unlock()

ks.log.Verbo("CreateUser called with %s", args.Username)
ks.log.Verbo("CreateUser called with %.*s", maxUserPassLen, args.Username)

if len(args.Username) > maxUserPassLen || len(args.Password) > maxUserPassLen {
return errUserPassMaxLength
}

if args.Username == "" {
return errEmptyUsername
Expand All @@ -123,6 +147,10 @@ func (ks *Keystore) CreateUser(_ *http.Request, args *CreateUserArgs, reply *Cre
return fmt.Errorf("user already exists: %s", args.Username)
}

if zxcvbn.PasswordStrength(args.Password, nil).Score < requiredPassScore {
return errWeakPassword
}

usr := &User{}
if err := usr.Initialize(args.Password); err != nil {
return err
Expand Down
104 changes: 92 additions & 12 deletions api/keystore/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,21 @@ package keystore

import (
"bytes"
"fmt"
"math/rand"
"testing"

"github.com/ava-labs/gecko/database/memdb"
"github.com/ava-labs/gecko/ids"
"github.com/ava-labs/gecko/utils/logging"
)

var (
// strongPassword defines a password used for the following tests that
// scores high enough to pass the password strength scoring system
strongPassword = "N_+=_jJ;^(<;{4,:*m6CET}'&N;83FYK.wtNpwp-Jt"
)

func TestServiceListNoUsers(t *testing.T) {
ks := Keystore{}
ks.Initialize(logging.NoLog{}, memdb.New())
Expand All @@ -33,7 +41,7 @@ func TestServiceCreateUser(t *testing.T) {
reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
}, &reply); err != nil {
t.Fatal(err)
}
Expand All @@ -56,6 +64,78 @@ func TestServiceCreateUser(t *testing.T) {
}
}

// genStr returns a string of given length
func genStr(n int) string {
b := make([]byte, n)
rand.Read(b)
return fmt.Sprintf("%x", b)[:n]
}

// TestServiceCreateUserArgsChecks generates excessively long usernames or
// passwords to assure the santity checks on string length are not exceeded
func TestServiceCreateUserArgsCheck(t *testing.T) {
ks := Keystore{}
ks.Initialize(logging.NoLog{}, memdb.New())

{
reply := CreateUserReply{}
err := ks.CreateUser(nil, &CreateUserArgs{
Username: genStr(maxUserPassLen + 1),
Password: strongPassword,
}, &reply)

if reply.Success || err != errUserPassMaxLength {
t.Fatal("User was created when it should have been rejected due to too long a Username, err =", err)
}
}

{
reply := CreateUserReply{}
err := ks.CreateUser(nil, &CreateUserArgs{
Username: "shortuser",
Password: genStr(maxUserPassLen + 1),
}, &reply)

if reply.Success || err != errUserPassMaxLength {
t.Fatal("User was created when it should have been rejected due to too long a Password, err =", err)
}
}

{
reply := ListUsersReply{}
if err := ks.ListUsers(nil, &ListUsersArgs{}, &reply); err != nil {
t.Fatal(err)
}

if len(reply.Users) > 0 {
t.Fatalf("A user exists when there should be none")
}
}
}

// TestServiceCreateUserWeakPassword tests creating a new user with a weak
// password to ensure the password strength check is working
func TestServiceCreateUserWeakPassword(t *testing.T) {
ks := Keystore{}
ks.Initialize(logging.NoLog{}, memdb.New())

{
reply := CreateUserReply{}
err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "weak",
}, &reply)

if err != errWeakPassword {
t.Error("Unexpected error occurred when testing weak password:", err)
}

if reply.Success {
t.Fatal("User was created when it should have been rejected due to weak password")
}
}
}

func TestServiceCreateDuplicate(t *testing.T) {
ks := Keystore{}
ks.Initialize(logging.NoLog{}, memdb.New())
Expand All @@ -64,7 +144,7 @@ func TestServiceCreateDuplicate(t *testing.T) {
reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
}, &reply); err != nil {
t.Fatal(err)
}
Expand All @@ -77,7 +157,7 @@ func TestServiceCreateDuplicate(t *testing.T) {
reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "launch!",
Password: strongPassword,
}, &reply); err == nil {
t.Fatalf("Should have errored due to the username already existing")
}
Expand All @@ -90,7 +170,7 @@ func TestServiceCreateUserNoName(t *testing.T) {

reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Password: "launch",
Password: strongPassword,
}, &reply); err == nil {
t.Fatalf("Shouldn't have allowed empty username")
}
Expand All @@ -104,7 +184,7 @@ func TestServiceUseBlockchainDB(t *testing.T) {
reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
}, &reply); err != nil {
t.Fatal(err)
}
Expand All @@ -114,7 +194,7 @@ func TestServiceUseBlockchainDB(t *testing.T) {
}

{
db, err := ks.GetDatabase(ids.Empty, "bob", "launch")
db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword)
if err != nil {
t.Fatal(err)
}
Expand All @@ -124,7 +204,7 @@ func TestServiceUseBlockchainDB(t *testing.T) {
}

{
db, err := ks.GetDatabase(ids.Empty, "bob", "launch")
db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword)
if err != nil {
t.Fatal(err)
}
Expand All @@ -144,7 +224,7 @@ func TestServiceExportImport(t *testing.T) {
reply := CreateUserReply{}
if err := ks.CreateUser(nil, &CreateUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
}, &reply); err != nil {
t.Fatal(err)
}
Expand All @@ -154,7 +234,7 @@ func TestServiceExportImport(t *testing.T) {
}

{
db, err := ks.GetDatabase(ids.Empty, "bob", "launch")
db, err := ks.GetDatabase(ids.Empty, "bob", strongPassword)
if err != nil {
t.Fatal(err)
}
Expand All @@ -166,7 +246,7 @@ func TestServiceExportImport(t *testing.T) {
exportReply := ExportUserReply{}
if err := ks.ExportUser(nil, &ExportUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
}, &exportReply); err != nil {
t.Fatal(err)
}
Expand All @@ -178,7 +258,7 @@ func TestServiceExportImport(t *testing.T) {
reply := ImportUserReply{}
if err := newKS.ImportUser(nil, &ImportUserArgs{
Username: "bob",
Password: "launch",
Password: strongPassword,
User: exportReply.User,
}, &reply); err != nil {
t.Fatal(err)
Expand All @@ -189,7 +269,7 @@ func TestServiceExportImport(t *testing.T) {
}

{
db, err := newKS.GetDatabase(ids.Empty, "bob", "launch")
db, err := newKS.GetDatabase(ids.Empty, "bob", strongPassword)
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion utils/logging/log.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (l *Log) format(level Level, format string, args ...interface{}) string {

return fmt.Sprintf("%s[%s]%s %s\n",
level,
time.Now().Format("01-02|15:04:05.000"),
time.Now().Format("01-02|15:04:05"),
prefix,
text)
}
Expand Down

0 comments on commit 58e7949

Please sign in to comment.