From 5eba6f2e47728ddeebef4387dd909df9dfdbc484 Mon Sep 17 00:00:00 2001 From: Manu Gupta Date: Tue, 18 Dec 2018 07:26:04 -0800 Subject: [PATCH] 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 | 11 +++- config.dev.toml | 0 pkg/config/config.go | 19 +++--- pkg/config/config_test.go | 96 ++++++++++++++++++++----------- pkg/middleware/middleware_test.go | 10 +++- pkg/module/filter_test.go | 8 ++- 6 files changed, 91 insertions(+), 53 deletions(-) mode change 100644 => 100755 config.dev.toml diff --git a/cmd/proxy/actions/actions_test.go b/cmd/proxy/actions/actions_test.go index 22731252..53354408 100644 --- a/cmd/proxy/actions/actions_test.go +++ b/cmd/proxy/actions/actions_test.go @@ -1,6 +1,7 @@ package actions import ( + "os" "path/filepath" "testing" @@ -8,16 +9,20 @@ import ( "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()) } diff --git a/config.dev.toml b/config.dev.toml old mode 100644 new mode 100755 diff --git a/pkg/config/config.go b/pkg/config/config.go index 9af31886..63e295b3 100644 --- a/pkg/config/config.go +++ b/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 diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 52f25342..06d0aabc 100644 --- a/pkg/config/config_test.go +++ b/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)) + + 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) } - f2, err := ioutil.TempFile(os.TempDir(), "prefix-") - if err != nil { - t.Fatalf("Cannot create 2nd temp file: %s", err) + correctPerms := []os.FileMode{0600, 0400} + var correctFiles = make([]string, len(correctPerms)) + + 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) } - defer os.Remove(f2.Name()) - if err = os.Chmod(f2.Name(), 0640); err != nil { - t.Fatalf("Cannot chmod 2nd temp file: %s", err) - } - - type args struct { - files []string - } - 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) } }) diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index b058802c..faa2786b 100644 --- a/pkg/middleware/middleware_test.go +++ b/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()) } diff --git a/pkg/module/filter_test.go b/pkg/module/filter_test.go index e9f1d3cb..1deadcb9 100644 --- a/pkg/module/filter_test.go +++ b/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