posix: Mkdir() and OpenFile() should honor umask. (#1972)

Adds two unit tests for validation as well.

Fixes #1965

logger-file-hook.go

@@ -39,7 +39,8 @@ func enableFileLogger() {
 		return
 	}
 
-	file, err := os.OpenFile(flogger.Filename, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600)
+	// Creates the named file with mode 0666, honors system umask.
+	file, err := os.OpenFile(flogger.Filename, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0666)
 	fatalIf(err, "Unable to open log file.")
 
 	// Add a local file hook.

posix-utils_nix_test.go

@@ -0,0 +1,126 @@
+// +build linux darwin dragonfly freebsd netbsd openbsd
+
+/*
+ * Minio Cloud Storage, (C) 2016 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 main
+
+import (
+	"io/ioutil"
+	"os"
+	"path"
+	"syscall"
+	"testing"
+)
+
+// Based on `man getumask` a vaporware GNU extension to glibc.
+// returns file mode creation mask.
+func getUmask() int {
+	mask := syscall.Umask(0)
+	syscall.Umask(mask)
+	return mask
+}
+
+// Tests if the directory and file creations happen with proper umask.
+func TestIsValidUmaskVol(t *testing.T) {
+	tmpPath, err := ioutil.TempDir(os.TempDir(), "minio-")
+	if err != nil {
+		t.Fatalf("Initializing temporary directory failed with %s.", err)
+	}
+	testCases := []struct {
+		volName       string
+		expectedUmask int
+	}{
+		{"is-this-valid", getUmask()},
+	}
+	testCase := testCases[0]
+
+	// Initialize a new posix layer.
+	disk, err := newPosix(tmpPath)
+	if err != nil {
+		t.Fatalf("Initializing posix failed with %s.", err)
+	}
+
+	// Attempt to create a volume to verify the permissions later.
+	// MakeVol creates 0777.
+	if err = disk.MakeVol(testCase.volName); err != nil {
+		t.Fatalf("Creating a volume failed with %s expected to pass.", err)
+	}
+	defer removeAll(tmpPath)
+
+	// Stat to get permissions bits.
+	st, err := os.Stat(path.Join(tmpPath, testCase.volName))
+	if err != nil {
+		t.Fatalf("Stat failed with %s expected to pass.", err)
+	}
+
+	// Get umask of the bits stored.
+	currentUmask := 0777 - uint32(st.Mode().Perm())
+
+	// Verify if umask is correct.
+	if int(currentUmask) != testCase.expectedUmask {
+		t.Fatalf("Umask check failed expected %d, got %d", testCase.expectedUmask, currentUmask)
+	}
+}
+
+// Tests if the file creations happen with proper umask.
+func TestIsValidUmaskFile(t *testing.T) {
+	tmpPath, err := ioutil.TempDir(os.TempDir(), "minio-")
+	if err != nil {
+		t.Fatalf("Initializing temporary directory failed with %s.", err)
+	}
+	testCases := []struct {
+		volName       string
+		expectedUmask int
+	}{
+		{"is-this-valid", getUmask()},
+	}
+	testCase := testCases[0]
+
+	// Initialize a new posix layer.
+	disk, err := newPosix(tmpPath)
+	if err != nil {
+		t.Fatalf("Initializing posix failed with %s.", err)
+	}
+
+	// Attempt to create a volume to verify the permissions later.
+	// MakeVol creates directory with 0777 perms.
+	if err = disk.MakeVol(testCase.volName); err != nil {
+		t.Fatalf("Creating a volume failed with %s expected to pass.", err)
+	}
+
+	defer removeAll(tmpPath)
+
+	// Attempt to create a file to verify the permissions later.
+	// AppendFile creates file with 0666 perms.
+	if err = disk.AppendFile(testCase.volName, "hello-world.txt", []byte("Hello World")); err != nil {
+		t.Fatalf("Create a file `test` failed with %s expected to pass.", err)
+	}
+
+	// StatFile - stat the file.
+	fi, err := disk.StatFile(testCase.volName, "hello-world.txt")
+	if err != nil {
+		t.Fatalf("Stat failed with %s expected to pass.", err)
+	}
+
+	// Get umask of the bits stored.
+	currentUmask := 0666 - uint32(fi.Mode.Perm())
+
+	// Verify if umask is correct.
+	if int(currentUmask) != testCase.expectedUmask {
+		t.Fatalf("Umask check failed expected %d, got %d", testCase.expectedUmask, currentUmask)
+	}
+}

posix-utils_test.go

@@ -0,0 +1,73 @@
+/*
+ * Minio Cloud Storage, (C) 2016 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 main
+
+import "testing"
+
+// Tests validate volume name.
+func TestIsValidVolname(t *testing.T) {
+	testCases := []struct {
+		volName    string
+		shouldPass bool
+	}{
+		// Cases which should pass the test.
+		// passing in valid bucket names.
+		{"lol", true},
+		{"1-this-is-valid", true},
+		{"1-this-too-is-valid-1", true},
+		{"this.works.too.1", true},
+		{"1234567", true},
+		{"123", true},
+		{"s3-eu-west-1.amazonaws.com", true},
+		{"ideas-are-more-powerful-than-guns", true},
+		{"testbucket", true},
+		{"1bucket", true},
+		{"bucket1", true},
+		{"$this-is-not-valid-too", true},
+		{"contains-$-dollar", true},
+		{"contains-^-carrot", true},
+		{"contains-$-dollar", true},
+		{"contains-$-dollar", true},
+		{".starts-with-a-dot", true},
+		{"ends-with-a-dot.", true},
+		{"ends-with-a-dash-", true},
+		{"-starts-with-a-dash", true},
+		{"THIS-BEINGS-WITH-UPPERCASe", true},
+		{"tHIS-ENDS-WITH-UPPERCASE", true},
+		{"ThisBeginsAndEndsWithUpperCase", true},
+		{"una ñina", true},
+		// cases for which test should fail.
+		// passing invalid bucket names.
+		{"", false},
+		{"/", false},
+		{"a", false},
+		{"ab", false},
+		{"ab/", false},
+		{"......", true},
+		{"lalalallalallalalalallalallalala-theString-size-is-greater-than-64", false},
+	}
+
+	for i, testCase := range testCases {
+		isValidVolname := isValidVolname(testCase.volName)
+		if testCase.shouldPass && !isValidVolname {
+			t.Errorf("Test case %d: Expected \"%s\" to be a valid bucket name", i+1, testCase.volName)
+		}
+		if !testCase.shouldPass && isValidVolname {
+			t.Errorf("Test case %d: Expected bucket name \"%s\" to be invalid", i+1, testCase.volName)
+		}
+	}
+}

posix-utils_windows_test.go

@@ -108,7 +108,7 @@ func TestUNCPathDiskName(t *testing.T) {
 	var err error
 	// Instantiate posix object to manage a disk
 	longPathDisk := `\\?\c:\testdisk`
-	err = mkdirAll(longPathDisk, 0700)
+	err = mkdirAll(longPathDisk, 0777)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -147,7 +147,7 @@ func Test32kUNCPath(t *testing.T) {
 		remaining := 32767 - 25 - len(longDiskName)
 		longDiskName = longDiskName + `\` + strings.Repeat("a", remaining)
 	}
-	err = mkdirAll(longDiskName, 0700)
+	err = mkdirAll(longDiskName, 0777)
 	if err != nil {
 		t.Fatal(err)
 	}

posix.go

@@ -204,8 +204,8 @@ func (s *posix) MakeVol(volume string) (err error) {
 	if err != nil {
 		return err
 	}
-	// Make a volume entry.
-	err = os.Mkdir(preparePath(volumeDir), 0700)
+	// Make a volume entry, with mode 0777 mkdir honors system umask.
+	err = os.Mkdir(preparePath(volumeDir), 0777)
 	if err != nil && os.IsExist(err) {
 		return errVolumeExists
 	}
@@ -472,10 +472,14 @@ func (s *posix) AppendFile(volume, path string, buf []byte) (err error) {
 		}
 	}
 	// Create top level directories if they don't exist.
-	if err = mkdirAll(filepath.Dir(filePath), 0700); err != nil {
+	// with mode 0777 mkdir honors system umask.
+	if err = mkdirAll(filepath.Dir(filePath), 0777); err != nil {
 		return err
 	}
-	w, err := os.OpenFile(preparePath(filePath), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600)
+
+	// Creates the named file with mode 0666 (before umask), or starts appending
+	// to an existig file.
+	w, err := os.OpenFile(preparePath(filePath), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0666)
 	if err != nil {
 		// File path cannot be verified since one of the parents is a file.
 		if strings.Contains(err.Error(), "not a directory") {
@@ -483,6 +487,7 @@ func (s *posix) AppendFile(volume, path string, buf []byte) (err error) {
 		}
 		return err
 	}
+
 	// Close upon return.
 	defer w.Close()
 
@@ -689,7 +694,8 @@ func (s *posix) RenameFile(srcVolume, srcPath, dstVolume, dstPath string) (err e
 		}
 		// Destination does not exist, hence proceed with the rename.
 	}
-	if err = mkdirAll(preparePath(slashpath.Dir(dstFilePath)), 0755); err != nil {
+	// Creates all the parent directories, with mode 0777 mkdir honors system umask.
+	if err = mkdirAll(preparePath(slashpath.Dir(dstFilePath)), 0777); err != nil {
 		// File path cannot be verified since one of the parents is a file.
 		if strings.Contains(err.Error(), "not a directory") {
 			return errFileAccessDenied
@@ -701,6 +707,7 @@ func (s *posix) RenameFile(srcVolume, srcPath, dstVolume, dstPath string) (err e
 		}
 		return err
 	}
+	// Finally attempt a rename.
 	err = os.Rename(preparePath(srcFilePath), preparePath(dstFilePath))
 	if err != nil {
 		if os.IsNotExist(err) {