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

feat: Bcrypt algorithm support #1169

Merged
merged 14 commits into from
Apr 1, 2021
6 changes: 6 additions & 0 deletions driver/config/.schema/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1294,6 +1294,12 @@
"title": "Hashing Algorithm Configuration",
"type": "object",
"properties": {
"algorithm": {
"title": "Password hashing algorithm",
"description": "One of the values: argon2, bcrypt",
"type": "string",
"default": "argon2"
seremenko-wish marked this conversation as resolved.
Show resolved Hide resolved
},
seremenko-wish marked this conversation as resolved.
Show resolved Hide resolved
"argon2": {
"title": "Configuration for the Argon2id hasher.",
"type": "object",
Expand Down
27 changes: 27 additions & 0 deletions driver/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const (
DefaultIdentityTraitsSchemaID = "default"
DefaultBrowserReturnURL = "default_browser_return_url"
DefaultSQLiteMemoryDSN = dbal.SQLiteInMemory
DefaultPasswordHashingAlgorithm = "argon2"
UnknownVersion = "unknown version"
ViperKeyDSN = "dsn"
ViperKeyCourierSMTPURL = "courier.smtp.connection_uri"
Expand Down Expand Up @@ -84,18 +85,21 @@ const (
ViperKeySelfServiceVerificationBrowserDefaultReturnTo = "selfservice.flows.verification.after." + DefaultBrowserReturnURL
ViperKeyDefaultIdentitySchemaURL = "identity.default_schema_url"
ViperKeyIdentitySchemas = "identity.schemas"
ViperKeyHasherAlgorithm = "hashers.algorithm"
ViperKeyHasherArgon2ConfigMemory = "hashers.argon2.memory"
ViperKeyHasherArgon2ConfigIterations = "hashers.argon2.iterations"
ViperKeyHasherArgon2ConfigParallelism = "hashers.argon2.parallelism"
ViperKeyHasherArgon2ConfigSaltLength = "hashers.argon2.salt_length"
ViperKeyHasherArgon2ConfigKeyLength = "hashers.argon2.key_length"
ViperKeyHasherBcryptCost = "hashers.bcrypt.cost"
ViperKeyPasswordMaxBreaches = "selfservice.methods.password.config.max_breaches"
ViperKeyIgnoreNetworkErrors = "selfservice.methods.password.config.ignore_network_errors"
ViperKeyVersion = "version"
Argon2DefaultMemory uint32 = 4 * 1024 * 1024
Argon2DefaultIterations uint32 = 4
Argon2DefaultSaltLength uint32 = 16
Argon2DefaultKeyLength uint32 = 32
BcryptDefaultCost uint32 = 12
)

// DefaultSessionCookieName returns the default cookie name for the kratos session.
Expand All @@ -109,6 +113,9 @@ type (
SaltLength uint32 `json:"salt_length"`
KeyLength uint32 `json:"key_length"`
}
Bcrypt struct {
Cost uint32 `json:"cost"`
}
SelfServiceHook struct {
Name string `json:"hook"`
Config json.RawMessage `json:"config"`
Expand Down Expand Up @@ -235,6 +242,15 @@ func (p *Config) HasherArgon2() *Argon2 {
}
}


func (p *Config) HasherBcrypt() *Bcrypt {
// warn about usage of default values and point to the docs
// warning will require https://github.com/ory/viper/issues/19
return &Bcrypt{
Cost: uint32(p.p.IntF(ViperKeyHasherBcryptCost, int(BcryptDefaultCost))),
}
}

func (p *Config) listenOn(key string) string {
fb := 4433
if key == "admin" {
Expand Down Expand Up @@ -732,3 +748,14 @@ func (p *Config) PasswordPolicyConfig() *PasswordPolicy {
IgnoreNetworkErrors: p.p.BoolF(ViperKeyIgnoreNetworkErrors, true),
}
}

func (p *Config) HasherPasswordHashingAlgorithm() string {
configValue := p.p.StringF(ViperKeyHasherAlgorithm, DefaultPasswordHashingAlgorithm)
switch configValue {
case "bcrypt":
return configValue
case "argon2":
seremenko-wish marked this conversation as resolved.
Show resolved Hide resolved
default:
return configValue
}
}
6 changes: 5 additions & 1 deletion driver/registry_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,11 @@ func (m *RegistryDefault) SessionHandler() *session.Handler {

func (m *RegistryDefault) Hasher() hash.Hasher {
if m.passwordHasher == nil {
m.passwordHasher = hash.NewHasherArgon2(m)
if m.c.HasherPasswordHashingAlgorithm() == "bcrypt" {
m.passwordHasher = hash.NewHasherBcrypt(m)
} else {
m.passwordHasher = hash.NewHasherArgon2(m)
}
}
return m.passwordHasher
}
Expand Down
56 changes: 56 additions & 0 deletions hash/hasher_bcrypt.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package hash

import (
"context"
"github.com/ory/kratos/driver/config"
"github.com/pkg/errors"
"golang.org/x/crypto/bcrypt"
)

type Bcrypt struct {
c BcryptConfiguration
}

type BcryptConfiguration interface {
config.Provider
}

func NewHasherBcrypt(c BcryptConfiguration) *Bcrypt {
return &Bcrypt{c: c}
}

func (h *Bcrypt) Generate(ctx context.Context, password []byte) ([]byte, error) {
if err := validatePasswordLength(password); err != nil {
return nil, err
}

hash, err := bcrypt.GenerateFromPassword(password, int(h.c.Config(ctx).HasherBcrypt().Cost))
if err != nil {
return nil, err
}

return hash, nil
}

func (h *Bcrypt) Compare(_ context.Context, password []byte, hash []byte) error {
if err := validatePasswordLength(password); err != nil {
return err
}

err := bcrypt.CompareHashAndPassword(hash, password)
if err != nil {
return err
}

return nil
}

func validatePasswordLength(password []byte) error {
// Bcrypt truncates the password to the first 72 bytes, following the OpenBSD implementation,
// so if password is longer than 72 bytes, function returns an error
// See https://en.wikipedia.org/wiki/Bcrypt#User_input
if len(password) > 72 {
return errors.New("passwords are limited to a maximum length of 72 characters")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error will be passed down to the UI I guess. Please add it to the messages to allow translations and mapping to other text: https://github.com/ory/kratos/blob/1940a679eb6b695e22cb939fb0d8d85cebb82a1e/text/message_registration.go

I guess you would then in the flow check for this error you return here similar to

a.Messages.Add(text.NewErrorValidationRegistrationFlowExpired(e.ago))

But maybe @aeneasr can give better pointers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it would be an error like this one (actually we can just add it do that file for now): https://github.com/ory/kratos/blob/master/schema/errors.go#L59-L70

}
return nil
}
91 changes: 90 additions & 1 deletion hash/hasher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func mkpw(t *testing.T, length int) []byte {
return pw
}

func TestHasher(t *testing.T) {
func TestArgonHasher(t *testing.T) {
for k, pw := range [][]byte{
mkpw(t, 8),
mkpw(t, 16),
Expand Down Expand Up @@ -50,3 +50,92 @@ func TestHasher(t *testing.T) {
})
}
}

func TestBcryptHasherGeneratesErrorWhenPasswordIsLong(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
hasher := hash.NewHasherBcrypt(reg)

password := mkpw(t, 73)
res, err := hasher.Generate(context.Background(), password)

assert.Error(t, err, "password is too long")
assert.Nil(t, res)
}

func TestBcryptHasherGeneratesHash(t *testing.T) {
for k, pw := range [][]byte{
mkpw(t, 8),
mkpw(t, 16),
mkpw(t, 32),
mkpw(t, 64),
mkpw(t, 72),
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
hasher := hash.NewHasherBcrypt(reg)
res, err := hasher.Generate(context.Background(), pw)

assert.Nil(t, err)

// Valid format: $2a$12$[22 character salt][31 character hash]
assert.Equal(t, 60, len(string(res)), "invalid bcrypt hash length")
assert.Equal(t, "$2a$12$", string(res)[:7], "invalid bcrypt identifier")
})
}
}


func TestBcryptHasherComparesFailsWhenPasswordIsTooLong(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
hasher := hash.NewHasherBcrypt(reg)

password := mkpw(t, 73)
err := hasher.Compare(context.Background(), password, []byte("hash"))

assert.Error(t, err, "password is too long")
}


func TestBcryptHasherComparesHashSuccess(t *testing.T) {
for k, pw := range [][]byte{
mkpw(t, 8),
mkpw(t, 16),
mkpw(t, 32),
mkpw(t, 64),
mkpw(t, 72),
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
hasher := hash.NewHasherBcrypt(reg)

hs, err := hasher.Generate(context.Background(), pw)

assert.Nil(t, err)

err = hasher.Compare(context.Background(), pw, hs)
assert.Nil(t, err, "hash validation fails")
})
}
}

func TestBcryptHasherComparesHashFail(t *testing.T) {
for k, pw := range [][]byte{
mkpw(t, 8),
mkpw(t, 16),
mkpw(t, 32),
mkpw(t, 64),
mkpw(t, 72),
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
hasher := hash.NewHasherBcrypt(reg)

mod := make([]byte, len(pw))
copy(mod, pw)
mod[len(pw)-1] = ^pw[len(pw)-1]

err := hasher.Compare(context.Background(), pw, mod)
assert.Error(t, err)
})
}
}