Repository: coreos/dex
Pull request #1040 (2017-08-17 23:28:45)

server: set sane bcrypt cost upper bound
api.go: `checkCost` only returns an error if hash cost is too high or low (or if `Cost` fails)
server.go: remove timeout logic and reverte back to a hash comparison, with an addition cost check
server_test.go: `MinCost` will cause `Login` to error now, so use `DefaultCost` instead.

Instead of timing out after 10 seconds of hashing, dex should not permit a bcrypt hash cost > 16. The below benchmarks show that cost > 16 exponentially grows to times that a user would notice when logging in, considering that dex will likely be running on a server (much beefier than my machine).

Tests for login timeouts have been removed.

Applies to #1030. Middleware for generically timing out requests will be implemented soon.

Benchmarks (Intel(R) Core(TM) i7-6500U CPU @ 2.50GHz):

Cost | Time (sec)
:---: | :---:
10 | 0.073507
11 | 0.151875
12 | 0.301854
13 | 0.596907
14 | 1.151219
15 | 2.365981
**16** | **4.941790**
17 | 9.467757


Patch:
diff --git a/connector/github/github.go b/connector/github/github.go
index aff2f1b4..83281aec 100644
--- a/connector/github/github.go
+++ b/connector/github/github.go
@@ -501,20 +501,6 @@ func (c *githubConnector) userEmail(ctx context.Context, client *http.Client) (s
 		}
 
 		for _, email := range emails {
-			/*
-				if GitHub Enterprise, set email.Verified to true
-				This change being made because GitHub Enterprise does not
-				support email verification. CircleCI indicated that GitHub
-				advised them not to check for verified emails
-				(https://circleci.com/enterprise/changelog/#1-47-1).
-				In addition, GitHub Enterprise support replied to a support
-				ticket with "There is no way to verify an email address in 
-				GitHub Enterprise."			
-			*/
-			if c.hostName != "" {
-				email.Verified = true
-			}
-			
 			if email.Verified && email.Primary {
 				return email.Email, nil
 			}
diff --git a/server/api.go b/server/api.go
index 5e8fad76..15266994 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/server.go b/server/server.go
index 41c28853..aabe7abe 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,


Test patch:
diff --git a/server/api_test.go b/server/api_test.go
index e9e2ea7d..bea5d225 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_test.go b/server/server_test.go
index a77c92bf..88110087 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()