Skip to content

Commit

Permalink
Removes check whether plugin is executable
Browse files Browse the repository at this point in the history 
 Fixes #365
 The code used for checking whether plugin is executable or not in plugin verifier
 does not build for Windows.

 Removing the executable check to unblock Windows builds, we could add the check back
 which is portable.
  • Loading branch information
@navidshaikh
navidshaikh committed Aug 15, 2019
1 parent c4e8d5a commit ebf9e87
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 155 deletions.
14 changes: 0 additions & 14 deletions pkg/kn/commands/plugin/plugin_list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,20 +144,6 @@ func TestPluginList(t *testing.T) {
})
})

t.Run("with non-executable plugin", func(t *testing.T) {
t.Run("warns user plugin invalid", func(t *testing.T) {
ctx := setup(t)
defer ctx.cleanup()

err := ctx.createTestPlugin(KnTestPluginName, FileModeReadable, false)
assert.NilError(t, err)

err = ctx.execute("plugin", "list", "--lookup-plugins-in-path=false")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(ctx.output(), "WARNING", "not executable", "current user"))
})
})

t.Run("with plugins with same name", func(t *testing.T) {

t.Run("warns user about second (in $PATH) plugin shadowing first", func(t *testing.T) {
Expand Down
96 changes: 1 addition & 95 deletions pkg/kn/commands/plugin/plugin_verifier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ import (
"io"
"os"
"path/filepath"
"runtime"
"strings"
"syscall"

"github.com/pkg/errors"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -64,7 +62,7 @@ func (v *pluginVerifier) verify(eaw errorsAndWarnings, path string) errorsAndWar
}

// Verify that plugin actually exists
fileInfo, err := os.Stat(path)
_, err := os.Stat(path)
if err != nil {
if err == os.ErrNotExist {
return eaw.addError("cannot find plugin in %s", path)
Expand All @@ -73,7 +71,6 @@ func (v *pluginVerifier) verify(eaw errorsAndWarnings, path string) errorsAndWar
}

eaw = v.addErrorIfWrongPrefix(eaw, path)
eaw = v.addWarningIfNotExecutable(eaw, path, fileInfo)
eaw = v.addWarningIfAlreadySeen(eaw, path)
eaw = v.addErrorIfOverwritingExistingCommand(eaw, path)

Expand Down Expand Up @@ -124,58 +121,6 @@ func (v *pluginVerifier) addErrorIfWrongPrefix(eaw errorsAndWarnings, path strin
return eaw
}

func (v *pluginVerifier) addWarningIfNotExecutable(eaw errorsAndWarnings, path string, fileInfo os.FileInfo) errorsAndWarnings {
if runtime.GOOS == "windows" {
return checkForWindowsExecutable(eaw, fileInfo, path)
}

mode := fileInfo.Mode()
if !mode.IsRegular() && !isSymlink(mode) {
return eaw.addWarning("%s is not a file", path)
}
perms := uint32(mode.Perm())

var sys *syscall.Stat_t
var ok bool
if sys, ok = fileInfo.Sys().(*syscall.Stat_t); !ok {
// We can check the files' owner/group
return eaw.addWarning("cannot check owner/group of file %s", path)
}

isOwner := checkIfUserIsFileOwner(sys.Uid)
isInGroup, err := checkIfUserInGroup(sys.Gid)
if err != nil {
return eaw.addError("cannot get group ids for checking executable status of file %s", path)
}

// User is owner and owner can execute
if canOwnerExecute(perms, isOwner) {
return eaw
}

// User is in group which can execute, but user is not file owner
if canGroupExecute(perms, isOwner, isInGroup) {
return eaw
}

// All can execute, and the user is not file owner and not in the file's perm group
if canOtherExecute(perms, isOwner, isInGroup) {
return eaw
}

return eaw.addWarning("%s is not executable by current user", path)
}

func checkForWindowsExecutable(eaw errorsAndWarnings, fileInfo os.FileInfo, path string) errorsAndWarnings {
fileExt := strings.ToLower(filepath.Ext(fileInfo.Name()))

switch fileExt {
case ".bat", ".cmd", ".com", ".exe", ".ps1":
return eaw
}
return eaw.addWarning("%s is not executable as it does not have the proper extension", path)
}

func checkIfUserInGroup(gid uint32) (bool, error) {
groups, err := os.Getgroups()
if err != nil {
Expand All @@ -196,45 +141,6 @@ func checkIfUserIsFileOwner(uid uint32) bool {
return false
}

// Check if all can execute, and the user is not file owner and not in the file's perm group
func canOtherExecute(perms uint32, isOwner bool, isInGroup bool) bool {
if perms&OtherExecute != 0 {
if os.Getuid() == 0 {
return true
}
if !isOwner && !isInGroup {
return true
}
}
return false
}

// Check if user is owner and owner can execute
func canOwnerExecute(perms uint32, isOwner bool) bool {
if perms&UserExecute != 0 {
if os.Getuid() == 0 {
return true
}
if isOwner {
return true
}
}
return false
}

// Check if user is in group which can execute, but user is not file owner
func canGroupExecute(perms uint32, isOwner bool, isInGroup bool) bool {
if perms&GroupExecute != 0 {
if os.Getuid() == 0 {
return true
}
if !isOwner && isInGroup {
return true
}
}
return false
}

func (eaw *errorsAndWarnings) addError(format string, args ...interface{}) errorsAndWarnings {
eaw.errors = append(eaw.errors, fmt.Sprintf(format, args...))
return *eaw
Expand Down
46 changes: 0 additions & 46 deletions pkg/kn/commands/plugin/plugin_verifier_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,9 @@ package plugin

import (
"errors"
"io/ioutil"
"os"
"os/exec"
"os/user"
"path/filepath"
"runtime"
"strconv"
"testing"

Expand Down Expand Up @@ -66,49 +63,6 @@ func TestPluginVerifier(t *testing.T) {
})

t.Run("with root command", func(t *testing.T) {
t.Run("whether plugin in path is executable (unix only)", func(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Skip test for windows as the permission check are for Unix only")
return
}

setup(t)
defer cleanup(t)

pluginDir, err := ioutil.TempDir("", "plugin")
assert.NilError(t, err)
defer os.RemoveAll(pluginDir)
pluginPath := filepath.Join(pluginDir, "kn-execution-test")
err = ioutil.WriteFile(pluginPath, []byte("#!/bin/sh\ntrue"), 0644)
assert.NilError(t, err, "can't create test plugin")

for _, uid := range getExecTestUids() {
for _, gid := range getExecTestGids() {
for _, userPerm := range []int{0, UserExecute} {
for _, groupPerm := range []int{0, GroupExecute} {
for _, otherPerm := range []int{0, OtherExecute} {
perm := os.FileMode(userPerm | groupPerm | otherPerm + 0444)
assert.NilError(t, prepareFile(pluginPath, uid, gid, perm), "prepare plugin file, uid: %d, gid: %d, perm: %03o", uid, gid, perm)

eaw := errorsAndWarnings{}
eaw = newPluginVerifier(rootCmd).verify(eaw, pluginPath)

if isExecutable(pluginPath) {
assert.Assert(t, len(eaw.warnings) == 0, "executable: perm %03o | uid %d | gid %d | %v", perm, uid, gid, eaw.warnings)
assert.Assert(t, len(eaw.errors) == 0)
} else {
assert.Assert(t, len(eaw.warnings) == 1, "not executable: perm %03o | uid %d | gid %d | %v", perm, uid, gid, eaw.warnings)
assert.Assert(t, len(eaw.errors) == 0)
assert.Assert(t, util.ContainsAll(eaw.warnings[0], pluginPath))
}

}
}
}
}
}
})

t.Run("when kn plugin in path is executable", func(t *testing.T) {
setup(t)
defer cleanup(t)
Expand Down

0 comments on commit ebf9e87

Please sign in to comment.