File permissions on config files allow more restrictive setting (#966)

* File permissions on config files allow more restrictive setting

* Make the tests a bit more readable

* Bring back the tests dude

* Update error message

* remove else

* Add more test cases

* Change Lstat to Stat

* Add note for umask

* Make sure the permissions are 0600 or lower

* Update config file

* Do not check for windows

* Fix CI errors

* Fix CI test

cmd/proxy/actions/actions_test.go

@@ -1,23 +1,28 @@
 package actions
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/gobuffalo/suite"
 	"github.com/gomods/athens/pkg/config"
 )
 
-var (
+func testConfigFile(t *testing.T) (testConfigFile string) {
 	testConfigFile = filepath.Join("..", "..", "..", "config.dev.toml")
-)
+	if err := os.Chmod(testConfigFile, 0700); err != nil {
+		t.Fatalf("%s\n", err)
+	}
+	return testConfigFile
+}
 
 type ActionSuite struct {
 	*suite.Action
 }
 
 func Test_ActionSuite(t *testing.T) {
-	conf, err := config.GetConf(testConfigFile)
+	conf, err := config.GetConf(testConfigFile(t))
 	if err != nil {
 		t.Fatalf("Unable to parse config file: %s", err.Error())
 	}

pkg/config/config.go

@@ -89,7 +89,7 @@ func ParseConfigFile(configFile string) (*Config, error) {
 	}
 
 	// Check file perms from config
-	if err := checkFilePerms(config.FilterFile); err != nil {
+	if err := checkFilePerms(configFile, config.FilterFile); err != nil {
 		return nil, err
 	}
 
@@ -162,21 +162,18 @@ func checkFilePerms(files ...string) error {
 		// TODO: Do not ignore errors when a file is not found
 		// There is a subtle bug in the filter module which ignores the filter file if it does not find it.
 		// This check can be removed once that has been fixed
-		fInfo, err := os.Lstat(f)
+		fInfo, err := os.Stat(f)
 		if err != nil {
 			continue
 		}
 
-		if runtime.GOOS == "windows" {
-			if (fInfo.Mode() & 0600) != 0600 {
-				return errors.E(op, f+" should have 0600 as permission")
-			}
-		} else {
-			// Assume unix based system (MacOS and Linux)
-			if fInfo.Mode() != 0640 {
-				return errors.E(op, f+" should have 0640 as permission")
-			}
+		// Assume unix based system (MacOS and Linux)
+		// the bit mask is calculated using the umask command which tells which permissions
+		// should not be allowed for a particular user, group or world
+		if fInfo.Mode()&0077 != 0 && runtime.GOOS != "windows" {
+			return errors.E(op, f+" should have at most rwx,-, - (bit mask 077) as permission")
 		}
+
 	}
 
 	return nil

pkg/config/config_test.go

@@ -12,7 +12,13 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 )
 
-const exampleConfigPath = "../../config.dev.toml"
+func testConfigFile(t *testing.T) (testConfigFile string) {
+	testConfigFile = filepath.Join("..", "..", "config.dev.toml")
+	if err := os.Chmod(testConfigFile, 0700); err != nil {
+		t.Fatalf("%s\n", err)
+	}
+	return testConfigFile
+}
 
 func compareConfigs(parsedConf *Config, expConf *Config, t *testing.T) {
 	opts := cmpopts.IgnoreTypes(StorageConfig{})
@@ -218,7 +224,7 @@ func TestParseExampleConfig(t *testing.T) {
 		TraceExporter:    "jaeger",
 	}
 
-	absPath, err := filepath.Abs(exampleConfigPath)
+	absPath, err := filepath.Abs(testConfigFile(t))
 	if err != nil {
 		t.Errorf("Unable to construct absolute path to example config file")
 	}
@@ -298,64 +304,86 @@ func restoreEnv(envVars map[string]string) {
 	}
 }
 
-func Test_checkFilePerms(t *testing.T) {
+func tempFile(perm os.FileMode) (name string, err error) {
+	f, err := ioutil.TempFile(os.TempDir(), "prefix-")
+	if err != nil {
+		return "", err
+	}
+	if err = os.Chmod(f.Name(), perm); err != nil {
+		os.Remove(f.Name())
+		return "", err
+	}
+	return f.Name(), nil
+}
 
+func Test_checkFilePerms(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skipf("Chmod is not supported in windows, so not possible to test. Ref: https://github.com/golang/go/blob/master/src/os/os_test.go#L1031\n")
 	}
 
-	f1, err := ioutil.TempFile(os.TempDir(), "prefix-")
-	if err != nil {
-		t.Fatalf("Cannot create 1st temp file: %s", err)
-	}
-	defer os.Remove(f1.Name())
-	if err = os.Chmod(f1.Name(), 0700); err != nil {
-		t.Fatalf("Cannot chmod 1st temp file: %s", err)
-	}
+	incorrectPerms := []os.FileMode{0777, 0610, 0660}
+	var incorrectFiles = make([]string, len(incorrectPerms))
 
-	f2, err := ioutil.TempFile(os.TempDir(), "prefix-")
-	if err != nil {
-		t.Fatalf("Cannot create 2nd temp file: %s", err)
+	for i := range incorrectPerms {
+		f, err := tempFile(incorrectPerms[i])
+		if err != nil {
+			t.Fatalf("tempFile creation error %s", err)
+		}
+		incorrectFiles[i] = f
+		defer os.Remove(f)
 	}
 
-	defer os.Remove(f2.Name())
-	if err = os.Chmod(f2.Name(), 0640); err != nil {
-		t.Fatalf("Cannot chmod 2nd temp file: %s", err)
-	}
+	correctPerms := []os.FileMode{0600, 0400}
+	var correctFiles = make([]string, len(correctPerms))
 
-	type args struct {
-		files []string
+	for i := range correctPerms {
+		f, err := tempFile(correctPerms[i])
+		if err != nil {
+			t.Fatalf("tempFile creation error %s", err)
+		}
+		correctFiles[i] = f
+		defer os.Remove(f)
 	}
-	tests := []struct {
+
+	type test struct {
 		name    string
-		args    args
+		files   []string
 		wantErr bool
-	}{
+	}
+
+	tests := []test{
+		{
+			"should not have an error on 0600, 0400, 0640",
+			[]string{correctFiles[0], correctFiles[1]},
+			false,
+		},
 		{
 			"should not have an error on empty file name",
-			args{
-				[]string{"", f2.Name()},
-			},
+			[]string{"", correctFiles[1]},
 			false,
 		},
 		{
 			"should have an error if all the files have incorrect permissions",
-			args{
-				[]string{f1.Name(), f1.Name(), f1.Name()},
-			},
+			[]string{incorrectFiles[0], incorrectFiles[1], incorrectFiles[1]},
 			true,
 		},
 		{
 			"should have an error when at least 1 file has wrong permissions",
-			args{
-				[]string{f2.Name(), f1.Name()},
-			},
+			[]string{correctFiles[0], correctFiles[1], incorrectFiles[1]},
 			true,
 		},
 	}
+
+	for _, f := range incorrectFiles {
+		tests = append(tests, test{
+			"incorrect file permission passed",
+			[]string{f},
+			true,
+		})
+	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := checkFilePerms(tt.args.files...); (err != nil) != tt.wantErr {
+			if err := checkFilePerms(tt.files...); (err != nil) != tt.wantErr {
 				t.Errorf("checkFilePerms() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

pkg/middleware/middleware_test.go

@@ -22,9 +22,13 @@ const (
 	pathVersionInfo = "/{module:.+}/@v/{version}.info"
 )
 
-var (
+func testConfigFile(t *testing.T) (testConfigFile string) {
 	testConfigFile = filepath.Join("..", "..", "config.dev.toml")
-)
+	if err := os.Chmod(testConfigFile, 0700); err != nil {
+		t.Fatalf("%s\n", err)
+	}
+	return testConfigFile
+}
 
 func middlewareFilterApp(filterFile, registryEndpoint string) (*buffalo.App, error) {
 	h := func(c buffalo.Context) error {
@@ -63,7 +67,7 @@ func Test_FilterMiddleware(t *testing.T) {
 	}
 	defer os.Remove(filter.Name())
 
-	conf, err := config.GetConf(testConfigFile)
+	conf, err := config.GetConf(testConfigFile(t))
 	if err != nil {
 		t.Fatalf("Unable to parse config file: %s", err.Error())
 	}

pkg/module/filter_test.go

@@ -9,9 +9,13 @@ import (
 	"github.com/stretchr/testify/suite"
 )
 
-var (
+func testConfigFile(t *testing.T) (testConfigFile string) {
 	testConfigFile = filepath.Join("..", "..", "config.dev.toml")
-)
+	if err := os.Chmod(testConfigFile, 0700); err != nil {
+		t.Fatalf("%s\n", err)
+	}
+	return testConfigFile
+}
 
 type FilterTests struct {
 	suite.Suite