Simplify credential usage. (#3893)

cmd/admin-handlers.go

@@ -165,18 +165,12 @@ func (adminAPI adminAPIHandlers) ServiceCredentialsHandler(w http.ResponseWriter
 		return
 	}
 
-	// Check passed credentials
-	err = validateAuthKeys(req.Username, req.Password)
+	creds, err := createCredential(req.Username, req.Password)
 	if err != nil {
 		writeErrorResponse(w, toAPIErrorCode(err), r.URL)
 		return
 	}
 
-	creds := credential{
-		AccessKey: req.Username,
-		SecretKey: req.Password,
-	}
-
 	// Notify all other Minio peers to update credentials
 	updateErrs := updateCredsOnPeers(creds)
 	for peer, err := range updateErrs {

cmd/auth-handler_test.go

@@ -316,7 +316,11 @@ func TestIsReqAuthenticated(t *testing.T) {
 	}
 	defer removeAll(path)
 
-	creds := newCredentialWithKeys("myuser", "mypassword")
+	creds, err := createCredential("myuser", "mypassword")
+	if err != nil {
+		t.Fatalf("unable create credential, %s", err)
+	}
+
 	serverConfig.SetCredential(creds)
 
 	// List of test cases for validating http request authentication.

cmd/browser-peer-rpc.go

@@ -1,5 +1,5 @@
 /*
- * Minio Cloud Storage, (C) 2016 Minio, Inc.
+ * Minio Cloud Storage, (C) 2016, 2017 Minio, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@
 package cmd
 
 import (
+	"fmt"
 	"path"
 	"sync"
 	"time"
@@ -63,8 +64,8 @@ func (br *browserPeerAPIHandlers) SetAuthPeer(args SetAuthPeerArgs, reply *AuthR
 		return err
 	}
 
-	if err := validateAuthKeys(args.Creds.AccessKey, args.Creds.SecretKey); err != nil {
-		return err
+	if !args.Creds.IsValid() {
+		return fmt.Errorf("Invalid credential passed")
 	}
 
 	// Update credentials in memory

cmd/browser-peer-rpc_test.go

@@ -63,9 +63,9 @@ func TestBrowserPeerRPC(t *testing.T) {
 // Tests for browser peer rpc.
 func (s *TestRPCBrowserPeerSuite) testBrowserPeerRPC(t *testing.T) {
 	// Construct RPC call arguments.
-	creds := credential{
-		AccessKey: "abcd1",
-		SecretKey: "abcd1234",
+	creds, err := createCredential("abcd1", "abcd1234")
+	if err != nil {
+		t.Fatalf("unable to create credential. %v", err)
 	}
 
 	// Validate for invalid token.
@@ -73,7 +73,7 @@ func (s *TestRPCBrowserPeerSuite) testBrowserPeerRPC(t *testing.T) {
 	args.AuthToken = "garbage"
 	rclient := newRPCClient(s.testAuthConf.serverAddr, s.testAuthConf.serviceEndpoint, false)
 	defer rclient.Close()
-	err := rclient.Call("BrowserPeer.SetAuthPeer", &args, &AuthRPCReply{})
+	err = rclient.Call("BrowserPeer.SetAuthPeer", &args, &AuthRPCReply{})
 	if err != nil {
 		if err.Error() != errInvalidToken.Error() {
 			t.Fatal(err)

cmd/config-migrate.go

@@ -116,13 +116,16 @@ func migrateV2ToV3() error {
 	if cv2.Version != "2" {
 		return nil
 	}
+
+	cred, err := createCredential(cv2.Credentials.AccessKey, cv2.Credentials.SecretKey)
+	if err != nil {
+		return fmt.Errorf("Invalid credential in V2 configuration file. %v", err)
+	}
+
 	srvConfig := &configV3{}
 	srvConfig.Version = "3"
 	srvConfig.Addr = ":9000"
-	srvConfig.Credential = credential{
-		AccessKey: cv2.Credentials.AccessKey,
-		SecretKey: cv2.Credentials.SecretKey,
-	}
+	srvConfig.Credential = cred
 	srvConfig.Region = cv2.Credentials.Region
 	if srvConfig.Region == "" {
 		// Region needs to be set for AWS Signature V4.

cmd/config-v14.go

@@ -57,7 +57,7 @@ func newServerConfigV14() *serverConfigV14 {
 		Logger:  &logger{},
 		Notify:  &notifier{},
 	}
-	srvCfg.SetCredential(newCredential())
+	srvCfg.SetCredential(mustGetNewCredential())
 	srvCfg.SetBrowser("on")
 	// Enable console logger by default on a fresh run.
 	srvCfg.Logger.Console = consoleLogger{
@@ -253,8 +253,8 @@ func validateConfig() error {
 	}
 
 	// Validate credential field
-	if err := srvCfg.Credential.Validate(); err != nil {
-		return err
+	if !srvCfg.Credential.IsValid() {
+		return errors.New("invalid credential")
 	}
 
 	// Validate logger field
@@ -303,7 +303,7 @@ func (s *serverConfigV14) SetCredential(creds credential) {
 	defer serverConfigMu.Unlock()
 
 	// Set updated credential.
-	s.Credential = newCredentialWithKeys(creds.AccessKey, creds.SecretKey)
+	s.Credential = creds
 }
 
 // GetCredentials get current credentials.

cmd/credential.go

@@ -1,5 +1,5 @@
 /*
- * Minio Cloud Storage, (C) 2015, 2016 Minio, Inc.
+ * Minio Cloud Storage, (C) 2015, 2016, 2017 Minio, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,7 +20,6 @@ import (
 	"crypto/rand"
 	"encoding/base64"
 	"errors"
-	"os"
 
 	"github.com/minio/mc/pkg/console"
 
@@ -37,27 +36,10 @@ const (
 	alphaNumericTableLen = byte(len(alphaNumericTable))
 )
 
-func mustGetAccessKey() string {
-	keyBytes := make([]byte, accessKeyMaxLen)
-	if _, err := rand.Read(keyBytes); err != nil {
-		console.Fatalf("Unable to generate access key. Err: %s.\n", err)
-	}
-
-	for i := 0; i < accessKeyMaxLen; i++ {
-		keyBytes[i] = alphaNumericTable[keyBytes[i]%alphaNumericTableLen]
-	}
-
-	return string(keyBytes)
-}
-
-func mustGetSecretKey() string {
-	keyBytes := make([]byte, secretKeyMaxLen)
-	if _, err := rand.Read(keyBytes); err != nil {
-		console.Fatalf("Unable to generate secret key. Err: %s.\n", err)
-	}
-
-	return string([]byte(base64.StdEncoding.EncodeToString(keyBytes))[:secretKeyMaxLen])
-}
+var (
+	errInvalidAccessKeyLength = errors.New("Invalid access key, access key should be 5 to 20 characters in length")
+	errInvalidSecretKeyLength = errors.New("Invalid secret key, secret key should be 8 to 40 characters in length")
+)
 
 // isAccessKeyValid - validate access key for right length.
 func isAccessKeyValid(accessKey string) bool {
@@ -76,75 +58,72 @@ type credential struct {
 	secretKeyHash []byte
 }
 
-func (c *credential) Validate() error {
-	if !isAccessKeyValid(c.AccessKey) {
-		return errors.New("Invalid access key")
-	}
-	if !isSecretKeyValid(c.SecretKey) {
-		return errors.New("Invalid secret key")
-	}
-	return nil
+// IsValid - returns whether credential is valid or not.
+func (cred credential) IsValid() bool {
+	return isAccessKeyValid(cred.AccessKey) && isSecretKeyValid(cred.SecretKey)
 }
 
-// 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 {
-		console.Fatalf("Unable to generate secret hash for secret key. Err: %s.\n", err)
+// Equals - returns whether two credentials are equal or not.
+func (cred credential) Equal(ccred credential) bool {
+	if !ccred.IsValid() {
+		return false
 	}
-	return hashedSecretKey
-}
 
-// Initialize a new credential object
-func newCredential() credential {
-	return newCredentialWithKeys(mustGetAccessKey(), mustGetSecretKey())
-}
+	if cred.secretKeyHash == nil {
+		secretKeyHash, err := bcrypt.GenerateFromPassword([]byte(cred.SecretKey), bcrypt.DefaultCost)
+		if err != nil {
+			errorIf(err, "Unable to generate hash of given password")
+			return false
+		}
+
+		cred.secretKeyHash = secretKeyHash
+	}
 
-func newCredentialWithKeys(accessKey, secretKey string) credential {
-	secretHash := mustGetHashedSecretKey(secretKey)
-	return credential{accessKey, secretKey, secretHash}
+	return (cred.AccessKey == ccred.AccessKey &&
+		bcrypt.CompareHashAndPassword(cred.secretKeyHash, []byte(ccred.SecretKey)) == nil)
 }
 
-// Validate incoming auth keys.
-func validateAuthKeys(accessKey, secretKey string) error {
-	// Validate the env values before proceeding.
+func createCredential(accessKey, secretKey string) (cred credential, err error) {
 	if !isAccessKeyValid(accessKey) {
-		return errInvalidAccessKeyLength
-	}
-	if !isSecretKeyValid(secretKey) {
-		return errInvalidSecretKeyLength
+		err = errInvalidAccessKeyLength
+	} else if !isSecretKeyValid(secretKey) {
+		err = errInvalidSecretKeyLength
+	} else {
+		var secretKeyHash []byte
+		secretKeyHash, err = bcrypt.GenerateFromPassword([]byte(secretKey), bcrypt.DefaultCost)
+		if err == nil {
+			cred.AccessKey = accessKey
+			cred.SecretKey = secretKey
+			cred.secretKeyHash = secretKeyHash
+		}
 	}
-	return nil
-}
 
-// Variant of getCredentialFromEnv but upon error fails right here.
-func mustGetCredentialFromEnv() credential {
-	creds, err := getCredentialFromEnv()
-	if err != nil {
-		console.Fatalf("Unable to load credentials from environment. Err: %s.\n", err)
-	}
-	return creds
+	return cred, err
 }
 
-// Converts accessKey and secretKeys into credential object which
-// contains bcrypt secret key hash for future validation.
-func getCredentialFromEnv() (credential, error) {
-	// Fetch access keys from environment variables and update the config.
-	accessKey := os.Getenv("MINIO_ACCESS_KEY")
-	secretKey := os.Getenv("MINIO_SECRET_KEY")
-
-	// Envs are set globally.
-	globalIsEnvCreds = accessKey != "" && secretKey != ""
+// Initialize a new credential object
+func mustGetNewCredential() credential {
+	// Generate access key.
+	keyBytes := make([]byte, accessKeyMaxLen)
+	if _, err := rand.Read(keyBytes); err != nil {
+		console.Fatalln("Unable to generate access key.", err)
+	}
+	for i := 0; i < accessKeyMaxLen; i++ {
+		keyBytes[i] = alphaNumericTable[keyBytes[i]%alphaNumericTableLen]
+	}
+	accessKey := string(keyBytes)
 
-	if globalIsEnvCreds {
-		// Validate the env values before proceeding.
-		if err := validateAuthKeys(accessKey, secretKey); err != nil {
-			return credential{}, err
-		}
+	// Generate secret key.
+	keyBytes = make([]byte, secretKeyMaxLen)
+	if _, err := rand.Read(keyBytes); err != nil {
+		console.Fatalln("Unable to generate secret key.", err)
+	}
+	secretKey := string([]byte(base64.StdEncoding.EncodeToString(keyBytes))[:secretKeyMaxLen])
 
-		// Return credential object.
-		return newCredentialWithKeys(accessKey, secretKey), nil
+	cred, err := createCredential(accessKey, secretKey)
+	if err != nil {
+		console.Fatalln("Unable to generate new credential.", err)
 	}
 
-	return credential{}, nil
+	return cred
 }

cmd/credential_test.go

@@ -0,0 +1,101 @@
+/*
+ * Minio Cloud Storage, (C) 2017 Minio, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package cmd
+
+import "testing"
+
+func TestMustGetNewCredential(t *testing.T) {
+	cred := mustGetNewCredential()
+	if !cred.IsValid() {
+		t.Fatalf("Failed to get new valid credential")
+	}
+}
+
+func TestCreateCredential(t *testing.T) {
+	cred := mustGetNewCredential()
+	testCases := []struct {
+		accessKey      string
+		secretKey      string
+		expectedResult bool
+		expectedErr    error
+	}{
+		// Access key too small.
+		{"user", "pass", false, errInvalidAccessKeyLength},
+		// Access key too long.
+		{"user12345678901234567", "pass", false, errInvalidAccessKeyLength},
+		// Access key contains unsuppported characters.
+		{"!@#$", "pass", false, errInvalidAccessKeyLength},
+		// Secret key too small.
+		{"myuser", "pass", false, errInvalidSecretKeyLength},
+		// Secret key too long.
+		{"myuser", "pass1234567890123456789012345678901234567", false, errInvalidSecretKeyLength},
+		// Success when access key contains leading/trailing spaces.
+		{" user ", cred.SecretKey, true, nil},
+		{"myuser", "mypassword", true, nil},
+		{cred.AccessKey, cred.SecretKey, true, nil},
+	}
+
+	for _, testCase := range testCases {
+		cred, err := createCredential(testCase.accessKey, testCase.secretKey)
+		if testCase.expectedErr == nil {
+			if err != nil {
+				t.Fatalf("error: expected = <nil>, got = %v", err)
+			}
+		} else if err == nil {
+			t.Fatalf("error: expected = %v, got = <nil>", testCase.expectedErr)
+		} else if testCase.expectedErr.Error() != err.Error() {
+			t.Fatalf("error: expected = %v, got = %v", testCase.expectedErr, err)
+		}
+
+		if testCase.expectedResult != cred.IsValid() {
+			t.Fatalf("cred: expected: %v, got: %v", testCase.expectedResult, cred.IsValid())
+		}
+	}
+}
+
+func TestCredentialEqual(t *testing.T) {
+	cred := mustGetNewCredential()
+	testCases := []struct {
+		cred           credential
+		ccred          credential
+		expectedResult bool
+	}{
+		// Empty compare credential
+		{cred, credential{}, false},
+		// Empty credential
+		{credential{}, cred, false},
+		// Two different credentials
+		{cred, mustGetNewCredential(), false},
+		// Access key is different in compare credential.
+		{cred, credential{AccessKey: "myuser", SecretKey: cred.SecretKey}, false},
+		// Secret key is different in compare credential.
+		{cred, credential{AccessKey: cred.AccessKey, SecretKey: "mypassword"}, false},
+		// secretHashKey is missing in compare credential.
+		{cred, credential{AccessKey: cred.AccessKey, SecretKey: cred.SecretKey}, true},
+		// secretHashKey is missing in credential.
+		{credential{AccessKey: cred.AccessKey, SecretKey: cred.SecretKey}, cred, true},
+		// Same credentials.
+		{cred, cred, true},
+	}
+
+	for _, testCase := range testCases {
+		result := testCase.cred.Equal(testCase.ccred)
+		if result != testCase.expectedResult {
+			t.Fatalf("cred: expected: %v, got: %v", testCase.expectedResult, result)
+		}
+	}
+}

cmd/jwt.go

@@ -1,5 +1,5 @@
 /*
- * Minio Cloud Storage, (C) 2016 Minio, Inc.
+ * Minio Cloud Storage, (C) 2016, 2017 Minio, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,12 +20,10 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"strings"
 	"time"
 
 	jwtgo "github.com/dgrijalva/jwt-go"
 	jwtreq "github.com/dgrijalva/jwt-go/request"
-	"golang.org/x/crypto/bcrypt"
 )
 
 const (
@@ -39,9 +37,6 @@ const (
 )
 
 var (
-	errInvalidAccessKeyLength = errors.New("Invalid access key, access key should be 5 to 20 characters in length")
-	errInvalidSecretKeyLength = errors.New("Invalid secret key, secret key should be 8 to 40 characters in length")
-
 	errInvalidAccessKeyID   = errors.New("The access key ID you provided does not exist in our records")
 	errChangeCredNotAllowed = errors.New("Changing access key and secret key not allowed")
 	errAuthentication       = errors.New("Authentication failed, check your access credentials")
@@ -49,34 +44,19 @@ var (
 )
 
 func authenticateJWT(accessKey, secretKey string, expiry time.Duration) (string, error) {
-	// Trim spaces.
-	accessKey = strings.TrimSpace(accessKey)
-
-	if !isAccessKeyValid(accessKey) {
-		return "", errInvalidAccessKeyLength
-	}
-	if !isSecretKeyValid(secretKey) {
-		return "", errInvalidSecretKeyLength
+	passedCredential, err := createCredential(accessKey, secretKey)
+	if err != nil {
+		return "", err
 	}
 
 	serverCred := serverConfig.GetCredential()
 
-	// Validate access key.
-	if accessKey != serverCred.AccessKey {
+	if serverCred.AccessKey != passedCredential.AccessKey {
 		return "", errInvalidAccessKeyID
 	}
 
-	// Validate secret key.
-	// Using bcrypt to avoid timing attacks.
-	if serverCred.secretKeyHash != nil {
-		if bcrypt.CompareHashAndPassword(serverCred.secretKeyHash, []byte(secretKey)) != nil {
-			return "", errAuthentication
-		}
-	} else {
-		// Secret key hash not set then generate and validate.
-		if bcrypt.CompareHashAndPassword(mustGetHashedSecretKey(serverCred.SecretKey), []byte(secretKey)) != nil {
-			return "", errAuthentication
-		}
+	if !serverCred.Equal(passedCredential) {
+		return "", errAuthentication
 	}
 
 	utcNow := time.Now().UTC()

cmd/jwt_test.go

@@ -39,6 +39,8 @@ func testAuthenticate(authType string, t *testing.T) {
 		{"user12345678901234567", "pass", errInvalidAccessKeyLength},
 		// Access key contains unsuppported characters.
 		{"!@#$", "pass", errInvalidAccessKeyLength},
+		// Success when access key contains leading/trailing spaces.
+		{" " + serverCred.AccessKey + " ", serverCred.SecretKey, errInvalidAccessKeyLength},
 		// Secret key too small.
 		{"myuser", "pass", errInvalidSecretKeyLength},
 		// Secret key too long.
@@ -49,8 +51,6 @@ func testAuthenticate(authType string, t *testing.T) {
 		{serverCred.AccessKey, "mypassword", errAuthentication},
 		// Success.
 		{serverCred.AccessKey, serverCred.SecretKey, nil},
-		// Success when access key contains leading/trailing spaces.
-		{" " + serverCred.AccessKey + " ", serverCred.SecretKey, nil},
 	}
 
 	// Run tests.

cmd/server-main.go

@@ -118,10 +118,32 @@ func enableLoggers() {
 // Initializes a new config if it doesn't exist, else migrates any old config
 // to newer config and finally loads the config to memory.
 func initConfig() {
+	accessKey := os.Getenv("MINIO_ACCESS_KEY")
+	secretKey := os.Getenv("MINIO_SECRET_KEY")
+
+	var cred credential
+	var err error
+	if accessKey != "" && secretKey != "" {
+		if cred, err = createCredential(accessKey, secretKey); err != nil {
+			console.Fatalf("Invalid access/secret Key set in environment. Err: %s.\n", err)
+		}
+
+		// Envs are set globally.
+		globalIsEnvCreds = true
+	}
+
+	browser := os.Getenv("MINIO_BROWSER")
+	if browser != "" {
+		if !(strings.EqualFold(browser, "off") || strings.EqualFold(browser, "on")) {
+			console.Fatalf("Invalid value ‘%s’ in MINIO_BROWSER environment variable.", browser)
+		}
+
+		globalIsEnvBrowser = strings.EqualFold(browser, "off")
+	}
 
 	envs := envParams{
-		creds:   mustGetCredentialFromEnv(),
-		browser: mustGetBrowserFromEnv(),
+		creds:   cred,
+		browser: browser,
 	}
 
 	// Config file does not exist, we create it fresh and return upon success.

cmd/server_test.go

@@ -94,7 +94,7 @@ func (s *TestSuiteCommon) TearDownSuite(c *C) {
 }
 
 func (s *TestSuiteCommon) TestAuth(c *C) {
-	cred := newCredential()
+	cred := mustGetNewCredential()
 
 	c.Assert(len(cred.AccessKey), Equals, accessKeyMaxLen)
 	c.Assert(len(cred.SecretKey), Equals, secretKeyMaxLen)

cmd/utils.go

@@ -29,7 +29,6 @@ import (
 	"encoding/json"
 
 	humanize "github.com/dustin/go-humanize"
-	"github.com/minio/mc/pkg/console"
 	"github.com/pkg/profile"
 )
 
@@ -255,28 +254,6 @@ func dumpRequest(r *http.Request) string {
 	return string(jsonBytes)
 }
 
-// Variant of getBrowserFromEnv but upon error fails right here.
-func mustGetBrowserFromEnv() string {
-	browser, err := getBrowserFromEnv()
-	if err != nil {
-		console.Fatalf("Unable to load MINIO_BROWSER value from environment. Err: %s.\n", err)
-	}
-	return browser
-}
-
-//
-func getBrowserFromEnv() (string, error) {
-	b := os.Getenv("MINIO_BROWSER")
-	if strings.TrimSpace(b) == "" {
-		return "", nil
-	}
-	if !strings.EqualFold(b, "off") && !strings.EqualFold(b, "on") {
-		return "", errInvalidArgument
-	}
-	globalIsEnvBrowser = true
-	return strings.ToLower(b), nil
-}
-
 // isFile - returns whether given path is a file or not.
 func isFile(path string) bool {
 	if fi, err := os.Stat(path); err == nil {

cmd/web-handlers.go

@@ -373,7 +373,7 @@ func (web webAPIHandlers) GenerateAuth(r *http.Request, args *WebGenericArgs, re
 	if !isHTTPRequestValid(r) {
 		return toJSONError(errAuthentication)
 	}
-	cred := newCredential()
+	cred := mustGetNewCredential()
 	reply.AccessKey = cred.AccessKey
 	reply.SecretKey = cred.SecretKey
 	reply.UIVersion = browser.UIVersion
@@ -404,16 +404,11 @@ func (web *webAPIHandlers) SetAuth(r *http.Request, args *SetAuthArgs, reply *Se
 		return toJSONError(errChangeCredNotAllowed)
 	}
 
-	// As we already validated the authentication, we save given access/secret keys.
-	if err := validateAuthKeys(args.AccessKey, args.SecretKey); err != nil {
+	creds, err := createCredential(args.AccessKey, args.SecretKey)
+	if err != nil {
 		return toJSONError(err)
 	}
 
-	creds := credential{
-		AccessKey: args.AccessKey,
-		SecretKey: args.SecretKey,
-	}
-
 	// Notify all other Minio peers to update credentials
 	errsMap := updateCredsOnPeers(creds)
 
@@ -421,7 +416,7 @@ func (web *webAPIHandlers) SetAuth(r *http.Request, args *SetAuthArgs, reply *Se
 	serverConfig.SetCredential(creds)
 
 	// Persist updated credentials.
-	if err := serverConfig.Save(); err != nil {
+	if err = serverConfig.Save(); err != nil {
 		errsMap[globalMinioAddr] = err
 	}