jwt: Cache the bcrypt password hash. (#3526)

Creds don't require secretKeyHash to be calculated
everytime, cache it instead and re-use.

This is an optimization for bcrypt.

Relevant results from the benchmark done locally, negative
value means improvement in this scenario.

```
benchmark                       old ns/op     new ns/op     delta
BenchmarkAuthenticateNode-4     160590992     80125647      -50.11%
BenchmarkAuthenticateWeb-4      160556692     80432144      -49.90%

benchmark                       old allocs     new allocs     delta
BenchmarkAuthenticateNode-4     87             75             -13.79%
BenchmarkAuthenticateWeb-4      87             75             -13.79%

benchmark                       old bytes     new bytes     delta
BenchmarkAuthenticateNode-4     15222         9785          -35.72%
BenchmarkAuthenticateWeb-4      15222         9785          -35.72%
```

appveyor.yml

@@ -35,8 +35,8 @@ test_script:
   # Unit tests
   - ps: Add-AppveyorTest "Unit Tests" -Outcome Running
   - mkdir build\coverage
-  - go test -timeout 15m -v -race github.com/minio/minio/cmd...
-  - go test -v -race github.com/minio/minio/pkg...
+  - go test -timeout 15m -race github.com/minio/minio/cmd...
+  - go test -race github.com/minio/minio/pkg...
   - go test -coverprofile=build\coverage\coverage.txt -covermode=atomic github.com/minio/minio/cmd
   - ps: Update-AppveyorTest "Unit Tests" -Outcome Passed
 

cmd/admin-handlers.go

@@ -131,8 +131,8 @@ func (adminAPI adminAPIHandlers) ServiceCredentialsHandler(w http.ResponseWriter
 	}
 
 	// Avoid setting new credentials when they are already passed
-	// by the environnement
-	if globalEnvAccessKey != "" || globalEnvSecretKey != "" {
+	// by the environment.
+	if globalIsEnvCreds {
 		writeErrorResponse(w, ErrMethodNotAllowed, r.URL)
 		return
 	}

cmd/admin-handlers_test.go

@@ -294,11 +294,9 @@ func TestServiceSetCreds(t *testing.T) {
 	for i, testCase := range testCases {
 		// Set or unset environement keys
 		if !testCase.EnvKeysSet {
-			globalEnvAccessKey = ""
-			globalEnvSecretKey = ""
+			globalIsEnvCreds = false
 		} else {
-			globalEnvAccessKey = testCase.Username
-			globalEnvSecretKey = testCase.Password
+			globalIsEnvCreds = true
 		}
 
 		// Construct setCreds request body

cmd/auth-handler_test.go

@@ -315,7 +315,12 @@ func TestIsReqAuthenticated(t *testing.T) {
 	}
 	defer removeAll(path)
 
-	serverConfig.SetCredential(credential{"myuser", "mypassword"})
+	creds, err := getCredential("myuser", "mypassword")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	serverConfig.SetCredential(creds)
 
 	// List of test cases for validating http request authentication.
 	testCases := []struct {

cmd/browser-peer-rpc.go

@@ -63,8 +63,13 @@ func (br *browserPeerAPIHandlers) SetAuthPeer(args SetAuthPeerArgs, reply *AuthR
 		return err
 	}
 
+	creds, err := getCredential(args.Creds.AccessKey, args.Creds.SecretKey)
+	if err != nil {
+		return err
+	}
+
 	// Update credentials in memory
-	serverConfig.SetCredential(args.Creds)
+	serverConfig.SetCredential(creds)
 
 	// Save credentials to config file
 	if err := serverConfig.Save(); err != nil {

cmd/config-v13.go

@@ -90,10 +90,12 @@ func initConfig() (bool, error) {
 		// Save config into file.
 		return true, serverConfig.Save()
 	}
+
 	configFile, err := getConfigFile()
 	if err != nil {
 		return false, err
 	}
+
 	if _, err = os.Stat(configFile); err != nil {
 		return false, err
 	}
@@ -103,7 +105,13 @@ func initConfig() (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	if err := qc.Load(configFile); err != nil {
+
+	if err = qc.Load(configFile); err != nil {
+		return false, err
+	}
+
+	srvCfg.Credential, err = getCredential(srvCfg.Credential.AccessKey, srvCfg.Credential.SecretKey)
+	if err != nil {
 		return false, err
 	}
 
@@ -112,6 +120,7 @@ func initConfig() (bool, error) {
 	// Save the loaded config globally.
 	serverConfig = srvCfg
 	serverConfigMu.Unlock()
+
 	// Set the version properly after the unmarshalled json is loaded.
 	serverConfig.Version = globalMinioConfigVersion
 
@@ -337,6 +346,7 @@ func (s *serverConfigV13) SetCredential(creds credential) {
 	serverConfigMu.Lock()
 	defer serverConfigMu.Unlock()
 
+	// Set updated credential.
 	s.Credential = creds
 }
 

cmd/credential.go

@@ -19,6 +19,8 @@ package cmd
 import (
 	"crypto/rand"
 	"encoding/base64"
+
+	"golang.org/x/crypto/bcrypt"
 )
 
 const (
@@ -65,14 +67,31 @@ func isSecretKeyValid(secretKey string) bool {
 
 // credential container for access and secret keys.
 type credential struct {
-	AccessKey string `json:"accessKey"`
-	SecretKey string `json:"secretKey"`
+	AccessKey     string `json:"accessKey,omitempty"`
+	SecretKey     string `json:"secretKey,omitempty"`
+	SecretKeyHash []byte `json:"secretKeyHash,omitempty"`
+}
+
+// Generate a bcrypt hashed key for input secret key.
+func mustGetHashedSecretKey(secretKey string) []byte {
+	hashedSecretKey, err := bcrypt.GenerateFromPassword([]byte(secretKey), bcrypt.DefaultCost)
+	if err != nil {
+		panic(err)
+	}
+	return hashedSecretKey
 }
 
+// Initialize a new credential object.
 func newCredential() credential {
-	return credential{mustGetAccessKey(), mustGetSecretKey()}
+	secretKey := mustGetSecretKey()
+	accessKey := mustGetAccessKey()
+
+	secretHash := mustGetHashedSecretKey(secretKey)
+	return credential{accessKey, secretKey, secretHash}
 }
 
+// Converts accessKey and secretKeys into credential object which
+// contains bcrypt secret key hash for future validation.
 func getCredential(accessKey, secretKey string) (credential, error) {
 	if !isAccessKeyValid(accessKey) {
 		return credential{}, errInvalidAccessKeyLength
@@ -82,5 +101,6 @@ func getCredential(accessKey, secretKey string) (credential, error) {
 		return credential{}, errInvalidSecretKeyLength
 	}
 
-	return credential{accessKey, secretKey}, nil
+	secretHash := mustGetHashedSecretKey(secretKey)
+	return credential{accessKey, secretKey, secretHash}, nil
 }

cmd/globals.go

@@ -110,11 +110,8 @@ var (
 	// Minio server user agent string.
 	globalServerUserAgent = "Minio/" + ReleaseTag + " (" + runtime.GOOS + "; " + runtime.GOARCH + ")"
 
-	// Global server's access key
-	globalEnvAccessKey = ""
-
-	// Global server's secret key
-	globalEnvSecretKey = ""
+	// Set to true if credentials were passed from env, default is false.
+	globalIsEnvCreds = false
 
 	// url.URL endpoints of disks that belong to the object storage.
 	globalEndpoints = []*url.URL{}

cmd/jwt.go

@@ -65,8 +65,7 @@ func authenticateJWT(accessKey, secretKey string, expiry time.Duration) (string,
 
 	// Validate secret key.
 	// Using bcrypt to avoid timing attacks.
-	hashedSecretKey, _ := bcrypt.GenerateFromPassword([]byte(serverCred.SecretKey), bcrypt.DefaultCost)
-	if bcrypt.CompareHashAndPassword(hashedSecretKey, []byte(secretKey)) != nil {
+	if bcrypt.CompareHashAndPassword(serverCred.SecretKeyHash, []byte(secretKey)) != nil {
 		return "", errAuthentication
 	}
 

cmd/jwt_test.go

@@ -75,10 +75,40 @@ func testAuthenticate(authType string, t *testing.T) {
 	}
 }
 
-func TestNodeAuthenticate(t *testing.T) {
+func TestAuthenticateNode(t *testing.T) {
 	testAuthenticate("node", t)
 }
 
-func TestWebAuthenticate(t *testing.T) {
+func TestAuthenticateWeb(t *testing.T) {
 	testAuthenticate("web", t)
 }
+
+func BenchmarkAuthenticateNode(b *testing.B) {
+	testPath, err := newTestConfig(globalMinioDefaultRegion)
+	if err != nil {
+		b.Fatalf("unable initialize config file, %s", err)
+	}
+	defer removeAll(testPath)
+
+	creds := serverConfig.GetCredential()
+	b.ResetTimer()
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		authenticateNode(creds.AccessKey, creds.SecretKey)
+	}
+}
+
+func BenchmarkAuthenticateWeb(b *testing.B) {
+	testPath, err := newTestConfig(globalMinioDefaultRegion)
+	if err != nil {
+		b.Fatalf("unable initialize config file, %s", err)
+	}
+	defer removeAll(testPath)
+
+	creds := serverConfig.GetCredential()
+	b.ResetTimer()
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		authenticateWeb(creds.AccessKey, creds.SecretKey)
+	}
+}

cmd/main.go

@@ -190,20 +190,14 @@ func minioInit(ctx *cli.Context) {
 	enableLoggers()
 
 	// Fetch access keys from environment variables and update the config.
-	globalEnvAccessKey = os.Getenv("MINIO_ACCESS_KEY")
-	globalEnvSecretKey = os.Getenv("MINIO_SECRET_KEY")
-	if globalEnvAccessKey != "" && globalEnvSecretKey != "" {
+	accessKey := os.Getenv("MINIO_ACCESS_KEY")
+	secretKey := os.Getenv("MINIO_SECRET_KEY")
+	if accessKey != "" && secretKey != "" {
+		creds, err := getCredential(accessKey, secretKey)
+		fatalIf(err, "Credentials are invalid, please set proper credentials `minio server --help`")
+
 		// Set new credentials.
-		serverConfig.SetCredential(credential{
-			AccessKey: globalEnvAccessKey,
-			SecretKey: globalEnvSecretKey,
-		})
-	}
-	if !isAccessKeyValid(serverConfig.GetCredential().AccessKey) {
-		fatalIf(errInvalidArgument, "Invalid access key. Accept only a string starting with a alphabetic and containing from 5 to 20 characters.")
-	}
-	if !isSecretKeyValid(serverConfig.GetCredential().SecretKey) {
-		fatalIf(errInvalidArgument, "Invalid secret key. Accept only a string containing from 8 to 40 characters.")
+		serverConfig.SetCredential(creds)
 	}
 
 	// Init the error tracing module.

cmd/web-handlers.go

@@ -363,16 +363,18 @@ func (web *webAPIHandlers) SetAuth(r *http.Request, args *SetAuthArgs, reply *Se
 	}
 
 	// As we already validated the authentication, we save given access/secret keys.
-	cred, err := getCredential(args.AccessKey, args.SecretKey)
+	creds, err := getCredential(args.AccessKey, args.SecretKey)
 	if err != nil {
 		return toJSONError(err)
 	}
 
 	// Notify all other Minio peers to update credentials
-	errsMap := updateCredsOnPeers(cred)
+	errsMap := updateCredsOnPeers(creds)
 
 	// Update local credentials
-	serverConfig.SetCredential(cred)
+	serverConfig.SetCredential(creds)
+
+	// Persist updated credentials.
 	if err = serverConfig.Save(); err != nil {
 		errsMap[globalMinioAddr] = err
 	}
@@ -506,14 +508,7 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {
 	objectLock.RLock()
 	defer objectLock.RUnlock()
 
-	objInfo, err := objectAPI.GetObjectInfo(bucket, object)
-	if err != nil {
-		writeWebErrorResponse(w, err)
-		return
-	}
-	offset := int64(0)
-	err = objectAPI.GetObject(bucket, object, offset, objInfo.Size, w)
-	if err != nil {
+	if err := objectAPI.GetObject(bucket, object, 0, -1, w); err != nil {
 		/// No need to print error, response writer already written to.
 		return
 	}

cmd/web-handlers_test.go

@@ -1533,10 +1533,6 @@ func TestWebObjectLayerFaultyDisks(t *testing.T) {
 	if rec.Code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code)
 	}
-	resp := string(rec.Body.Bytes())
-	if !strings.Contains(resp, "We encountered an internal error, please try again.") {
-		t.Fatalf("Unexpected error message, expected: `Invalid token`, found: `%s`", resp)
-	}
 
 	// Test authorization of Web.Upload
 	content := []byte("temporary file's content")
@@ -1553,8 +1549,4 @@ func TestWebObjectLayerFaultyDisks(t *testing.T) {
 	if rec.Code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code)
 	}
-	resp = string(rec.Body.Bytes())
-	if !strings.Contains(resp, "We encountered an internal error, please try again.") {
-		t.Fatalf("Unexpected error message, expected: `Invalid token`, found: `%s`", resp)
-	}
 }