From ec5293ce29ef03726c7174d9e3304bc156259c4e Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Mon, 24 Jul 2017 12:46:37 -0700 Subject: [PATCH] jwt,browser: allow short-expiry tokens for GETs (#4684) This commit fixes a potential security issue, whereby a full-access token to the server would be available in the GET URL of a download request. This fixes that issue by introducing short-expiry tokens, which are only valid for one minute, and are regenerated for every download request. This commit specifically introduces the short-lived tokens, adds tests for the tokens, adds an RPC call for generating a token given a full-access token, updates the browser to use the new tokens for requests where the token is passed as a GET parameter, and adds some tests with the new temporary tokens. Refs: https://github.com/minio/minio/pull/4673 --- browser/app/js/components/Browse.js | 27 +++++++++--- browser/app/js/web.js | 3 ++ cmd/jwt.go | 7 +++ cmd/jwt_test.go | 6 +++ cmd/web-handlers.go | 24 +++++++++++ cmd/web-handlers_test.go | 67 ++++++++++++++++++++++++++++- cmd/web-router.go | 3 ++ 7 files changed, 131 insertions(+), 6 deletions(-) diff --git a/browser/app/js/components/Browse.js b/browser/app/js/components/Browse.js index 15d1d508c..f532c4a44 100644 --- a/browser/app/js/components/Browse.js +++ b/browser/app/js/components/Browse.js @@ -150,7 +150,16 @@ export default class Browse extends React.Component { if (prefix === currentPath) return browserHistory.push(utils.pathJoin(currentBucket, encPrefix)) } else { - window.location = `${window.location.origin}/minio/download/${currentBucket}/${encPrefix}?token=${storage.getItem('token')}` + // Download the selected file. + web.CreateURLToken() + .then(res => { + let url = `${window.location.origin}/minio/download/${currentBucket}/${encPrefix}?token=${res.token}` + window.location = url + }) + .catch(err => dispatch(actions.showAlert({ + type: 'danger', + message: err.message + }))) } } @@ -406,16 +415,24 @@ export default class Browse extends React.Component { } downloadSelected() { - const {dispatch} = this.props + const {dispatch, web} = this.props let req = { bucketName: this.props.currentBucket, objects: this.props.checkedObjects, prefix: this.props.currentPath } - let requestUrl = location.origin + "/minio/zip?token=" + localStorage.token - this.xhr = new XMLHttpRequest() - dispatch(actions.downloadSelected(requestUrl, req, this.xhr)) + web.CreateURLToken() + .then(res => { + let requestUrl = location.origin + "/minio/zip?token=" + res.token + + this.xhr = new XMLHttpRequest() + dispatch(actions.downloadSelected(requestUrl, req, this.xhr)) + }) + .catch(err => dispatch(actions.showAlert({ + type: 'danger', + message: err.message + }))) } clearSelected() { diff --git a/browser/app/js/web.js b/browser/app/js/web.js index 96b37ba05..df78595e3 100644 --- a/browser/app/js/web.js +++ b/browser/app/js/web.js @@ -112,6 +112,9 @@ export default class Web { return res }) } + CreateURLToken() { + return this.makeCall('CreateURLToken') + } GetBucketPolicy(args) { return this.makeCall('GetBucketPolicy', args) } diff --git a/cmd/jwt.go b/cmd/jwt.go index b2bd75729..d1f39ac42 100644 --- a/cmd/jwt.go +++ b/cmd/jwt.go @@ -34,6 +34,9 @@ const ( // Inter-node JWT token expiry is 100 years approx. defaultInterNodeJWTExpiry = 100 * 365 * 24 * time.Hour + + // URL JWT token expiry is one minute (might be exposed). + defaultURLJWTExpiry = time.Minute ) var ( @@ -77,6 +80,10 @@ func authenticateWeb(accessKey, secretKey string) (string, error) { return authenticateJWT(accessKey, secretKey, defaultJWTExpiry) } +func authenticateURL(accessKey, secretKey string) (string, error) { + return authenticateJWT(accessKey, secretKey, defaultURLJWTExpiry) +} + func keyFuncCallback(jwtToken *jwtgo.Token) (interface{}, error) { if _, ok := jwtToken.Method.(*jwtgo.SigningMethodHMAC); !ok { return nil, fmt.Errorf("Unexpected signing method: %v", jwtToken.Header["alg"]) diff --git a/cmd/jwt_test.go b/cmd/jwt_test.go index 3edc25c28..ac0c512d1 100644 --- a/cmd/jwt_test.go +++ b/cmd/jwt_test.go @@ -60,6 +60,8 @@ func testAuthenticate(authType string, t *testing.T) { _, err = authenticateNode(testCase.accessKey, testCase.secretKey) } else if authType == "web" { _, err = authenticateWeb(testCase.accessKey, testCase.secretKey) + } else if authType == "url" { + _, err = authenticateURL(testCase.accessKey, testCase.secretKey) } if testCase.expectedErr != nil { @@ -83,6 +85,10 @@ func TestAuthenticateWeb(t *testing.T) { testAuthenticate("web", t) } +func TestAuthenticateURL(t *testing.T) { + testAuthenticate("url", t) +} + func BenchmarkAuthenticateNode(b *testing.B) { testPath, err := newTestConfig(globalMinioDefaultRegion) if err != nil { diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index dced81ba2..1e886f45d 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -467,6 +467,30 @@ func (web *webAPIHandlers) GetAuth(r *http.Request, args *WebGenericArgs, reply return nil } +// URLTokenReply contains the reply for CreateURLToken. +type URLTokenReply struct { + Token string `json:"token"` + UIVersion string `json:"uiVersion"` +} + +// CreateURLToken creates a URL token (short-lived) for GET requests. +func (web *webAPIHandlers) CreateURLToken(r *http.Request, args *WebGenericArgs, reply *URLTokenReply) error { + if !isHTTPRequestValid(r) { + return toJSONError(errAuthentication) + } + + creds := serverConfig.GetCredential() + + token, err := authenticateURL(creds.AccessKey, creds.SecretKey) + if err != nil { + return toJSONError(err) + } + + reply.Token = token + reply.UIVersion = browser.UIVersion + return nil +} + // Upload - file upload handler. func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) { objectAPI := web.ObjectAPI() diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index 728b053ba..dfcad8c24 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -662,6 +662,45 @@ func testGetAuthWebHandler(obj ObjectLayer, instanceType string, t TestErrHandle } } +func TestWebCreateURLToken(t *testing.T) { + ExecObjectLayerTest(t, testCreateURLToken) +} + +func testCreateURLToken(obj ObjectLayer, instanceType string, t TestErrHandler) { + apiRouter := initTestWebRPCEndPoint(obj) + credentials := serverConfig.GetCredential() + + authorization, err := getWebRPCToken(apiRouter, credentials.AccessKey, credentials.SecretKey) + if err != nil { + t.Fatal(err) + } + + args := WebGenericArgs{} + tokenReply := &URLTokenReply{} + + req, err := newTestWebRPCRequest("Web.CreateURLToken", authorization, args) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("Expected the response status to be 200, but instead found `%d`", rec.Code) + } + + err = getTestWebRPCResponse(rec, &tokenReply) + if err != nil { + t.Fatal(err) + } + + // Ensure the token is valid now. It will expire later. + if !isAuthTokenValid(tokenReply.Token) { + t.Fatalf("token is not valid") + } +} + // Wrapper for calling Upload Handler func TestWebHandlerUpload(t *testing.T) { ExecObjectLayerTest(t, testUploadWebHandler) @@ -815,6 +854,32 @@ func testDownloadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandl t.Fatalf("The downloaded file is corrupted") } + // Temporary token should succeed. + tmpToken, err := authenticateURL(credentials.AccessKey, credentials.SecretKey) + if err != nil { + t.Fatal(err) + } + + code, bodyContent = test(tmpToken) + + if code != http.StatusOK { + t.Fatalf("Expected the response status to be 200, but instead found `%d`", code) + } + + if !bytes.Equal(bodyContent, content) { + t.Fatalf("The downloaded file is corrupted") + } + + // Old token should fail. + code, bodyContent = test("eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE1MDAzMzIwOTUsImlhdCI6MTUwMDMzMjAzNSwic3ViIjoiRFlLSU01VlRZNDBJMVZQSE5VMTkifQ.tXQ45GJc8eOFet_a4VWVyeqJEOPWybotQYNr2zVxBpEOICkGbu_YWGhd9TkLLe1E65oeeiLHPdXSN8CzcbPoRA") + if code != http.StatusForbidden { + t.Fatalf("Expected the response status to be 403, but instead found `%d`", code) + } + + if !bytes.Equal(bodyContent, bytes.NewBufferString("Authentication failed, check your access credentials").Bytes()) { + t.Fatalf("Expected authentication error message, got %v", bodyContent) + } + // Unauthenticated download should fail. code, _ = test("") if code != http.StatusForbidden { @@ -848,7 +913,7 @@ func testWebHandlerDownloadZip(obj ObjectLayer, instanceType string, t TestErrHa apiRouter := initTestWebRPCEndPoint(obj) credentials := serverConfig.GetCredential() - authorization, err := getWebRPCToken(apiRouter, credentials.AccessKey, credentials.SecretKey) + authorization, err := authenticateURL(credentials.AccessKey, credentials.SecretKey) if err != nil { t.Fatal("Cannot authenticate") } diff --git a/cmd/web-router.go b/cmd/web-router.go index 6f85156c3..76a388ad3 100644 --- a/cmd/web-router.go +++ b/cmd/web-router.go @@ -83,6 +83,9 @@ func registerWebRouter(mux *router.Router) error { // RPC handler at URI - /minio/webrpc webBrowserRouter.Methods("POST").Path("/webrpc").Handler(webRPC) webBrowserRouter.Methods("PUT").Path("/upload/{bucket}/{object:.+}").HandlerFunc(web.Upload) + + // These methods use short-expiry tokens in the URLs. These tokens may unintentionally + // be logged, so a new one must be generated for each request. webBrowserRouter.Methods("GET").Path("/download/{bucket}/{object:.+}").Queries("token", "{token:.*}").HandlerFunc(web.Download) webBrowserRouter.Methods("POST").Path("/zip").Queries("token", "{token:.*}").HandlerFunc(web.DownloadZip)