diff --git a/server/api.go b/server/api.go
index 5e8fad763055e03118db02156528bb488ff4b919..15266994373af4f25dbe15a329fed03d1438c19c 100644
--- a/server/api.go
+++ b/server/api.go
@@ -21,8 +21,16 @@ import (
// to determine if the server supports specific features.
const apiVersion = 2
-// recCost is the recommended bcrypt cost, which balances hash strength and time
-const recCost = 12
+const (
+ // recCost is the recommended bcrypt cost, which balances hash strength and
+ // efficiency.
+ recCost = 12
+
+ // upBoundCost is a sane upper bound on bcrypt cost determined by benchmarking:
+ // high enough to ensure secure encryption, low enough to not put unnecessary
+ // load on a dex server.
+ upBoundCost = 16
+)
// NewAPI returns a server which implements the gRPC API interface.
func NewAPI(s storage.Storage, logger logrus.FieldLogger) api.DexServer {
@@ -83,16 +91,20 @@ func (d dexAPI) DeleteClient(ctx context.Context, req *api.DeleteClientReq) (*ap
return &api.DeleteClientResp{}, nil
}
-// checkCost returns an error if the hash provided does not meet minimum cost requirement, and the actual bcrypt cost
-func checkCost(hash []byte) (int, error) {
+// checkCost returns an error if the hash provided does not meet lower or upper
+// bound cost requirements.
+func checkCost(hash []byte) error {
actual, err := bcrypt.Cost(hash)
if err != nil {
- return 0, fmt.Errorf("parsing bcrypt hash: %v", err)
+ return fmt.Errorf("parsing bcrypt hash: %v", err)
}
if actual < bcrypt.DefaultCost {
- return actual, fmt.Errorf("given hash cost = %d, does not meet minimum cost requirement = %d", actual, bcrypt.DefaultCost)
+ return fmt.Errorf("given hash cost = %d does not meet minimum cost requirement = %d", actual, bcrypt.DefaultCost)
+ }
+ if actual > upBoundCost {
+ return fmt.Errorf("given hash cost = %d is above upper bound cost = %d, recommended cost = %d", actual, upBoundCost, recCost)
}
- return actual, nil
+ return nil
}
func (d dexAPI) CreatePassword(ctx context.Context, req *api.CreatePasswordReq) (*api.CreatePasswordResp, error) {
@@ -103,13 +115,9 @@ func (d dexAPI) CreatePassword(ctx context.Context, req *api.CreatePasswordReq)
return nil, errors.New("no user ID supplied")
}
if req.Password.Hash != nil {
- cost, err := checkCost(req.Password.Hash)
- if err != nil {
+ if err := checkCost(req.Password.Hash); err != nil {
return nil, err
}
- if cost > recCost {
- d.logger.Warnln("bcrypt cost = %d, password encryption might timeout. Recommended bcrypt cost is 12", cost)
- }
} else {
return nil, errors.New("no hash of password supplied")
}
@@ -140,13 +148,9 @@ func (d dexAPI) UpdatePassword(ctx context.Context, req *api.UpdatePasswordReq)
}
if req.NewHash != nil {
- cost, err := checkCost(req.NewHash)
- if err != nil {
+ if err := checkCost(req.NewHash); err != nil {
return nil, err
}
- if cost > recCost {
- d.logger.Warnln("bcrypt cost = %d, password encryption might timeout. Recommended bcrypt cost is 12", cost)
- }
}
updater := func(old storage.Password) (storage.Password, error) {
diff --git a/server/api_test.go b/server/api_test.go
index e9e2ea7d003be4f2703835154b5264538bba2ab7..bea5d2258585aec307a0d74c85f1703c011aaa6a 100644
--- a/server/api_test.go
+++ b/server/api_test.go
@@ -132,16 +132,15 @@ func TestCheckCost(t *testing.T) {
defer client.Close()
tests := []struct {
- name string
- inputHash []byte
- expectedCost int
- wantErr bool
+ name string
+ inputHash []byte
+
+ wantErr bool
}{
{
name: "valid cost",
- // bcrypt hash of the value "test1" with cost 12
- inputHash: []byte("$2a$12$M2Ot95Qty1MuQdubh1acWOiYadJDzeVg3ve4n5b.dgcgPdjCseKx2"),
- expectedCost: recCost,
+ // bcrypt hash of the value "test1" with cost 12 (default)
+ inputHash: []byte("$2a$12$M2Ot95Qty1MuQdubh1acWOiYadJDzeVg3ve4n5b.dgcgPdjCseKx2"),
},
{
name: "invalid hash",
@@ -156,15 +155,14 @@ func TestCheckCost(t *testing.T) {
},
{
name: "cost above recommendation",
- // bcrypt hash of the value "test1" with cost 20
- inputHash: []byte("$2a$20$yODn5quqK9MZdePqYLs6Y.Jr4cOO1P0aXsKz0eTa2rxOmu8e7ETpi"),
- expectedCost: 20,
+ // bcrypt hash of the value "test1" with cost 17
+ inputHash: []byte("$2a$17$tWuZkTxtSmRyWZAGWVHQE.7npdl.TgP8adjzLJD.SyjpFznKBftPe"),
+ wantErr: true,
},
}
for _, tc := range tests {
- cost, err := checkCost(tc.inputHash)
- if err != nil {
+ if err := checkCost(tc.inputHash); err != nil {
if !tc.wantErr {
t.Errorf("%s: %s", tc.name, err)
}
@@ -175,10 +173,6 @@ func TestCheckCost(t *testing.T) {
t.Errorf("%s: expected err", tc.name)
continue
}
-
- if cost != tc.expectedCost {
- t.Errorf("%s: exepcted cost = %d but got cost = %d", tc.name, tc.expectedCost, cost)
- }
}
}
diff --git a/server/server.go b/server/server.go
index 41c2885375d46117f4051da82aec98945f14c8f8..aabe7abeac5266191957758529cb2b2b74fd57b4 100644
--- a/server/server.go
+++ b/server/server.go
@@ -288,25 +288,14 @@ func (db passwordDB) Login(ctx context.Context, s connector.Scopes, email, passw
}
return connector.Identity{}, false, nil
}
-
- // Return an error if password-hash comparison takes longer than 10 seconds
- errCh := make(chan error, 1)
- go func() {
- errCh <- bcrypt.CompareHashAndPassword(p.Hash, []byte(password))
- }()
- select {
- case err = <-errCh:
- if err != nil {
- return connector.Identity{}, false, nil
- }
- case <-time.After(time.Second * 10):
- var cost int
- if cost, err = bcrypt.Cost(p.Hash); err == nil {
- err = fmt.Errorf("password-hash comparison timeout: your bcrypt cost = %d, recommended cost = %d", cost, recCost)
- }
+ // This check prevents dex users from logging in using static passwords
+ // configured with hash costs that are too high or low.
+ if err := checkCost(p.Hash); err != nil {
return connector.Identity{}, false, err
}
-
+ if err := bcrypt.CompareHashAndPassword(p.Hash, []byte(password)); err != nil {
+ return connector.Identity{}, false, nil
+ }
return connector.Identity{
UserID: p.UserID,
Username: p.Username,
diff --git a/server/server_test.go b/server/server_test.go
index a77c92bfb2f1ab6d7583548669642e2aee0dc5f5..881100872aee041085febdf3a46bf01b262ef8f4 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -818,7 +818,7 @@ func TestPasswordDB(t *testing.T) {
pw := "hi"
- h, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.MinCost)
+ h, err := bcrypt.GenerateFromPassword([]byte(pw), bcrypt.DefaultCost)
if err != nil {
t.Fatal(err)
}
@@ -896,116 +896,6 @@ func TestPasswordDB(t *testing.T) {
}
-// A warning message should be logged if password-hash comparison takes longer than 10s
-func TestLoginTimeout(t *testing.T) {
- s := memory.New(logger)
- conn := newPasswordDB(s)
-
- pw := "test1"
-
- tests := []struct {
- name, email, password string
- pwHash []byte
- wantIdentity connector.Identity
- wantInvalid bool
- wantedErr string
- }{
- {
- name: "valid password min cost",
- email: "jane@example.com",
- password: pw,
- // bcrypt hash of the value "test1" with cost 4
- pwHash: []byte("$2a$04$lGqOe5gnlpsfreQ1OJHxGOO7f5FyyESyICkswSFATM1cnBVgCyyuG"),
- wantIdentity: connector.Identity{
- Email: "jane@example.com",
- Username: "jane",
- UserID: "foobar",
- EmailVerified: true,
- },
- },
- {
- name: "valid password reccomended cost",
- email: "jane@example.com",
- password: pw,
- // bcrypt hash of the value "test1" with cost 12
- pwHash: []byte("$2a$12$VZNNjuCUGX2NG5S1ci.3Ku9mI9DmA9XeXyrr7YzJuyTxuVBGdRAbm"),
- wantIdentity: connector.Identity{
- Email: "jane@example.com",
- Username: "jane",
- UserID: "foobar",
- EmailVerified: true,
- },
- },
- {
- name: "valid password timeout cost",
- email: "jane@example.com",
- password: pw,
- // bcrypt hash of the value "test1" with cost 20
- pwHash: []byte("$2a$20$yODn5quqK9MZdePqYLs6Y.Jr4cOO1P0aXsKz0eTa2rxOmu8e7ETpi"),
- wantedErr: fmt.Sprintf("password-hash comparison timeout: your bcrypt cost = 20, recommended cost = %d", recCost),
- },
- {
- name: "invalid password min cost",
- email: "jane@example.com",
- password: pw,
- // bcrypt hash of the value "test2" with cost 4
- pwHash: []byte("$2a$04$pX8wwwpxw8xlXrToYaEgZemK0JIibMZYXPsgau7aPDoGyHPF73br."),
- wantInvalid: true,
- },
- {
- name: "invalid password timeout cost",
- email: "jane@example.com",
- password: pw,
- // bcrypt hash of the value "test2" with cost 20
- pwHash: []byte("$2a$20$WBD9cs63Zf0zqS99yyrQhODoDXphWw8MlYqVYRiftJH.lRJ1stnAa"),
- wantedErr: fmt.Sprintf("password-hash comparison timeout: your bcrypt cost = 20, recommended cost = %d", recCost),
- },
- }
-
- sEmail, sUsername, sUserID := "jane@example.com", "jane", "foobar"
- for _, tc := range tests {
- // Clean up before new test case
- s.DeletePassword(sEmail)
-
- s.CreatePassword(storage.Password{
- Email: sEmail,
- Username: sUsername,
- UserID: sUserID,
- Hash: tc.pwHash,
- })
-
- ident, valid, err := conn.Login(context.Background(), connector.Scopes{}, tc.email, tc.password)
- if err != nil {
- if err.Error() != tc.wantedErr {
- t.Errorf("%s: error was incorrect:\n%v", tc.name, err)
- }
- continue
- }
-
- if tc.wantedErr != "" {
- t.Errorf("%s: expected error", tc.name)
- continue
- }
-
- if !valid {
- if !tc.wantInvalid {
- t.Errorf("%s: expected valid response", tc.name)
- }
- continue
- }
-
- if tc.wantInvalid {
- t.Errorf("%s: expected invalid response", tc.name)
- continue
- }
-
- if diff := pretty.Compare(tc.wantIdentity, ident); diff != "" {
- t.Errorf("%s: %s", tc.email, diff)
- }
- }
-
-}
-
type storageWithKeysTrigger struct {
storage.Storage
f func()