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.

Sign up for GitHub

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: avoid excessive memory allocations #2389

Merged
merged 6 commits into from
Apr 15, 2022
Merged

fix: avoid excessive memory allocations #2389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a012ef2
fix: use ristretto cache to store HIBP API calls results
harnash Apr 11, 2022
2363879
fix: update tests
harnash Apr 12, 2022
50f7a30
Use ignoreInternalCost=true for ristretto cache setup
harnash Apr 12, 2022
c9183ec
fix: goimports
harnash Apr 12, 2022
c2bd607
Merge branch 'master' into avoid_excessive_memory_allocations
harnash Apr 13, 2022
1f2c9e1
Remove Wait() calls
harnash Apr 15, 2022
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
6 changes: 5 additions & 1 deletion driver/registry_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,11 @@ func (m *RegistryDefault) Hasher() hash.Hasher {

func (m *RegistryDefault) PasswordValidator() password2.Validator {
if m.passwordValidator == nil {
m.passwordValidator = password2.NewDefaultPasswordValidatorStrategy(m)
var err error
m.passwordValidator, err = password2.NewDefaultPasswordValidatorStrategy(m)
if err != nil {
m.Logger().WithError(err).Fatal("could not initialize DefaultPasswordValidator")
}
}
return m.passwordValidator
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ require (
github.com/cortesi/modd v0.0.0-20210323234521-b35eddab86cc
github.com/davecgh/go-spew v1.1.1
github.com/davidrjonas/semver-cli v0.0.0-20190116233701-ee19a9a0dda6
github.com/dgraph-io/ristretto v0.1.0
github.com/duo-labs/webauthn v0.0.0-20220330035159-03696f3d4499
github.com/fatih/color v1.13.0
github.com/form3tech-oss/jwt-go v3.2.3+incompatible
Expand Down
39 changes: 22 additions & 17 deletions selfservice/strategy/password/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import (
"net/http"
"strconv"
"strings"
"sync"
"time"

"github.com/arbovm/levenshtein"
"github.com/dgraph-io/ristretto"
"github.com/hashicorp/go-retryablehttp"
"github.com/pkg/errors"

Expand All @@ -22,6 +22,8 @@ import (
"github.com/ory/x/httpx"
)

const hashCacheItemTTL = time.Hour

// Validator implements a validation strategy for passwords. One example is that the password
// has to have at least 6 characters and at least one lower and one uppercase password.
type Validator interface {
Expand Down Expand Up @@ -49,10 +51,9 @@ var ErrUnexpectedStatusCode = errors.New("unexpected status code")
// [haveibeenpwnd](https://haveibeenpwned.com/API/v2#SearchingPwnedPasswordsByRange) service to check if the
// password has been breached in a previous data leak using k-anonymity.
type DefaultPasswordValidator struct {
sync.RWMutex
reg validatorDependencies
Client *retryablehttp.Client
hashes map[string]int64
hashes *ristretto.Cache

minIdentifierPasswordDist int
maxIdentifierPasswordSubstrThreshold float32
Expand All @@ -62,12 +63,22 @@ type validatorDependencies interface {
config.Provider
}

func NewDefaultPasswordValidatorStrategy(reg validatorDependencies) *DefaultPasswordValidator {
func NewDefaultPasswordValidatorStrategy(reg validatorDependencies) (*DefaultPasswordValidator, error) {
cache, err := ristretto.NewCache(&ristretto.Config{
NumCounters: 10 * 10000,
MaxCost: 60 * 10000, // BCrypt hash size is 60 bytes
BufferItems: 64,
IgnoreInternalCost: true,
})
// sanity check - this should never happen unless above configuration variables are invalid
if err != nil {
return nil, errors.Wrap(err, "error while setting up validator cache")
}
return &DefaultPasswordValidator{
Client: httpx.NewResilientClient(httpx.ResilientClientWithConnectionTimeout(time.Second)),
reg: reg,
hashes: map[string]int64{},
minIdentifierPasswordDist: 5, maxIdentifierPasswordSubstrThreshold: 0.5}
hashes: cache,
minIdentifierPasswordDist: 5, maxIdentifierPasswordSubstrThreshold: 0.5}, nil
}

func b20(src []byte) string {
Expand Down Expand Up @@ -109,9 +120,7 @@ func (s *DefaultPasswordValidator) fetch(hpw []byte, apiDNSName string) error {
return errors.Wrapf(ErrUnexpectedStatusCode, "%d", res.StatusCode)
}

s.Lock()
s.hashes[b20(hpw)] = 0
s.Unlock()
s.hashes.SetWithTTL(b20(hpw), 0, 1, hashCacheItemTTL)

sc := bufio.NewScanner(res.Body)
for sc.Scan() {
Expand All @@ -130,9 +139,7 @@ func (s *DefaultPasswordValidator) fetch(hpw []byte, apiDNSName string) error {
}
}

s.Lock()
s.hashes[(prefix + result[0])] = count
s.Unlock()
s.hashes.SetWithTTL(prefix+result[0], count, 1, hashCacheItemTTL)
}

if err := sc.Err(); err != nil {
Expand Down Expand Up @@ -169,10 +176,7 @@ func (s *DefaultPasswordValidator) Validate(ctx context.Context, identifier, pas
}
hpw := h.Sum(nil)

s.RLock()
c, ok := s.hashes[b20(hpw)]
s.RUnlock()

c, ok := s.hashes.Get(b20(hpw))
if !ok {
err := s.fetch(hpw, passwordPolicyConfig.HaveIBeenPwnedHost)
if (errors.Is(err, ErrNetworkFailure) || errors.Is(err, ErrUnexpectedStatusCode)) && passwordPolicyConfig.IgnoreNetworkErrors {
Expand All @@ -184,7 +188,8 @@ func (s *DefaultPasswordValidator) Validate(ctx context.Context, identifier, pas
return s.Validate(ctx, identifier, password)
}

if c > int64(s.reg.Config(ctx).PasswordPolicyConfig().MaxBreaches) {
v, ok := c.(int64)
if ok && v > int64(s.reg.Config(ctx).PasswordPolicyConfig().MaxBreaches) {
return errors.New("the password has been found in data breaches and must no longer be used")
}

Expand Down
14 changes: 7 additions & 7 deletions selfservice/strategy/password/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestDefaultPasswordValidationStrategy(t *testing.T) {

t.Run("default strategy", func(t *testing.T) {
_, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
for k, tc := range []struct {
id string
pw string
Expand Down Expand Up @@ -80,7 +80,7 @@ func TestDefaultPasswordValidationStrategy(t *testing.T) {

t.Run("failure cases", func(t *testing.T) {
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
fakeClient := NewFakeHTTPClient()
s.Client = httpx.NewResilientClient(httpx.ResilientClientWithClient(&fakeClient.Client), httpx.ResilientClientWithMaxRetry(1), httpx.ResilientClientWithConnectionTimeout(time.Millisecond), httpx.ResilientClientWithMaxRetryWait(time.Millisecond))

Expand Down Expand Up @@ -117,7 +117,7 @@ func TestDefaultPasswordValidationStrategy(t *testing.T) {

t.Run("max breaches", func(t *testing.T) {
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
fakeClient := NewFakeHTTPClient()
s.Client = httpx.NewResilientClient(httpx.ResilientClientWithClient(&fakeClient.Client), httpx.ResilientClientWithMaxRetry(1), httpx.ResilientClientWithConnectionTimeout(time.Millisecond))

Expand Down Expand Up @@ -194,7 +194,7 @@ func TestChangeHaveIBeenPwnedValidationHost(t *testing.T) {
testServer.StartTLS()
testServerURL, _ := url.Parse(testServer.URL)
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
conf.MustSet(config.ViperKeyPasswordHaveIBeenPwnedHost, testServerURL.Host)

fakeClient := NewFakeHTTPClient()
Expand All @@ -211,7 +211,7 @@ func TestChangeHaveIBeenPwnedValidationHost(t *testing.T) {

func TestDisableHaveIBeenPwnedValidationHost(t *testing.T) {
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
conf.MustSet(config.ViperKeyPasswordHaveIBeenPwnedEnabled, false)

fakeClient := NewFakeHTTPClient()
Expand All @@ -225,7 +225,7 @@ func TestDisableHaveIBeenPwnedValidationHost(t *testing.T) {

func TestChangeMinPasswordLength(t *testing.T) {
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)
conf.MustSet(config.ViperKeyPasswordMinLength, 10)

t.Run("case=should not fail if password is longer than min length", func(t *testing.T) {
Expand All @@ -239,7 +239,7 @@ func TestChangeMinPasswordLength(t *testing.T) {

func TestChangeIdentifierSimilarityCheckEnabled(t *testing.T) {
conf, reg := internal.NewFastRegistryWithMocks(t)
s := password.NewDefaultPasswordValidatorStrategy(reg)
s, _ := password.NewDefaultPasswordValidatorStrategy(reg)

t.Run("case=should not fail if password is similar to identifier", func(t *testing.T) {
conf.MustSet(config.ViperKeyPasswordIdentifierSimilarityCheckEnabled, false)
Expand Down