From 21d73a3eefc6ade7e65e761bcb0a789bda3ecb78 Mon Sep 17 00:00:00 2001 From: Bala FA Date: Thu, 16 Mar 2017 12:46:06 +0530 Subject: [PATCH] Simplify credential usage. (#3893) --- cmd/admin-handlers.go | 8 +-- cmd/auth-handler_test.go | 6 +- cmd/browser-peer-rpc.go | 7 +- cmd/browser-peer-rpc_test.go | 8 +-- cmd/config-migrate.go | 11 +-- cmd/config-v14.go | 8 +-- cmd/credential.go | 135 +++++++++++++++-------------------- cmd/credential_test.go | 101 ++++++++++++++++++++++++++ cmd/jwt.go | 34 ++------- cmd/jwt_test.go | 4 +- cmd/server-main.go | 26 ++++++- cmd/server_test.go | 2 +- cmd/utils.go | 23 ------ cmd/web-handlers.go | 13 ++-- 14 files changed, 221 insertions(+), 165 deletions(-) create mode 100644 cmd/credential_test.go diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index d549d81b9..816263c3a 100644 --- a/cmd/admin-handlers.go +++ b/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 { diff --git a/cmd/auth-handler_test.go b/cmd/auth-handler_test.go index b67398bdf..bd6cafd8e 100644 --- a/cmd/auth-handler_test.go +++ b/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. diff --git a/cmd/browser-peer-rpc.go b/cmd/browser-peer-rpc.go index f078c43d0..4602cba27 100644 --- a/cmd/browser-peer-rpc.go +++ b/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 diff --git a/cmd/browser-peer-rpc_test.go b/cmd/browser-peer-rpc_test.go index ccf3301cf..2bea40e9c 100644 --- a/cmd/browser-peer-rpc_test.go +++ b/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) diff --git a/cmd/config-migrate.go b/cmd/config-migrate.go index e45bdfae5..f75b2c18e 100644 --- a/cmd/config-migrate.go +++ b/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. diff --git a/cmd/config-v14.go b/cmd/config-v14.go index 070b7cce5..a2986091e 100644 --- a/cmd/config-v14.go +++ b/cmd/config-v14.go @@ -57,7 +57,7 @@ func newServerConfigV14() *serverConfigV14 { Logger: &logger{}, Notify: ¬ifier{}, } - 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. diff --git a/cmd/credential.go b/cmd/credential.go index c6f9953a4..db0cbf711 100644 --- a/cmd/credential.go +++ b/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 } diff --git a/cmd/credential_test.go b/cmd/credential_test.go new file mode 100644 index 000000000..4d30519d3 --- /dev/null +++ b/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 = , got = %v", err) + } + } else if err == nil { + t.Fatalf("error: expected = %v, got = ", 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) + } + } +} diff --git a/cmd/jwt.go b/cmd/jwt.go index 54f4668f7..430cada9f 100644 --- a/cmd/jwt.go +++ b/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() diff --git a/cmd/jwt_test.go b/cmd/jwt_test.go index c8b531269..7a101252e 100644 --- a/cmd/jwt_test.go +++ b/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. diff --git a/cmd/server-main.go b/cmd/server-main.go index 5f91e4de3..b45c52e22 100644 --- a/cmd/server-main.go +++ b/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. diff --git a/cmd/server_test.go b/cmd/server_test.go index 70fb9e6ce..3dffd14b2 100644 --- a/cmd/server_test.go +++ b/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) diff --git a/cmd/utils.go b/cmd/utils.go index 4d8512408..d644731ed 100644 --- a/cmd/utils.go +++ b/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 { diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index e5a189358..feccf9767 100644 --- a/cmd/web-handlers.go +++ b/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 }