From f8024cadbbf03c39170cb975991441a546fb21c3 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 19 Sep 2017 12:37:56 -0700 Subject: [PATCH] [security] rpc: Do not transfer access/secret key. (#4857) This is an improvement upon existing implementation by avoiding transfer of access and secret keys over the network. This change only exchanges JWT tokens generated by an rpc client. Even if the JWT can be traced over the network on a non-TLS connection, this change makes sure that we never really expose the secret key over the network. --- cmd/admin-rpc-server_test.go | 60 +++++++++++++++++++++++------------- cmd/auth-rpc-client.go | 13 ++++---- cmd/auth-rpc-server.go | 13 +++----- cmd/auth-rpc-server_test.go | 58 +++++++++------------------------- cmd/browser-peer-rpc.go | 20 ------------ cmd/browser-peer-rpc_test.go | 21 +++++++------ cmd/browser-rpc-router.go | 2 +- cmd/jwt.go | 28 +++++++++++------ cmd/jwt_test.go | 53 +++++++++++++++++++++++++++++++ cmd/lock-rpc-server_test.go | 9 +++--- cmd/notifier-config.go | 5 +-- cmd/rpc-common.go | 10 ++---- cmd/web-handlers_test.go | 26 ++++++++++++++++ pkg/quick/quick.go | 7 +---- 14 files changed, 184 insertions(+), 141 deletions(-) diff --git a/cmd/admin-rpc-server_test.go b/cmd/admin-rpc-server_test.go index 2dea7caa0..36c46398d 100644 --- a/cmd/admin-rpc-server_test.go +++ b/cmd/admin-rpc-server_test.go @@ -33,16 +33,19 @@ func testAdminCmd(cmd cmdType, t *testing.T) { } defer os.RemoveAll(rootPath) - adminServer := adminCmd{} creds := serverConfig.GetCredential() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } + + adminServer := adminCmd{} args := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } - reply := LoginRPCReply{} - err = adminServer.Login(&args, &reply) + err = adminServer.Login(&args, &LoginRPCReply{}) if err != nil { t.Fatalf("Failed to login to admin server - %v", err) } @@ -52,7 +55,7 @@ func testAdminCmd(cmd cmdType, t *testing.T) { <-globalServiceSignalCh }() - ga := AuthRPCArgs{AuthToken: reply.AuthToken} + ga := AuthRPCArgs{AuthToken: token} genReply := AuthRPCReply{} switch cmd { case restartCmd: @@ -91,21 +94,25 @@ func TestReInitDisks(t *testing.T) { // Setup admin rpc server for an XL backend. globalIsXL = true adminServer := adminCmd{} + creds := serverConfig.GetCredential() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } + args := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } - reply := LoginRPCReply{} - err = adminServer.Login(&args, &reply) + err = adminServer.Login(&args, &LoginRPCReply{}) if err != nil { t.Fatalf("Failed to login to admin server - %v", err) } authArgs := AuthRPCArgs{ - AuthToken: reply.AuthToken, + AuthToken: token, } authReply := AuthRPCReply{} @@ -114,12 +121,15 @@ func TestReInitDisks(t *testing.T) { t.Errorf("Expected to pass, but failed with %v", err) } + token, err = authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } // Negative test case with admin rpc server setup for FS. globalIsXL = false fsAdminServer := adminCmd{} fsArgs := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } @@ -130,7 +140,7 @@ func TestReInitDisks(t *testing.T) { } authArgs = AuthRPCArgs{ - AuthToken: fsReply.AuthToken, + AuthToken: token, } authReply = AuthRPCReply{} // Attempt ReInitDisks service on a FS backend. @@ -154,9 +164,14 @@ func TestGetConfig(t *testing.T) { adminServer := adminCmd{} creds := serverConfig.GetCredential() + + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } + args := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } @@ -167,7 +182,7 @@ func TestGetConfig(t *testing.T) { } authArgs := AuthRPCArgs{ - AuthToken: reply.AuthToken, + AuthToken: token, } configReply := ConfigReply{} @@ -198,9 +213,12 @@ func TestWriteAndCommitConfig(t *testing.T) { adminServer := adminCmd{} creds := serverConfig.GetCredential() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } args := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } @@ -215,7 +233,7 @@ func TestWriteAndCommitConfig(t *testing.T) { tmpFileName := mustGetUUID() wArgs := WriteConfigArgs{ AuthRPCArgs: AuthRPCArgs{ - AuthToken: reply.AuthToken, + AuthToken: token, }, TmpFileName: tmpFileName, Buf: buf, @@ -232,7 +250,7 @@ func TestWriteAndCommitConfig(t *testing.T) { cArgs := CommitConfigArgs{ AuthRPCArgs: AuthRPCArgs{ - AuthToken: reply.AuthToken, + AuthToken: token, }, FileName: tmpFileName, } diff --git a/cmd/auth-rpc-client.go b/cmd/auth-rpc-client.go index f9e667662..523019c77 100644 --- a/cmd/auth-rpc-client.go +++ b/cmd/auth-rpc-client.go @@ -99,21 +99,22 @@ func (authClient *AuthRPCClient) Login() (err error) { // Attempt to login if not logged in already. if authClient.authToken == "" { - // Login to authenticate and acquire a new auth token. + authClient.authToken, err = authenticateNode(authClient.config.accessKey, authClient.config.secretKey) + if err != nil { + return err + } + // Login to authenticate your token. var ( loginMethod = authClient.config.serviceName + loginMethodName loginArgs = LoginRPCArgs{ - Username: authClient.config.accessKey, - Password: authClient.config.secretKey, + AuthToken: authClient.authToken, Version: Version, RequestTime: UTCNow(), } - loginReply = LoginRPCReply{} ) - if err = authClient.rpcClient.Call(loginMethod, &loginArgs, &loginReply); err != nil { + if err = authClient.rpcClient.Call(loginMethod, &loginArgs, &LoginRPCReply{}); err != nil { return err } - authClient.authToken = loginReply.AuthToken } return nil } diff --git a/cmd/auth-rpc-server.go b/cmd/auth-rpc-server.go index 05a62b826..86e6a3cac 100644 --- a/cmd/auth-rpc-server.go +++ b/cmd/auth-rpc-server.go @@ -20,8 +20,7 @@ package cmd const loginMethodName = ".Login" // AuthRPCServer RPC server authenticates using JWT. -type AuthRPCServer struct { -} +type AuthRPCServer struct{} // Login - Handles JWT based RPC login. func (b AuthRPCServer) Login(args *LoginRPCArgs, reply *LoginRPCReply) error { @@ -30,14 +29,10 @@ func (b AuthRPCServer) Login(args *LoginRPCArgs, reply *LoginRPCReply) error { return err } - // Authenticate using JWT. - token, err := authenticateNode(args.Username, args.Password) - if err != nil { - return err + // Return an error if token is not valid. + if !isAuthTokenValid(args.AuthToken) { + return errAuthentication } - // Return the token. - reply.AuthToken = token - return nil } diff --git a/cmd/auth-rpc-server_test.go b/cmd/auth-rpc-server_test.go index 21355fcfc..fbd853952 100644 --- a/cmd/auth-rpc-server_test.go +++ b/cmd/auth-rpc-server_test.go @@ -29,6 +29,10 @@ func TestLogin(t *testing.T) { } defer os.RemoveAll(rootPath) creds := serverConfig.GetCredential() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } ls := AuthRPCServer{} testCases := []struct { args LoginRPCArgs @@ -38,9 +42,8 @@ func TestLogin(t *testing.T) { // Valid case. { args: LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, - Version: Version, + AuthToken: token, + Version: Version, }, skewTime: 0, expectedErr: nil, @@ -48,9 +51,8 @@ func TestLogin(t *testing.T) { // Valid username, password and request time, not version. { args: LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, - Version: "INVALID-" + Version, + AuthToken: token, + Version: "INVALID-" + Version, }, skewTime: 0, expectedErr: errServerVersionMismatch, @@ -58,49 +60,17 @@ func TestLogin(t *testing.T) { // Valid username, password and version, not request time { args: LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, - Version: Version, + AuthToken: token, + Version: Version, }, skewTime: 20 * time.Minute, expectedErr: errServerTimeMismatch, }, - // Invalid username length - { - args: LoginRPCArgs{ - Username: "aaa", - Password: "minio123", - Version: Version, - }, - skewTime: 0, - expectedErr: errInvalidAccessKeyLength, - }, - // Invalid password length - { - args: LoginRPCArgs{ - Username: "minio", - Password: "aaa", - Version: Version, - }, - skewTime: 0, - expectedErr: errInvalidSecretKeyLength, - }, - // Invalid username - { - args: LoginRPCArgs{ - Username: "aaaaa", - Password: creds.SecretKey, - Version: Version, - }, - skewTime: 0, - expectedErr: errInvalidAccessKeyID, - }, - // Invalid password + // Invalid token, fails with authentication error { args: LoginRPCArgs{ - Username: creds.AccessKey, - Password: "aaaaaaaa", - Version: Version, + AuthToken: "", + Version: Version, }, skewTime: 0, expectedErr: errAuthentication, @@ -108,7 +78,7 @@ func TestLogin(t *testing.T) { } for i, test := range testCases { reply := LoginRPCReply{} - test.args.RequestTime = time.Now().Add(test.skewTime).UTC() + test.args.RequestTime = UTCNow().Add(test.skewTime) err := ls.Login(&test.args, &reply) if err != test.expectedErr { t.Errorf("Test %d: Expected error %v but received %v", diff --git a/cmd/browser-peer-rpc.go b/cmd/browser-peer-rpc.go index b985d54ae..6ae72900c 100644 --- a/cmd/browser-peer-rpc.go +++ b/cmd/browser-peer-rpc.go @@ -23,26 +23,6 @@ import ( "time" ) -// Login handler implements JWT login token generator, which upon login request -// along with username and password is generated. -func (br *browserPeerAPIHandlers) Login(args *LoginRPCArgs, reply *LoginRPCReply) error { - // Validate LoginRPCArgs - if err := args.IsValid(); err != nil { - return err - } - - // Authenticate using JWT. - token, err := authenticateWeb(args.Username, args.Password) - if err != nil { - return err - } - - // Return the token. - reply.AuthToken = token - - return nil -} - // SetAuthPeerArgs - Arguments collection for SetAuth RPC call type SetAuthPeerArgs struct { // For Auth diff --git a/cmd/browser-peer-rpc_test.go b/cmd/browser-peer-rpc_test.go index 16e0aae30..4f3a4b122 100644 --- a/cmd/browser-peer-rpc_test.go +++ b/cmd/browser-peer-rpc_test.go @@ -91,9 +91,12 @@ func (s *TestRPCBrowserPeerSuite) testBrowserPeerRPC(t *testing.T) { // Validate for failure in login handler with previous credentials. rclient = newRPCClient(s.testAuthConf.serverAddr, s.testAuthConf.serviceEndpoint, false) defer rclient.Close() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } rargs := &LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } @@ -105,20 +108,18 @@ func (s *TestRPCBrowserPeerSuite) testBrowserPeerRPC(t *testing.T) { } } + token, err = authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } // Validate for success in loing handled with valid credetnails. rargs = &LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } rreply = &LoginRPCReply{} - err = rclient.Call("BrowserPeer"+loginMethodName, rargs, rreply) - if err != nil { + if err = rclient.Call("BrowserPeer"+loginMethodName, rargs, rreply); err != nil { t.Fatal(err) } - // Validate all the replied fields after successful login. - if rreply.AuthToken == "" { - t.Fatalf("Generated token cannot be empty %s", errInvalidToken) - } } diff --git a/cmd/browser-rpc-router.go b/cmd/browser-rpc-router.go index fdf88c856..f79488413 100644 --- a/cmd/browser-rpc-router.go +++ b/cmd/browser-rpc-router.go @@ -35,7 +35,7 @@ type browserPeerAPIHandlers struct { // Register RPC router func registerBrowserPeerRPCRouter(mux *router.Router) error { - bpHandlers := &browserPeerAPIHandlers{} + bpHandlers := &browserPeerAPIHandlers{AuthRPCServer{}} bpRPCServer := newRPCServer() err := bpRPCServer.RegisterName("BrowserPeer", bpHandlers) diff --git a/cmd/jwt.go b/cmd/jwt.go index d1f39ac42..980c62551 100644 --- a/cmd/jwt.go +++ b/cmd/jwt.go @@ -63,10 +63,10 @@ func authenticateJWT(accessKey, secretKey string, expiry time.Duration) (string, } utcNow := UTCNow() - token := jwtgo.NewWithClaims(jwtgo.SigningMethodHS512, jwtgo.MapClaims{ - "exp": utcNow.Add(expiry).Unix(), - "iat": utcNow.Unix(), - "sub": accessKey, + token := jwtgo.NewWithClaims(jwtgo.SigningMethodHS512, jwtgo.StandardClaims{ + ExpiresAt: utcNow.Add(expiry).Unix(), + IssuedAt: utcNow.Unix(), + Subject: accessKey, }) return token.SignedString([]byte(serverCred.SecretKey)) @@ -93,13 +93,17 @@ func keyFuncCallback(jwtToken *jwtgo.Token) (interface{}, error) { } func isAuthTokenValid(tokenString string) bool { - jwtToken, err := jwtgo.Parse(tokenString, keyFuncCallback) + var claims jwtgo.StandardClaims + jwtToken, err := jwtgo.ParseWithClaims(tokenString, &claims, keyFuncCallback) if err != nil { errorIf(err, "Unable to parse JWT token string") return false } - - return jwtToken.Valid + if err = claims.Valid(); err != nil { + errorIf(err, "Invalid claims in JWT token string") + return false + } + return jwtToken.Valid && claims.Subject == serverConfig.GetCredential().AccessKey } func isHTTPRequestValid(req *http.Request) bool { @@ -110,14 +114,20 @@ func isHTTPRequestValid(req *http.Request) bool { // Returns nil if the request is authenticated. errNoAuthToken if token missing. // Returns errAuthentication for all other errors. func webRequestAuthenticate(req *http.Request) error { - jwtToken, err := jwtreq.ParseFromRequest(req, jwtreq.AuthorizationHeaderExtractor, keyFuncCallback) + var claims jwtgo.StandardClaims + jwtToken, err := jwtreq.ParseFromRequestWithClaims(req, jwtreq.AuthorizationHeaderExtractor, &claims, keyFuncCallback) if err != nil { if err == jwtreq.ErrNoTokenInRequest { return errNoAuthToken } return errAuthentication } - + if err = claims.Valid(); err != nil { + return err + } + if claims.Subject != serverConfig.GetCredential().AccessKey { + return errInvalidAccessKeyID + } if !jwtToken.Valid { return errAuthentication } diff --git a/cmd/jwt_test.go b/cmd/jwt_test.go index 9f075ae28..f7b2efe10 100644 --- a/cmd/jwt_test.go +++ b/cmd/jwt_test.go @@ -17,6 +17,7 @@ package cmd import ( + "net/http" "os" "testing" ) @@ -88,6 +89,58 @@ func TestAuthenticateURL(t *testing.T) { testAuthenticate("url", t) } +// Tests web request authenticator. +func TestWebRequestAuthenticate(t *testing.T) { + testPath, err := newTestConfig(globalMinioDefaultRegion) + if err != nil { + t.Fatalf("unable initialize config file, %s", err) + } + defer os.RemoveAll(testPath) + + creds := serverConfig.GetCredential() + token, err := getTokenString(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatalf("unable get token %s", err) + } + testCases := []struct { + req *http.Request + expectedErr error + }{ + // Set valid authorization header. + { + req: &http.Request{ + Header: http.Header{ + "Authorization": []string{token}, + }, + }, + expectedErr: nil, + }, + // No authorization header. + { + req: &http.Request{ + Header: http.Header{}, + }, + expectedErr: errNoAuthToken, + }, + // Invalid authorization token. + { + req: &http.Request{ + Header: http.Header{ + "Authorization": []string{"invalid-token"}, + }, + }, + expectedErr: errAuthentication, + }, + } + + for i, testCase := range testCases { + gotErr := webRequestAuthenticate(testCase.req) + if testCase.expectedErr != gotErr { + t.Errorf("Test %d, expected err %s, got %s", i+1, testCase.expectedErr, gotErr) + } + } +} + func BenchmarkAuthenticateNode(b *testing.B) { testPath, err := newTestConfig(globalMinioDefaultRegion) if err != nil { diff --git a/cmd/lock-rpc-server_test.go b/cmd/lock-rpc-server_test.go index 8ae66ff02..31ff8c42b 100644 --- a/cmd/lock-rpc-server_test.go +++ b/cmd/lock-rpc-server_test.go @@ -58,9 +58,12 @@ func createLockTestServer(t *testing.T) (string, *lockServer, string) { }, } creds := serverConfig.GetCredential() + token, err := authenticateNode(creds.AccessKey, creds.SecretKey) + if err != nil { + t.Fatal(err) + } loginArgs := LoginRPCArgs{ - Username: creds.AccessKey, - Password: creds.SecretKey, + AuthToken: token, Version: Version, RequestTime: UTCNow(), } @@ -69,8 +72,6 @@ func createLockTestServer(t *testing.T) (string, *lockServer, string) { if err != nil { t.Fatalf("Failed to login to lock server - %v", err) } - token := loginReply.AuthToken - return testPath, locker, token } diff --git a/cmd/notifier-config.go b/cmd/notifier-config.go index 044033274..e46c16eb8 100644 --- a/cmd/notifier-config.go +++ b/cmd/notifier-config.go @@ -235,10 +235,7 @@ func (n *notifier) Validate() error { if err := n.MySQL.Validate(); err != nil { return err } - if err := n.MQTT.Validate(); err != nil { - return err - } - return nil + return n.MQTT.Validate() } func (n *notifier) SetAMQPByID(accountID string, amqpn amqpNotify) { diff --git a/cmd/rpc-common.go b/cmd/rpc-common.go index 9761787e3..5f5e56e95 100644 --- a/cmd/rpc-common.go +++ b/cmd/rpc-common.go @@ -60,8 +60,7 @@ type AuthRPCReply struct{} // LoginRPCArgs - login username and password for RPC. type LoginRPCArgs struct { - Username string - Password string + AuthToken string Version string RequestTime time.Time } @@ -80,11 +79,8 @@ func (args LoginRPCArgs) IsValid() error { return nil } -// LoginRPCReply - login reply provides generated token to be used -// with subsequent requests. -type LoginRPCReply struct { - AuthToken string -} +// LoginRPCReply - login reply is a dummy struct perhaps for future use. +type LoginRPCReply struct{} // LockArgs represents arguments for any authenticated lock RPC call. type LockArgs struct { diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index 571f4cf60..b2ddc8698 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -34,6 +34,7 @@ import ( "strings" "testing" + jwtgo "github.com/dgrijalva/jwt-go" humanize "github.com/dustin/go-humanize" "github.com/minio/minio-go/pkg/policy" "github.com/minio/minio-go/pkg/set" @@ -667,6 +668,16 @@ func TestWebCreateURLToken(t *testing.T) { ExecObjectLayerTest(t, testCreateURLToken) } +func getTokenString(accessKey, secretKey string) (string, error) { + utcNow := UTCNow() + token := jwtgo.NewWithClaims(jwtgo.SigningMethodHS512, jwtgo.StandardClaims{ + ExpiresAt: utcNow.Add(defaultJWTExpiry).Unix(), + IssuedAt: utcNow.Unix(), + Subject: accessKey, + }) + return token.SignedString([]byte(secretKey)) +} + func testCreateURLToken(obj ObjectLayer, instanceType string, t TestErrHandler) { apiRouter := initTestWebRPCEndPoint(obj) credentials := serverConfig.GetCredential() @@ -700,6 +711,21 @@ func testCreateURLToken(obj ObjectLayer, instanceType string, t TestErrHandler) if !isAuthTokenValid(tokenReply.Token) { t.Fatalf("token is not valid") } + + // Token is invalid. + if isAuthTokenValid("") { + t.Fatalf("token shouldn't be valid, but it is") + } + + token, err := getTokenString("invalid-access", credentials.SecretKey) + if err != nil { + t.Fatal(err) + } + + // Token has invalid access key. + if isAuthTokenValid(token) { + t.Fatalf("token shouldn't be valid, but it is") + } } // Wrapper for calling Upload Handler diff --git a/pkg/quick/quick.go b/pkg/quick/quick.go index e010c8f9a..a8a730a83 100644 --- a/pkg/quick/quick.go +++ b/pkg/quick/quick.go @@ -92,12 +92,7 @@ func (d config) Save(filename string) error { func (d config) Load(filename string) error { d.lock.Lock() defer d.lock.Unlock() - - if err := loadFileConfig(filename, d.data); err != nil { - return err - } - - return nil + return loadFileConfig(filename, d.data) } // Data - grab internal data map for reading