Do not list objects unless specified in policy (#6970)

Currently we use GetObject to check if we are allowed to list, this might be a security problem since there are many users now who actively disable a publicly readable listing, anyone who can guess the browser URL can list the objects. This PR turns off this behavior and provides a more expected way based on the policies. This PR also additionally improves the Download() object implementation to use a more streamlined code. These are precursor changes to facilitate federation and web identity support in browser.
6 years ago · c2ed1347d9
parent 50f6f9fe58
commit c2ed1347d9
2 changed files with 56 additions and 82 deletions
--- a/cmd/web-handlers.go
+++ b/cmd/web-handlers.go
@ -271,9 +271,10 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re
 		}
 		for _, dnsRecord := range dnsBuckets {
 			bucketName := strings.Trim(dnsRecord.Key, "/")
+
 			if globalIAMSys.IsAllowed(iampolicy.Args{
 				AccountName:     claims.Subject,
-				Action:          iampolicy.Action(policy.GetObjectAction),
+				Action:          iampolicy.ListBucketAction,
 				BucketName:      bucketName,
 				ConditionValues: getConditionValues(r, ""),
 				IsOwner:         owner,
@ -293,7 +294,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re
 		for _, bucket := range buckets {
 			if globalIAMSys.IsAllowed(iampolicy.Args{
 				AccountName:     claims.Subject,
-				Action:          iampolicy.Action(policy.GetObjectAction),
+				Action:          iampolicy.ListBucketAction,
 				BucketName:      bucket.Name,
 				ConditionValues: getConditionValues(r, ""),
 				IsOwner:         owner,
@ -355,13 +356,17 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r
 	claims, owner, authErr := webRequestAuthenticate(r)
 	if authErr != nil {
 		if authErr == errNoAuthToken {
+			// Add this for checking ListObjects conditional.
+			if args.Prefix != "" {
+				r.Header.Set("prefix", args.Prefix)
+			}
+
 			// Check if anonymous (non-owner) has access to download objects.
 			readable := globalPolicySys.IsAllowed(policy.Args{
-				Action:          policy.GetObjectAction,
+				Action:          policy.ListBucketAction,
 				BucketName:      args.BucketName,
 				ConditionValues: getConditionValues(r, ""),
 				IsOwner:         false,
-				ObjectName:      args.Prefix + "/",
 			})

 			// Check if anonymous (non-owner) has access to upload objects.
@ -389,18 +394,22 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r

 	// For authenticated users apply IAM policy.
 	if authErr == nil {
+		// Add this for checking ListObjects conditional.
+		if args.Prefix != "" {
+			r.Header.Set("prefix", args.Prefix)
+		}
+
 		readable := globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     claims.Subject,
-			Action:          iampolicy.Action(policy.GetObjectAction),
+			Action:          iampolicy.ListBucketAction,
 			BucketName:      args.BucketName,
 			ConditionValues: getConditionValues(r, ""),
 			IsOwner:         owner,
-			ObjectName:      args.Prefix + "/",
 		})

 		writable := globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     claims.Subject,
-			Action:          iampolicy.Action(policy.PutObjectAction),
+			Action:          iampolicy.PutObjectAction,
 			BucketName:      args.BucketName,
 			ConditionValues: getConditionValues(r, ""),
 			IsOwner:         owner,
@ -498,7 +507,7 @@ next:

 			if !globalIAMSys.IsAllowed(iampolicy.Args{
 				AccountName:     claims.Subject,
-				Action:          iampolicy.Action(policy.DeleteObjectAction),
+				Action:          iampolicy.DeleteObjectAction,
 				BucketName:      args.BucketName,
 				ConditionValues: getConditionValues(r, ""),
 				IsOwner:         owner,
@ -515,7 +524,7 @@ next:

 		if !globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     claims.Subject,
-			Action:          iampolicy.Action(policy.DeleteObjectAction),
+			Action:          iampolicy.DeleteObjectAction,
 			BucketName:      args.BucketName,
 			ConditionValues: getConditionValues(r, ""),
 			IsOwner:         owner,
@ -766,7 +775,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 	if authErr == nil {
 		if !globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     claims.Subject,
-			Action:          iampolicy.Action(policy.PutObjectAction),
+			Action:          iampolicy.PutObjectAction,
 			BucketName:      bucket,
 			ConditionValues: getConditionValues(r, ""),
 			IsOwner:         owner,
@ -866,7 +875,6 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {

 	defer logger.AuditLog(w, r, "WebDownload", mustGetClaimsFromToken(r))

-	var wg sync.WaitGroup
 	objectAPI := web.ObjectAPI()
 	if objectAPI == nil {
 		writeWebErrorResponse(w, errServerNotInitialized)
@ -902,7 +910,7 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {
 	if authErr == nil {
 		if !globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     claims.Subject,
-			Action:          iampolicy.Action(policy.GetObjectAction),
+			Action:          iampolicy.GetObjectAction,
 			BucketName:      bucket,
 			ConditionValues: getConditionValues(r, ""),
 			IsOwner:         owner,
@ -913,101 +921,67 @@ func (web *webAPIHandlers) Download(w http.ResponseWriter, r *http.Request) {
 		}
 	}

-	opts := ObjectOptions{}
-	getObjectInfo := objectAPI.GetObjectInfo
-	getObject := objectAPI.GetObject
+	getObjectNInfo := objectAPI.GetObjectNInfo
 	if web.CacheAPI() != nil {
-		getObjectInfo = web.CacheAPI().GetObjectInfo
-		getObject = web.CacheAPI().GetObject
+		getObjectNInfo = web.CacheAPI().GetObjectNInfo
 	}
-	objInfo, err := getObjectInfo(ctx, bucket, object, opts)
+
+	var opts ObjectOptions
+	gr, err := getObjectNInfo(ctx, bucket, object, nil, r.Header, readLock, opts)
 	if err != nil {
 		writeWebErrorResponse(w, err)
 		return
 	}
-	length := objInfo.Size
-	var actualSize int64
-	if objInfo.IsCompressed() {
-		// Read the decompressed size from the meta.json.
-		actualSize = objInfo.GetActualSize()
-		if actualSize < 0 {
-			return
-		}
-	}
+	defer gr.Close()
+
+	objInfo := gr.ObjInfo
+
 	if objectAPI.IsEncryptionSupported() {
 		if _, err = DecryptObjectInfo(&objInfo, r.Header); err != nil {
 			writeWebErrorResponse(w, err)
 			return
 		}
+	}
+
+	// Set encryption response headers
+	if objectAPI.IsEncryptionSupported() {
 		if crypto.IsEncrypted(objInfo.UserDefined) {
-			length, _ = objInfo.DecryptedSize()
+			switch {
+			case crypto.S3.IsEncrypted(objInfo.UserDefined):
+				w.Header().Set(crypto.SSEHeader, crypto.SSEAlgorithmAES256)
+			case crypto.SSEC.IsEncrypted(objInfo.UserDefined):
+				w.Header().Set(crypto.SSECAlgorithm, r.Header.Get(crypto.SSECAlgorithm))
+				w.Header().Set(crypto.SSECKeyMD5, r.Header.Get(crypto.SSECKeyMD5))
+			}
 		}
 	}
-	var startOffset int64
-	var writer io.Writer
-	if objInfo.IsCompressed() {
-		// The decompress metrics are set.
-		snappyStartOffset := 0
-		snappyLength := actualSize
-
-		// Open a pipe for compression
-		// Where compressWriter is actually passed to the getObject
-		decompressReader, compressWriter := io.Pipe()
-		snappyReader := snappy.NewReader(decompressReader)
-
-		// The limit is set to the actual size.
-		responseWriter := ioutil.LimitedWriter(w, int64(snappyStartOffset), snappyLength)
-		wg.Add(1) //For closures.
-		go func() {
-			defer wg.Done()

-			// Finally, writes to the client.
-			_, perr := io.Copy(responseWriter, snappyReader)
-
-			// Close the compressWriter if the data is read already.
-			// Closing the pipe, releases the writer passed to the getObject.
-			compressWriter.CloseWithError(perr)
-		}()
-		writer = compressWriter
-	} else {
-		writer = w
+	if err = setObjectHeaders(w, objInfo, nil); err != nil {
+		writeWebErrorResponse(w, err)
+		return
 	}
-	if objectAPI.IsEncryptionSupported() && crypto.S3.IsEncrypted(objInfo.UserDefined) {
-		// Response writer should be limited early on for decryption upto required length,
-		// additionally also skipping mod(offset)64KiB boundaries.
-		writer = ioutil.LimitedWriter(writer, startOffset%(64*1024), length)

-		writer, startOffset, length, err = DecryptBlocksRequest(writer, r, bucket, object, startOffset, length, objInfo, false)
-		if err != nil {
-			writeWebErrorResponse(w, err)
-			return
-		}
-		w.Header().Set(crypto.SSEHeader, crypto.SSEAlgorithmAES256)
-	}
+	// Add content disposition.
+	w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", path.Base(objInfo.Name)))

-	httpWriter := ioutil.WriteOnClose(writer)
+	setHeadGetRespHeaders(w, r.URL.Query())

-	// Add content disposition.
-	w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", path.Base(object)))
+	httpWriter := ioutil.WriteOnClose(w)

-	if err = getObject(ctx, bucket, object, 0, -1, httpWriter, "", opts); err != nil {
-		httpWriter.Close()
-		if objInfo.IsCompressed() {
-			wg.Wait()
+	// Write object content to response body
+	if _, err = io.Copy(httpWriter, gr); err != nil {
+		if !httpWriter.HasWritten() { // write error response only if no data or headers has been written to client yet
+			writeWebErrorResponse(w, err)
 		}
-		/// No need to print error, response writer already written to.
 		return
 	}
+
 	if err = httpWriter.Close(); err != nil {
-		if !httpWriter.HasWritten() { // write error response only if no data has been written to client yet
+		if !httpWriter.HasWritten() { // write error response only if no data or headers has been written to client yet
 			writeWebErrorResponse(w, err)
 			return
 		}
 	}
-	if objInfo.IsCompressed() {
-		// Wait for decompression go-routine to retire.
-		wg.Wait()
-	}

 	// Get host and port from Request.RemoteAddr.
 	host, port, err := net.SplitHostPort(handlers.GetSourceIP(r))
@ -1093,7 +1067,7 @@ func (web *webAPIHandlers) DownloadZip(w http.ResponseWriter, r *http.Request) {
 		for _, object := range args.Objects {
 			if !globalIAMSys.IsAllowed(iampolicy.Args{
 				AccountName:     claims.Subject,
-				Action:          iampolicy.Action(policy.GetObjectAction),
+				Action:          iampolicy.GetObjectAction,
 				BucketName:      args.BucketName,
 				ConditionValues: getConditionValues(r, ""),
 				IsOwner:         owner,
--- a/cmd/web-handlers_test.go
+++ b/cmd/web-handlers_test.go
@ -539,8 +539,8 @@ func testListObjectsWebHandler(obj ObjectLayer, instanceType string, t TestErrHa
 		Statements: []policy.Statement{policy.NewStatement(
 			policy.Allow,
 			policy.NewPrincipal("*"),
-			policy.NewActionSet(policy.GetObjectAction),
-			policy.NewResourceSet(policy.NewResource(bucketName, "*")),
+			policy.NewActionSet(policy.ListBucketAction),
+			policy.NewResourceSet(policy.NewResource(bucketName, "")),
 			condition.NewFunctions(),
 		)},
 	}