fix: HOME with different permissions (#2361)

* fixed integration tests Signed-off-by: gauron99 <[email protected]> * docker config.json credentials test when HOME not defined Signed-off-by: gauron99 <[email protected]> * pack test Signed-off-by: gauron99 <[email protected]> * simplify Signed-off-by: gauron99 <[email protected]> * og creds, small fixes Signed-off-by: gauron99 <[email protected]> * s2i test no home Signed-off-by: gauron99 <[email protected]> * remove unnecessary stuff Signed-off-by: gauron99 <[email protected]> * deploy test without home Signed-off-by: gauron99 <[email protected]> * confict fix after rebase Signed-off-by: gauron99 <[email protected]> * move test, dont delete Signed-off-by: gauron99 <[email protected]> * runtime change Signed-off-by: gauron99 <[email protected]> * node image signals fixed and smaller size for GH actions Signed-off-by: gauron99 <[email protected]> * return err Signed-off-by: gauron99 <[email protected]> * clean up comments Signed-off-by: gauron99 <[email protected]> * creds and test Signed-off-by: gauron99 <[email protected]> * test return commented code Signed-off-by: gauron99 <[email protected]> * config warning Signed-off-by: gauron99 <[email protected]> * cleanup Signed-off-by: gauron99 <[email protected]> * cleanup Signed-off-by: gauron99 <[email protected]> * fix Signed-off-by: gauron99 <[email protected]> * fix Signed-off-by: gauron99 <[email protected]> * skip test for windows Signed-off-by: gauron99 <[email protected]> * skip for prow Signed-off-by: David Fridrich <[email protected]> * fix repo on create, move warning up a function Signed-off-by: David Fridrich <[email protected]> * fix print Signed-off-by: David Fridrich <[email protected]> --------- Signed-off-by: gauron99 <[email protected]> Signed-off-by: David Fridrich <[email protected]>
knative · Sep 9, 2024 · 8dce9bc · 8dce9bc
1 parent dacb3ee
commit 8dce9bc
Show file tree

Hide file tree

Showing 4 changed files with 227 additions and 20 deletions.
diff --git a/cmd/root.go b/cmd/root.go
@@ -64,6 +64,11 @@ Learn more about Knative at: https://knative.dev`, cfg.Name),
 	viper.SetEnvPrefix("func") // ensure that all have the prefix
 	viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
 
+	// check if permissions for FUNC HOME are sufficient; warn if otherwise
+	cp := config.File()
+	if _, err := os.ReadFile(cp); os.IsPermission(err) {
+		fmt.Fprintf(os.Stderr, "Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp)
+	}
 	// Client
 	// Use the provided ClientFactory or default to NewClient
 	newClient := cfg.NewClient

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -81,8 +81,11 @@ func NewDefault() (cfg Global, err error) {
 	cp := File()
 	bb, err := os.ReadFile(cp)
 	if err != nil {
-		if os.IsNotExist(err) {
-			err = nil // config file is not required
+		// config file is not required
+		// permissions warning printed in cmd/root.go as to not spam here
+		// TODO: gauron99 - review the whole process for simplification
+		if os.IsNotExist(err) || os.IsPermission(err) {
+			err = nil
 		}
 		return
 	}

diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go
@@ -549,7 +549,8 @@ func TestCredentialsProviderSavingFromUserInput(t *testing.T) {
 	}
 }
 
-// TestCredentialsWithoutHome tests different scenarios when HOME is not set
+// TestCredentialsWithoutHome ensures that credentialProvider works when HOME is
+// not set or config is empty
 func TestCredentialsWithoutHome(t *testing.T) {
 	type args struct {
 		promptUser        creds.CredentialsCallback
@@ -585,30 +586,31 @@ func TestCredentialsWithoutHome(t *testing.T) {
 		},
 	}
 
+	// reset HOME to the original value after tests since they may change it
+	defer func() {
+		os.Setenv("HOME", homeTempDir)
+	}()
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			resetHomeDir(t)
-			if tt.args.setUpEnv != nil {
-				tt.args.setUpEnv(t)
-			}
-
-			// prepare config path for credentials provider
-			var configPath string
+			// set up HOME
 			if tt.testHomePathEmpty {
-				configPath = ""
+				os.Unsetenv("HOME")
 			} else {
-				configPath = testConfigPath(t)
+				os.Setenv("HOME", homeTempDir)
 			}
-
 			credentialsProvider := creds.NewCredentialsProvider(
-				configPath,
+				testConfigPath(t),
 				creds.WithPromptForCredentials(tt.args.promptUser),
 				creds.WithVerifyCredentials(tt.args.verifyCredentials),
 			)
 
 			got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag")
+
+			// ASSERT
 			if err != nil {
-				t.Errorf("unexpected error: %v", err)
+				t.Errorf("%v", err)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
@@ -618,6 +620,166 @@ func TestCredentialsWithoutHome(t *testing.T) {
 	}
 }
 
+// TestCredentialsHomePermissions tests whether the credentials provider
+// works in scenarios where HOME has different permissions
+func TestCredentialsHomePermissions(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("skip windows perms for this test until windows perms are added")
+	}
+
+	if os.Getenv("GITHUB_ACTION") == "" {
+		// skip for prow because its running as root
+		t.Skip()
+	}
+
+	type args struct {
+		promptUser        creds.CredentialsCallback
+		verifyCredentials creds.VerifyCredentialsCallback
+		registry          string
+		setUpEnv          setUpEnv
+	}
+	tests := []struct {
+		name              string
+		perms             os.FileMode
+		args              args
+		expPermsDeniedErr bool
+		want              Credentials
+	}{
+		{
+			name:  "home with 0000 permissions (no perms)",
+			perms: 0000,
+			args: args{
+				promptUser: pwdCbkThatShallNotBeCalled(t),
+				setUpEnv:   setHomeWithPermissions(0000),
+			},
+			expPermsDeniedErr: true,
+		},
+		{
+			name:  "home with 0333 permissions (write-execute only)",
+			perms: 0333,
+			args: args{
+
+				promptUser:        pwdCbkThatShallNotBeCalled(t),
+				verifyCredentials: correctVerifyCbk,
+				registry:          "docker.io",
+				setUpEnv: all(
+					withPopulatedDockerAuthConfig,
+					setUpMockHelper("docker-credential-mock", newInMemoryHelper()),
+					setHomeWithPermissions(0333)),
+			},
+
+			expPermsDeniedErr: false,
+			want:              Credentials{Username: dockerIoUser, Password: dockerIoUserPwd},
+		},
+		{
+			name:  "home with 0444 permissions (read-only)",
+			perms: 0444,
+			args: args{
+				promptUser: pwdCbkThatShallNotBeCalled(t),
+				setUpEnv:   setHomeWithPermissions(0444)},
+			expPermsDeniedErr: true,
+		},
+		{
+			name:  "home with 0555 permissions (read-execute-only)",
+			perms: 0555,
+			args: args{
+				promptUser: pwdCbkThatShallNotBeCalled(t),
+				setUpEnv:   setHomeWithPermissions(0555),
+			},
+			expPermsDeniedErr: true,
+		},
+		{
+			name:  "home with 0666 permissions (read-write-execute)",
+			perms: 0666,
+			args: args{
+				promptUser: pwdCbkThatShallNotBeCalled(t),
+				setUpEnv:   setHomeWithPermissions(0666),
+			},
+			expPermsDeniedErr: true,
+		},
+		{
+			name:  "home with 0777 permissions (full access)",
+			perms: 0777,
+
+			args: args{
+				promptUser:        pwdCbkThatShallNotBeCalled(t),
+				verifyCredentials: correctVerifyCbk,
+				registry:          "docker.io",
+				setUpEnv: all(
+					withPopulatedDockerAuthConfig,
+					setUpMockHelper("docker-credential-mock", newInMemoryHelper()),
+					setHomeWithPermissions(0777),
+				),
+			},
+			expPermsDeniedErr: false,
+			want:              Credentials{Username: dockerIoUser, Password: dockerIoUserPwd},
+		},
+	}
+
+	// return HOME dir into its original state
+	defer func() {
+		resetHomePermissions(t) //reset home permissions to 0700
+	}()
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			resetHomePermissions(t) //needs to be reset so that dir can be removed
+			resetHomeDir(t)
+
+			if tt.args.setUpEnv != nil {
+				tt.args.setUpEnv(t)
+			}
+
+			// try to create HOME/.config/func
+			_, err := testConfigPathError(t)
+			if err != nil { // If error was returned
+				if os.IsPermission(err) { // and its a permission error
+					if !tt.expPermsDeniedErr { // but it wasnt expected
+						t.Fatalf("didnt expect permissions denied error, but got: %s", err)
+					}
+
+				} else { // and it wasnt permission error
+					t.Fatalf("got unexpected error: %v", err)
+				}
+			} else { // Else no error was returned
+				if tt.expPermsDeniedErr { // but it was expected
+					t.Fatal("expected permissions denied error, but got none")
+				}
+			}
+
+			// if permissions were not denied, try to create Provider
+			if !tt.expPermsDeniedErr {
+
+				// try to stat HOME for permissions
+				info, err := os.Stat(os.Getenv(testHomeEnvName()))
+				if err != nil {
+					t.Fatalf("failed to stat HOME: %s", err)
+				}
+
+				if info.Mode().Perm() != tt.perms {
+					t.Errorf("expected permissions '%v', got '%v'", tt.perms, info.Mode().Perm())
+				}
+				credentialsProvider := creds.NewCredentialsProvider(
+					testConfigPath(t),
+					creds.WithPromptForCredentials(tt.args.promptUser),
+					creds.WithVerifyCredentials(tt.args.verifyCredentials),
+				)
+
+				got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag")
+				if err != nil {
+					t.Errorf("%v", err)
+					return
+				}
+				if !reflect.DeepEqual(got, tt.want) {
+					t.Errorf("got: %v, want: %v", got, tt.want)
+				}
+
+			}
+		})
+	}
+}
+
 // ********************** helper functions below **************************** \\
 
 func resetHomeDir(t *testing.T) {
@@ -630,6 +792,13 @@ func resetHomeDir(t *testing.T) {
 	}
 }
 
+// resetHomePermissions resets the HOME perms to 0700 (same as resetHomeDir(t))
+func resetHomePermissions(t *testing.T) {
+	if err := os.Chmod(homeTempDir, 0700); err != nil {
+		t.Fatal(err)
+	}
+}
+
 type setUpEnv = func(t *testing.T)
 
 func withPopulatedDockerAuthConfig(t *testing.T) {
@@ -743,13 +912,25 @@ func testHomeEnvName() string {
 func testConfigPath(t *testing.T) string {
 	t.Helper()
 	home := os.Getenv(testHomeEnvName())
-	configPath := filepath.Join(home, ".config", "func")
-	if err := os.MkdirAll(configPath, os.ModePerm); err != nil {
-		t.Fatal(err)
+	var configPath string
+	if home != "" { // if HOME is not set, don't create config dir
+		configPath = filepath.Join(home, ".config", "func")
+		if err := os.MkdirAll(configPath, os.ModePerm); err != nil {
+			t.Fatal(err)
+		}
 	}
 	return configPath
 }
 
+// testConfigPathError tries to create a config dir in HOME/.config/func.
+// Compared to testConfigPath, this returns the path AND error instead of failing
+func testConfigPathError(t *testing.T) (string, error) {
+	t.Helper()
+	home := os.Getenv(testHomeEnvName())
+	configPath := filepath.Join(home, ".config", "func")
+	return configPath, os.MkdirAll(configPath, os.ModePerm)
+}
+
 func handlerForCredHelper(t *testing.T, credHelper credentials.Helper) http.Handler {
 	return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
 		defer request.Body.Close()
@@ -981,3 +1162,22 @@ func setEmptyHome(t *testing.T) {
 	t.Setenv("HOME", "")
 	t.Setenv("XDG_CONFIG_HOME", "")
 }
+
+// setHomeWithPermissions sets home dir to specified permissions
+func setHomeWithPermissions(perm os.FileMode) func(t *testing.T) {
+	return func(t *testing.T) {
+		t.Helper()
+		homeDir := os.Getenv("HOME")
+
+		// if home is empty, nothing to do
+		if homeDir == "" {
+			t.Fatal("home dir is empty, cant set perms")
+		}
+
+		fmt.Printf("setting permissions (%v) on home dir: %s\n", perm, homeDir)
+		err := os.Chmod(homeDir, perm)
+		if err != nil {
+			t.Fatalf("failed to set permissions on home dir: %s", err)
+		}
+	}
+}
diff --git a/pkg/functions/repositories.go b/pkg/functions/repositories.go
@@ -100,9 +100,8 @@ func (r *Repositories) All() (repos []Repository, err error) {
 	if r.path == "" {
 		return
 	}
-
-	// Return empty if path does not exist
-	if _, err = os.Stat(r.path); os.IsNotExist(err) {
+	// Return empty if path does not exist or insufficient permissions
+	if _, err = os.Stat(r.path); os.IsNotExist(err) || os.IsPermission(err) {
 		return repos, nil
 	}