From e561df39d08e83c64fb7494ab45fdc05b9d6ad1e Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 24 Nov 2023 12:00:47 +0800 Subject: [PATCH 01/36] added install to plugin CLIManager Signed-off-by: Patrick Zheng --- internal/file/file.go | 50 +++++++- internal/file/file_test.go | 179 ++++++++++++++++++++++++++ internal/semver/semver.go | 44 +++++++ internal/semver/semver_test.go | 39 ++++++ plugin/manager.go | 79 ++++++++++++ plugin/manager_test.go | 228 ++++++++++++++++++++++++++++++++- plugin/plugin.go | 15 ++- plugin/plugin_test.go | 30 +++++ 8 files changed, 661 insertions(+), 3 deletions(-) create mode 100644 internal/file/file_test.go create mode 100644 internal/semver/semver.go create mode 100644 internal/semver/semver_test.go diff --git a/internal/file/file.go b/internal/file/file.go index 2fe42f05..d321323d 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -13,9 +13,57 @@ package file -import "regexp" +import ( + "fmt" + "io" + "os" + "path/filepath" + "regexp" + "strings" +) // IsValidFileName checks if a file name is cross-platform compatible func IsValidFileName(fileName string) bool { return regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`).MatchString(fileName) } + +// CopyToDir copies the src file to dst. Existing file will be overwritten. +func CopyToDir(src, dst string) (int64, error) { + sourceFileStat, err := os.Stat(src) + if err != nil { + return 0, err + } + + if !sourceFileStat.Mode().IsRegular() { + return 0, fmt.Errorf("%s is not a regular file", src) + } + + source, err := os.Open(src) + if err != nil { + return 0, err + } + defer source.Close() + + if err := os.MkdirAll(dst, 0700); err != nil { + return 0, err + } + dstFile := filepath.Join(dst, filepath.Base(src)) + destination, err := os.Create(dstFile) + if err != nil { + return 0, err + } + defer destination.Close() + err = destination.Chmod(0600) + if err != nil { + return 0, err + } + return io.Copy(destination, source) +} + +// FileNameWithoutExtension returns the file name without extension. +// For example, +// when input is xyz.exe, output is xyz +// when input is xyz.tar.gz, output is xyz.tar +func FileNameWithoutExtension(fileName string) string { + return strings.TrimSuffix(fileName, filepath.Ext(fileName)) +} diff --git a/internal/file/file_test.go b/internal/file/file_test.go new file mode 100644 index 00000000..cf6d348e --- /dev/null +++ b/internal/file/file_test.go @@ -0,0 +1,179 @@ +// Copyright The Notary Project Authors. +// 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 file + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestCopyToDir(t *testing.T) { + t.Run("copy file", func(t *testing.T) { + tempDir := t.TempDir() + data := []byte("data") + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + + destDir := filepath.Join(tempDir, "b") + if _, err := CopyToDir(filename, destDir); err != nil { + t.Fatal(err) + } + }) + + t.Run("source directory permission error", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + + tempDir := t.TempDir() + destDir := t.TempDir() + data := []byte("data") + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + + if err := os.Chmod(tempDir, 0000); err != nil { + t.Fatal(err) + } + defer os.Chmod(tempDir, 0700) + + if _, err := CopyToDir(filename, destDir); err == nil { + t.Fatal("should have error") + } + }) + + t.Run("not a regular file", func(t *testing.T) { + tempDir := t.TempDir() + destDir := t.TempDir() + if _, err := CopyToDir(tempDir, destDir); err == nil { + t.Fatal("should have error") + } + }) + + t.Run("source file permission error", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + + tempDir := t.TempDir() + destDir := t.TempDir() + data := []byte("data") + // prepare file + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + // forbid reading + if err := os.Chmod(filename, 0000); err != nil { + t.Fatal(err) + } + defer os.Chmod(filename, 0600) + if _, err := CopyToDir(filename, destDir); err == nil { + t.Fatal("should have error") + } + }) + + t.Run("dest directory permission error", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + + tempDir := t.TempDir() + destTempDir := t.TempDir() + data := []byte("data") + // prepare file + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + // forbid dest directory operation + if err := os.Chmod(destTempDir, 0000); err != nil { + t.Fatal(err) + } + defer os.Chmod(destTempDir, 0700) + if _, err := CopyToDir(filename, filepath.Join(destTempDir, "a")); err == nil { + t.Fatal("should have error") + } + }) + + t.Run("dest directory permission error 2", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } + + tempDir := t.TempDir() + destTempDir := t.TempDir() + data := []byte("data") + // prepare file + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + // forbid writing to destTempDir + if err := os.Chmod(destTempDir, 0000); err != nil { + t.Fatal(err) + } + defer os.Chmod(destTempDir, 0700) + if _, err := CopyToDir(filename, destTempDir); err == nil { + t.Fatal("should have error") + } + }) + + t.Run("copy file and check content", func(t *testing.T) { + tempDir := t.TempDir() + data := []byte("data") + filename := filepath.Join(tempDir, "a", "file.txt") + if err := writeFile(filename, data); err != nil { + t.Fatal(err) + } + + destDir := filepath.Join(tempDir, "b") + if _, err := CopyToDir(filename, destDir); err != nil { + t.Fatal(err) + } + validFileContent(t, filepath.Join(destDir, "file.txt"), data) + }) +} + +func TestFileNameWithoutExtension(t *testing.T) { + input := "testfile.tar.gz" + expectedOutput := "testfile.tar" + actualOutput := FileNameWithoutExtension(input) + if actualOutput != expectedOutput { + t.Errorf("expected '%s', but got '%s'", expectedOutput, actualOutput) + } +} + +func validFileContent(t *testing.T, filename string, content []byte) { + b, err := os.ReadFile(filename) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(content, b) { + t.Fatal("file content is not correct") + } +} + +func writeFile(path string, data []byte) error { + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + return err + } + return os.WriteFile(path, data, 0600) +} diff --git a/internal/semver/semver.go b/internal/semver/semver.go new file mode 100644 index 00000000..db59455d --- /dev/null +++ b/internal/semver/semver.go @@ -0,0 +1,44 @@ +// Copyright The Notary Project Authors. +// 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 semver + +import ( + "fmt" + "strings" + + "golang.org/x/mod/semver" +) + +// ComparePluginVersion validates and compares two plugin semantic versions. +// +// The result will be 0 if v == w, -1 if v < w, or +1 if v > w. +func ComparePluginVersion(v, w string) (int, error) { + // golang.org/x/mod/semver requires semantic version strings must begin + // with a leading "v". Adding prefix "v" in case the input plugin version + // does not have it. + // Reference: https://pkg.go.dev/golang.org/x/mod/semver#pkg-overview + if !strings.HasPrefix(v, "v") { + v = "v" + v + } + if !semver.IsValid(v) { + return 0, fmt.Errorf("%s is not a valid semantic version", v) + } + if !strings.HasPrefix(w, "v") { + w = "v" + w + } + if !semver.IsValid(w) { + return 0, fmt.Errorf("%s is not a valid semantic version", w) + } + return semver.Compare(v, w), nil +} diff --git a/internal/semver/semver_test.go b/internal/semver/semver_test.go new file mode 100644 index 00000000..fd91dea4 --- /dev/null +++ b/internal/semver/semver_test.go @@ -0,0 +1,39 @@ +// Copyright The Notary Project Authors. +// 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 semver + +import "testing" + +func TestComparePluginVersion(t *testing.T) { + comp, err := ComparePluginVersion("v1.0.0", "v1.0.1") + if err != nil || comp >= 0 { + t.Fatal("expected nil err and negative comp") + } + + comp, err = ComparePluginVersion("1.0.0", "1.0.1") + if err != nil || comp >= 0 { + t.Fatal("expected nil err and negative comp") + } + + comp, err = ComparePluginVersion("1.0.1", "1.0.1") + if err != nil || comp != 0 { + t.Fatal("expected nil err and comp equal to 0") + } + + expectedErrMsg := "vabc is not a valid semantic version" + _, err = ComparePluginVersion("abc", "1.0.1") + if err == nil || err.Error() != expectedErrMsg { + t.Fatalf("expected err %s, got %s", expectedErrMsg, err) + } +} diff --git a/plugin/manager.go b/plugin/manager.go index 6b5d98b8..4584f4d8 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -16,11 +16,16 @@ package plugin import ( "context" "errors" + "fmt" "io/fs" "os" "path" + "path/filepath" "github.com/notaryproject/notation-go/dir" + "github.com/notaryproject/notation-go/internal/file" + "github.com/notaryproject/notation-go/internal/semver" + "github.com/notaryproject/notation-go/plugin/proto" ) // ErrNotCompliant is returned by plugin methods when the response is not @@ -30,6 +35,14 @@ var ErrNotCompliant = errors.New("plugin not compliant") // ErrNotRegularFile is returned when the plugin file is not an regular file. var ErrNotRegularFile = errors.New("not regular file") +// ErrInstallLowerVersion is returned when installing a plugin with version +// lower than the exisiting plugin version. +var ErrInstallLowerVersion = errors.New("installing plugin with version lower than the existing plugin version") + +// ErrInstallEqualVersion is returned when installing a plugin with version +// equal to the exisiting plugin version. +var ErrInstallEqualVersion = errors.New("installing plugin with version equal to the existing plugin version") + // Manager manages plugins installed on the system. type Manager interface { Get(ctx context.Context, name string) (Plugin, error) @@ -84,6 +97,72 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { return plugins, nil } +// Install installs a plugin at filePath to the system, +// err == nil if and only if the installation succeeded. +// +// If plugin already exists, and overwrite is not set, then the new plugin +// version MUST be higher than the existing plugin version. +// If overwrite is set, version check is skipped. +// On sucess, existing plugin metadata and new plugin metadata are returned. +// +// If plugin does not exist, directly install the plugin from filePath. +// On success, the new plugin metadata is returned. +func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { + // validate and get new plugin metadata + pluginName, err := ExtractPluginNameFromFileName(filepath.Base(filePath)) + if err != nil { + return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) + } + newPlugin, err := NewCLIPlugin(ctx, pluginName, filePath) + if err != nil { + return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) + } + newPluginMetadata, err := newPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) + if err != nil { + return nil, nil, fmt.Errorf("failed to get metadata of new plugin: %w", err) + } + // check plugin existence and get existing plugin metadata + var existingPluginMetadata *proto.GetMetadataResponse + existingPlugin, err := m.Get(ctx, pluginName) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + return nil, nil, fmt.Errorf("failed to get existing plugin: %w", err) + } + } else { // plugin already exists + var err error + existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) + if err != nil { + return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) + } + if !overwrite { // overwrite is not set, check version + comp, err := semver.ComparePluginVersion(newPluginMetadata.Version, existingPluginMetadata.Version) + if err != nil { + return nil, nil, fmt.Errorf("failed to compare plugin versions: %w", err) + } + switch { + case comp < 0: + return nil, nil, ErrInstallLowerVersion + case comp == 0: + return nil, nil, ErrInstallEqualVersion + } + } + } + pluginDirPath, err := m.pluginFS.SysPath(pluginName) + if err != nil { + return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) + } + _, err = file.CopyToDir(filePath, pluginDirPath) + if err != nil { + return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", filePath, pluginDirPath, err) + } + // plugin is always executable + pluginFilePath := path.Join(pluginDirPath, binName(pluginName)) + if err := os.Chmod(pluginFilePath, 0700); err != nil { + return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) + } + return existingPluginMetadata, newPluginMetadata, nil +} + // Uninstall uninstalls a plugin on the system by its name // If the plugin dir does not exist, os.ErrNotExist is returned. func (m *CLIManager) Uninstall(ctx context.Context, name string) error { diff --git a/plugin/manager_test.go b/plugin/manager_test.go index b3b03488..20137776 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -18,6 +18,7 @@ import ( "encoding/json" "io/fs" "os" + "path/filepath" "reflect" "testing" "testing/fstest" @@ -36,8 +37,45 @@ func (t testCommander) Output(ctx context.Context, path string, command proto.Co return t.stdout, t.stderr, t.err } +type testInstallCommander struct { + existedPluginFilePath string + existedPluginStdout []byte + existedPluginStderr []byte + existedPluginErr error + newPluginFilePath string + newPluginStdout []byte + newPluginStderr []byte + newPluginErr error + err error +} + +func (t testInstallCommander) Output(ctx context.Context, path string, command proto.Command, req []byte) ([]byte, []byte, error) { + if path == t.existedPluginFilePath { + return t.existedPluginStdout, t.existedPluginStderr, t.existedPluginErr + } + if path == t.newPluginFilePath { + return t.newPluginStdout, t.newPluginStderr, t.newPluginErr + } + return nil, nil, t.err +} + var validMetadata = proto.GetMetadataResponse{ - Name: "foo", Description: "friendly", Version: "1", URL: "example.com", + Name: "foo", Description: "friendly", Version: "1.0.0", URL: "example.com", + SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, +} + +var validMetadataHigherVersion = proto.GetMetadataResponse{ + Name: "foo", Description: "friendly", Version: "1.1.0", URL: "example.com", + SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, +} + +var validMetadataLowerVersion = proto.GetMetadataResponse{ + Name: "foo", Description: "friendly", Version: "0.1.0", URL: "example.com", + SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, +} + +var validMetadataBar = proto.GetMetadataResponse{ + Name: "bar", Description: "friendly", Version: "1.0.0", URL: "example.com", SupportedContractVersions: []string{"1.0"}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, } @@ -88,6 +126,194 @@ func TestManager_List(t *testing.T) { }) } +func TestManager_Install(t *testing.T) { + existedPluginFilePath := "testdata/plugins/foo/notation-foo" + newPluginFilePath := "testdata/foo/notation-foo" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } + mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "testdata/plugins")) + + t.Run("success install with higher version", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataHigherVersion), + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), newPluginFilePath, false) + if err != nil { + t.Fatalf("expecting error to be nil, but got %v", err) + } + if existingPluginMetadata.Version != validMetadata.Version { + t.Fatalf("existing plugin version mismatch, existing plugin version: %s, but got: %s", validMetadata.Version, existingPluginMetadata.Version) + } + if newPluginMetadata.Version != validMetadataHigherVersion.Version { + t.Fatalf("new plugin version mismatch, new plugin version: %s, but got: %s", validMetadataHigherVersion.Version, newPluginMetadata.Version) + } + }) + + t.Run("success install with lower version and overwrite", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataLowerVersion), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, true) + if err != nil { + t.Fatalf("expecting error to be nil, bug got %v", err) + } + }) + + t.Run("success install without existing plugin", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), newPluginFilePath, false) + if err != nil { + t.Fatalf("expecting error to be nil, bug got %v", err) + } + if existingPluginMetadata != nil { + t.Fatalf("expecting existingPluginMetadata to be nil, bug got %v", existingPluginMetadata) + } + if newPluginMetadata.Version != validMetadataBar.Version { + t.Fatalf("new plugin version mismatch, new plugin version: %s, but got: %s", validMetadataBar.Version, newPluginMetadata.Version) + } + // clean up + err = mgr.Uninstall(context.Background(), "bar") + if err != nil { + t.Fatal("failed to clean up test plugin bar") + } + }) + + t.Run("fail to install due to equal version", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadata), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + if err == nil || err.Error() != ErrInstallEqualVersion.Error() { + t.Fatalf("expecting error %v, bug got %v", ErrInstallEqualVersion, err) + } + }) + + t.Run("fail to install due to lower version", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataLowerVersion), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + if err == nil || err.Error() != ErrInstallLowerVersion.Error() { + t.Fatalf("expecting error %v, bug got %v", ErrInstallLowerVersion, err) + } + }) + + t.Run("fail to install due to wrong plugin executable file name format", func(t *testing.T) { + newPluginFilePath := "testdata/bar/bar" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + expectedErrorMsg := "failed to get plugin name from file path: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + } + }) + + t.Run("fail to install due to new plugin executable file does not exist", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar" + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + expectedErrorMsg := "failed to create new CLI plugin: stat testdata/bar/notation-bar: no such file or directory" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + } + }) + + t.Run("fail to install due to invalid new plugin metadata", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(invalidMetadataName), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\"" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + } + }) + + t.Run("fail to install due to invalid existing plugin metadata", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadataBar), + newPluginStdout: metadataJSON(validMetadata), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + } + }) +} + func TestManager_Uninstall(t *testing.T) { executor = testCommander{stdout: metadataJSON(validMetadata)} mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "./testdata/plugins")) diff --git a/plugin/plugin.go b/plugin/plugin.go index ab720e35..eedd5465 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -25,7 +25,9 @@ import ( "os" "os/exec" "path/filepath" + "strings" + "github.com/notaryproject/notation-go/internal/file" "github.com/notaryproject/notation-go/internal/slices" "github.com/notaryproject/notation-go/log" "github.com/notaryproject/notation-go/plugin/proto" @@ -74,7 +76,7 @@ type CLIPlugin struct { path string } -// NewCLIPlugin validate the metadata of the plugin and return a *CLIPlugin. +// NewCLIPlugin returns a *CLIPlugin. func NewCLIPlugin(ctx context.Context, name, path string) (*CLIPlugin, error) { // validate file existence fi, err := os.Stat(path) @@ -251,3 +253,14 @@ func validate(metadata *proto.GetMetadataResponse) error { } return nil } + +// ExtractPluginNameFromFileName checks if fileName is a valid plugin file name +// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation +func ExtractPluginNameFromFileName(fileName string) (string, error) { + fname := file.FileNameWithoutExtension(fileName) + pluginName, found := strings.CutPrefix(fname, proto.Prefix) + if !found { + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + } + return pluginName, nil +} diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 45a43fc4..f09f9b02 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -270,3 +270,33 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { } }) } + +func TestExtractPluginNameFromExecutableFileName(t *testing.T) { + pluginName, err := ExtractPluginNameFromFileName("notation-my-plugin") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + + pluginName, err = ExtractPluginNameFromFileName("notation-my-plugin.exe") + if err != nil { + t.Fatalf("expected nil err, got %v", err) + } + if pluginName != "my-plugin" { + t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) + } + + _, err = ExtractPluginNameFromFileName("myPlugin") + expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, got %v", expectedErrorMsg, err) + } + + _, err = ExtractPluginNameFromFileName("my-plugin") + expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expected %s, got %v", expectedErrorMsg, err) + } +} From 6aabbeb0d466b8d34938b0f791dbb7b57fd31f72 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 24 Nov 2023 13:42:53 +0800 Subject: [PATCH 02/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 6 ++++-- plugin/manager_test.go | 34 ++++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 4584f4d8..a8fcaa3d 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -97,8 +97,10 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { return plugins, nil } -// Install installs a plugin at filePath to the system, -// err == nil if and only if the installation succeeded. +// Install installs a plugin at filePath to the system. +// +// It returns the new plugin metadata and a nil eror if and only if +// the installation succeeded. // // If plugin already exists, and overwrite is not set, then the new plugin // version MUST be higher than the existing plugin version. diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 20137776..98341386 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -20,6 +20,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "testing" "testing/fstest" @@ -90,6 +91,9 @@ var invalidContractVersionMetadata = proto.GetMetadataResponse{ } func TestManager_Get(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } executor = testCommander{stdout: metadataJSON(validMetadata)} mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "./testdata/plugins")) _, err := mgr.Get(context.Background(), "foo") @@ -127,6 +131,9 @@ func TestManager_List(t *testing.T) { } func TestManager_Install(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo" newPluginDir := filepath.Dir(newPluginFilePath) @@ -171,7 +178,7 @@ func TestManager_Install(t *testing.T) { } _, _, err = mgr.Install(context.Background(), newPluginFilePath, true) if err != nil { - t.Fatalf("expecting error to be nil, bug got %v", err) + t.Fatalf("expecting error to be nil, but got %v", err) } }) @@ -193,21 +200,17 @@ func TestManager_Install(t *testing.T) { newPluginFilePath: newPluginFilePath, newPluginStdout: metadataJSON(validMetadataBar), } + defer mgr.Uninstall(context.Background(), "bar") existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), newPluginFilePath, false) if err != nil { - t.Fatalf("expecting error to be nil, bug got %v", err) + t.Fatalf("expecting error to be nil, but got %v", err) } if existingPluginMetadata != nil { - t.Fatalf("expecting existingPluginMetadata to be nil, bug got %v", existingPluginMetadata) + t.Fatalf("expecting existingPluginMetadata to be nil, but got %v", existingPluginMetadata) } if newPluginMetadata.Version != validMetadataBar.Version { t.Fatalf("new plugin version mismatch, new plugin version: %s, but got: %s", validMetadataBar.Version, newPluginMetadata.Version) } - // clean up - err = mgr.Uninstall(context.Background(), "bar") - if err != nil { - t.Fatal("failed to clean up test plugin bar") - } }) t.Run("fail to install due to equal version", func(t *testing.T) { @@ -219,7 +222,7 @@ func TestManager_Install(t *testing.T) { } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) if err == nil || err.Error() != ErrInstallEqualVersion.Error() { - t.Fatalf("expecting error %v, bug got %v", ErrInstallEqualVersion, err) + t.Fatalf("expecting error %v, but got %v", ErrInstallEqualVersion, err) } }) @@ -232,7 +235,7 @@ func TestManager_Install(t *testing.T) { } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) if err == nil || err.Error() != ErrInstallLowerVersion.Error() { - t.Fatalf("expecting error %v, bug got %v", ErrInstallLowerVersion, err) + t.Fatalf("expecting error %v, but got %v", ErrInstallLowerVersion, err) } }) @@ -257,7 +260,7 @@ func TestManager_Install(t *testing.T) { _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) expectedErrorMsg := "failed to get plugin name from file path: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) @@ -270,7 +273,7 @@ func TestManager_Install(t *testing.T) { _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) expectedErrorMsg := "failed to create new CLI plugin: stat testdata/bar/notation-bar: no such file or directory" if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) @@ -295,7 +298,7 @@ func TestManager_Install(t *testing.T) { _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\"" if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) @@ -309,12 +312,15 @@ func TestManager_Install(t *testing.T) { _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, bug got %v", expectedErrorMsg, err) + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) } func TestManager_Uninstall(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on Windows") + } executor = testCommander{stdout: metadataJSON(validMetadata)} mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "./testdata/plugins")) if err := os.MkdirAll("./testdata/plugins/toUninstall", 0777); err != nil { From aa458a807851fca258e6b55f29a04c428f7b2c9d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 24 Nov 2023 14:39:46 +0800 Subject: [PATCH 03/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 3 +++ plugin/manager_test.go | 13 +++++++++++++ plugin/plugin.go | 30 ++++++++++++++++++++++-------- plugin/plugin_test.go | 27 +++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 8 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index a8fcaa3d..76fb7485 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -115,6 +115,9 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo if err != nil { return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) } + if err := validatePluginFileExtensionAgainstOS(filePath, pluginName); err != nil { + return nil, nil, err + } newPlugin, err := NewCLIPlugin(ctx, pluginName, filePath) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 98341386..3fe49acb 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -264,6 +264,19 @@ func TestManager_Install(t *testing.T) { } }) + t.Run("fail to install due to invalid new plugin file extension", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar.exe" + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-bar, but got notation-bar.exe" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + }) + t.Run("fail to install due to new plugin executable file does not exist", func(t *testing.T) { newPluginFilePath := "testdata/bar/notation-bar" executor = testInstallCommander{ diff --git a/plugin/plugin.go b/plugin/plugin.go index eedd5465..caa492f5 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -225,6 +225,17 @@ func (c execCommander) Output(ctx context.Context, name string, command proto.Co return stdout.Bytes(), nil, nil } +// ExtractPluginNameFromFileName checks if fileName is a valid plugin file name +// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation +func ExtractPluginNameFromFileName(fileName string) (string, error) { + fname := file.FileNameWithoutExtension(fileName) + pluginName, found := strings.CutPrefix(fname, proto.Prefix) + if !found { + return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) + } + return pluginName, nil +} + // validate checks if the metadata is correctly populated. func validate(metadata *proto.GetMetadataResponse) error { if metadata.Name == "" { @@ -254,13 +265,16 @@ func validate(metadata *proto.GetMetadataResponse) error { return nil } -// ExtractPluginNameFromFileName checks if fileName is a valid plugin file name -// and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation -func ExtractPluginNameFromFileName(fileName string) (string, error) { - fname := file.FileNameWithoutExtension(fileName) - pluginName, found := strings.CutPrefix(fname, proto.Prefix) - if !found { - return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) +// validatePluginFileExtensionAgainstOS validates if plugin executable file +// aligns with the runtime OS. +// +// On windows, `.exe` extension is required. +// On other OS, MUST not have the `.exe` extension. +func validatePluginFileExtensionAgainstOS(filePath string, pluginName string) error { + pluginFile := filepath.Base(filePath) + expectedPluginFile := binName(pluginName) + if filepath.Ext(pluginFile) != filepath.Ext(expectedPluginFile) { + return fmt.Errorf("invalid plugin file name extension. Expecting file %s, but got %s", expectedPluginFile, pluginFile) } - return pluginName, nil + return nil } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index f09f9b02..2329ebe0 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "reflect" + "runtime" "strconv" "strings" "testing" @@ -300,3 +301,29 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } } + +func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { + if runtime.GOOS == "windows" { + err := validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo", "foo") + expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo.exe, but got notation-foo" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + return + } + err := validatePluginFileExtensionAgainstOS("notation-foo", "foo") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo, but got notation-foo.exe" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } +} From 59101ed01bf09d93abea74c322aeb0797c12451d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 24 Nov 2023 17:33:35 +0800 Subject: [PATCH 04/36] updated error messages Signed-off-by: Patrick Zheng --- plugin/errors.go | 49 ++++++++++++++++++++++++++++++++++++++++++ plugin/manager.go | 19 ++-------------- plugin/manager_test.go | 10 +++++---- 3 files changed, 57 insertions(+), 21 deletions(-) create mode 100644 plugin/errors.go diff --git a/plugin/errors.go b/plugin/errors.go new file mode 100644 index 00000000..248a07b7 --- /dev/null +++ b/plugin/errors.go @@ -0,0 +1,49 @@ +// Copyright The Notary Project Authors. +// 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 plugin + +import "errors" + +// ErrNotCompliant is returned by plugin methods when the response is not +// compliant. +var ErrNotCompliant = errors.New("plugin not compliant") + +// ErrNotRegularFile is returned when the plugin file is not an regular file. +var ErrNotRegularFile = errors.New("not regular file") + +// ErrInstallLowerVersion is returned when installing a plugin with version +// lower than the exisiting plugin version. +type ErrInstallLowerVersion struct { + Msg string +} + +func (e ErrInstallLowerVersion) Error() string { + if e.Msg != "" { + return e.Msg + } + return "installing plugin with version lower than the existing plugin version" +} + +// ErrInstallEqualVersion is returned when installing a plugin with version +// equal to the exisiting plugin version. +type ErrInstallEqualVersion struct { + Msg string +} + +func (e ErrInstallEqualVersion) Error() string { + if e.Msg != "" { + return e.Msg + } + return "installing plugin with version equal to the existing plugin version" +} diff --git a/plugin/manager.go b/plugin/manager.go index 76fb7485..dabc9d79 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -28,21 +28,6 @@ import ( "github.com/notaryproject/notation-go/plugin/proto" ) -// ErrNotCompliant is returned by plugin methods when the response is not -// compliant. -var ErrNotCompliant = errors.New("plugin not compliant") - -// ErrNotRegularFile is returned when the plugin file is not an regular file. -var ErrNotRegularFile = errors.New("not regular file") - -// ErrInstallLowerVersion is returned when installing a plugin with version -// lower than the exisiting plugin version. -var ErrInstallLowerVersion = errors.New("installing plugin with version lower than the existing plugin version") - -// ErrInstallEqualVersion is returned when installing a plugin with version -// equal to the exisiting plugin version. -var ErrInstallEqualVersion = errors.New("installing plugin with version equal to the existing plugin version") - // Manager manages plugins installed on the system. type Manager interface { Get(ctx context.Context, name string) (Plugin, error) @@ -146,9 +131,9 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo } switch { case comp < 0: - return nil, nil, ErrInstallLowerVersion + return nil, nil, ErrInstallLowerVersion{Msg: fmt.Sprintf("The installing plugin version %s is lower than the existing plugin version %s.", newPluginMetadata.Version, existingPluginMetadata.Version)} case comp == 0: - return nil, nil, ErrInstallEqualVersion + return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("Plugin %s with version %s already exists.", pluginName, existingPluginMetadata.Version)} } } } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 3fe49acb..6f2cdb8b 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -221,8 +221,9 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadata), } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - if err == nil || err.Error() != ErrInstallEqualVersion.Error() { - t.Fatalf("expecting error %v, but got %v", ErrInstallEqualVersion, err) + expectedErrorMsg := "Plugin foo with version 1.0.0 already exists." + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) @@ -234,8 +235,9 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadataLowerVersion), } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - if err == nil || err.Error() != ErrInstallLowerVersion.Error() { - t.Fatalf("expecting error %v, but got %v", ErrInstallLowerVersion, err) + expectedErrorMsg := "The installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0." + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) From 24aef8a46c1e71bf26e4f9fbd269b6396b8ea09f Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 24 Nov 2023 17:40:40 +0800 Subject: [PATCH 05/36] updated error messages Signed-off-by: Patrick Zheng --- plugin/manager.go | 4 ++-- plugin/manager_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index dabc9d79..d6620511 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -131,9 +131,9 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo } switch { case comp < 0: - return nil, nil, ErrInstallLowerVersion{Msg: fmt.Sprintf("The installing plugin version %s is lower than the existing plugin version %s.", newPluginMetadata.Version, existingPluginMetadata.Version)} + return nil, nil, ErrInstallLowerVersion{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} case comp == 0: - return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("Plugin %s with version %s already exists.", pluginName, existingPluginMetadata.Version)} + return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)} } } } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 6f2cdb8b..cc6f7673 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -221,7 +221,7 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadata), } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - expectedErrorMsg := "Plugin foo with version 1.0.0 already exists." + expectedErrorMsg := "plugin foo with version 1.0.0 already exists" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -235,7 +235,7 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadataLowerVersion), } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - expectedErrorMsg := "The installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0." + expectedErrorMsg := "the installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } From b1e008452e567614eb1be5962d7a44501b033aa0 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 27 Nov 2023 12:10:28 +0800 Subject: [PATCH 06/36] updated err msg Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index d6620511..0cdd7d59 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -116,7 +116,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo existingPlugin, err := m.Get(ctx, pluginName) if err != nil { if !errors.Is(err, os.ErrNotExist) { - return nil, nil, fmt.Errorf("failed to get existing plugin: %w", err) + return nil, nil, fmt.Errorf("failed to check plugin existence: %w", err) } } else { // plugin already exists var err error From 75a090e11a34195b8fd536e137aba36746a602dc Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 28 Nov 2023 14:56:13 +0800 Subject: [PATCH 07/36] update per code review Signed-off-by: Patrick Zheng --- internal/file/file.go | 3 +++ internal/semver/semver.go | 2 ++ plugin/manager.go | 7 ++++--- plugin/plugin.go | 9 ++++----- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index d321323d..1a6802d1 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -61,8 +61,11 @@ func CopyToDir(src, dst string) (int64, error) { } // FileNameWithoutExtension returns the file name without extension. +// // For example, +// // when input is xyz.exe, output is xyz +// // when input is xyz.tar.gz, output is xyz.tar func FileNameWithoutExtension(fileName string) string { return strings.TrimSuffix(fileName, filepath.Ext(fileName)) diff --git a/internal/semver/semver.go b/internal/semver/semver.go index db59455d..af41e607 100644 --- a/internal/semver/semver.go +++ b/internal/semver/semver.go @@ -11,6 +11,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +// Package semver provides functions related to semanic version. +// This package is based on "golang.org/x/mod/semver" package semver import ( diff --git a/plugin/manager.go b/plugin/manager.go index 0cdd7d59..9f14922f 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -96,11 +96,12 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { // On success, the new plugin metadata is returned. func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { // validate and get new plugin metadata - pluginName, err := ExtractPluginNameFromFileName(filepath.Base(filePath)) + pluginFile := filepath.Base(filePath) + pluginName, err := ExtractPluginNameFromFileName(pluginFile) if err != nil { return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) } - if err := validatePluginFileExtensionAgainstOS(filePath, pluginName); err != nil { + if err := validatePluginFileExtensionAgainstOS(pluginFile, pluginName); err != nil { return nil, nil, err } newPlugin, err := NewCLIPlugin(ctx, pluginName, filePath) @@ -146,7 +147,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", filePath, pluginDirPath, err) } // plugin is always executable - pluginFilePath := path.Join(pluginDirPath, binName(pluginName)) + pluginFilePath := path.Join(pluginDirPath, pluginFile) if err := os.Chmod(pluginFilePath, 0700); err != nil { return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) } diff --git a/plugin/plugin.go b/plugin/plugin.go index caa492f5..97938960 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -266,15 +266,14 @@ func validate(metadata *proto.GetMetadataResponse) error { } // validatePluginFileExtensionAgainstOS validates if plugin executable file -// aligns with the runtime OS. +// extension aligns with the runtime OS. // // On windows, `.exe` extension is required. // On other OS, MUST not have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(filePath string, pluginName string) error { - pluginFile := filepath.Base(filePath) +func validatePluginFileExtensionAgainstOS(fileName string, pluginName string) error { expectedPluginFile := binName(pluginName) - if filepath.Ext(pluginFile) != filepath.Ext(expectedPluginFile) { - return fmt.Errorf("invalid plugin file name extension. Expecting file %s, but got %s", expectedPluginFile, pluginFile) + if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) { + return fmt.Errorf("invalid plugin file name extension. Expecting file %s, but got %s", expectedPluginFile, fileName) } return nil } From bd92473ef4d2f1e42e6f93da9c426b599c9f43ea Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 6 Dec 2023 13:13:51 +0800 Subject: [PATCH 08/36] updated per code review Signed-off-by: Patrick Zheng --- internal/file/file.go | 17 +++++++++-------- internal/file/file_test.go | 14 +++++++------- internal/semver/semver.go | 33 +++++++++++++++++++-------------- plugin/errors.go | 6 +++--- plugin/manager.go | 23 ++++++++++++----------- plugin/manager_test.go | 15 ++++++++++++++- plugin/plugin.go | 4 ++-- plugin/plugin_test.go | 4 ++-- verifier/helpers.go | 11 ++--------- verifier/verifier.go | 3 ++- 10 files changed, 72 insertions(+), 58 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index 1a6802d1..a9eab3fd 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -28,36 +28,37 @@ func IsValidFileName(fileName string) bool { } // CopyToDir copies the src file to dst. Existing file will be overwritten. -func CopyToDir(src, dst string) (int64, error) { +func CopyToDir(src, dst string) error { sourceFileStat, err := os.Stat(src) if err != nil { - return 0, err + return err } if !sourceFileStat.Mode().IsRegular() { - return 0, fmt.Errorf("%s is not a regular file", src) + return fmt.Errorf("%s is not a regular file", src) } source, err := os.Open(src) if err != nil { - return 0, err + return err } defer source.Close() if err := os.MkdirAll(dst, 0700); err != nil { - return 0, err + return err } dstFile := filepath.Join(dst, filepath.Base(src)) destination, err := os.Create(dstFile) if err != nil { - return 0, err + return err } defer destination.Close() err = destination.Chmod(0600) if err != nil { - return 0, err + return err } - return io.Copy(destination, source) + _, err = io.Copy(destination, source) + return err } // FileNameWithoutExtension returns the file name without extension. diff --git a/internal/file/file_test.go b/internal/file/file_test.go index cf6d348e..678758cf 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -31,7 +31,7 @@ func TestCopyToDir(t *testing.T) { } destDir := filepath.Join(tempDir, "b") - if _, err := CopyToDir(filename, destDir); err != nil { + if err := CopyToDir(filename, destDir); err != nil { t.Fatal(err) } }) @@ -54,7 +54,7 @@ func TestCopyToDir(t *testing.T) { } defer os.Chmod(tempDir, 0700) - if _, err := CopyToDir(filename, destDir); err == nil { + if err := CopyToDir(filename, destDir); err == nil { t.Fatal("should have error") } }) @@ -62,7 +62,7 @@ func TestCopyToDir(t *testing.T) { t.Run("not a regular file", func(t *testing.T) { tempDir := t.TempDir() destDir := t.TempDir() - if _, err := CopyToDir(tempDir, destDir); err == nil { + if err := CopyToDir(tempDir, destDir); err == nil { t.Fatal("should have error") } }) @@ -85,7 +85,7 @@ func TestCopyToDir(t *testing.T) { t.Fatal(err) } defer os.Chmod(filename, 0600) - if _, err := CopyToDir(filename, destDir); err == nil { + if err := CopyToDir(filename, destDir); err == nil { t.Fatal("should have error") } }) @@ -108,7 +108,7 @@ func TestCopyToDir(t *testing.T) { t.Fatal(err) } defer os.Chmod(destTempDir, 0700) - if _, err := CopyToDir(filename, filepath.Join(destTempDir, "a")); err == nil { + if err := CopyToDir(filename, filepath.Join(destTempDir, "a")); err == nil { t.Fatal("should have error") } }) @@ -131,7 +131,7 @@ func TestCopyToDir(t *testing.T) { t.Fatal(err) } defer os.Chmod(destTempDir, 0700) - if _, err := CopyToDir(filename, destTempDir); err == nil { + if err := CopyToDir(filename, destTempDir); err == nil { t.Fatal("should have error") } }) @@ -145,7 +145,7 @@ func TestCopyToDir(t *testing.T) { } destDir := filepath.Join(tempDir, "b") - if _, err := CopyToDir(filename, destDir); err != nil { + if err := CopyToDir(filename, destDir); err != nil { t.Fatal(err) } validFileContent(t, filepath.Join(destDir, "file.txt"), data) diff --git a/internal/semver/semver.go b/internal/semver/semver.go index af41e607..55e7efaf 100644 --- a/internal/semver/semver.go +++ b/internal/semver/semver.go @@ -17,30 +17,35 @@ package semver import ( "fmt" - "strings" + "regexp" "golang.org/x/mod/semver" ) +// SemVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string +var SemVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) + +// IsVersionSemverValid returns true if version is a valid semantic version +func IsVersionSemverValid(version string) bool { + return SemVerRegEx.MatchString(version) +} + // ComparePluginVersion validates and compares two plugin semantic versions. // // The result will be 0 if v == w, -1 if v < w, or +1 if v > w. func ComparePluginVersion(v, w string) (int, error) { - // golang.org/x/mod/semver requires semantic version strings must begin - // with a leading "v". Adding prefix "v" in case the input plugin version - // does not have it. - // Reference: https://pkg.go.dev/golang.org/x/mod/semver#pkg-overview - if !strings.HasPrefix(v, "v") { - v = "v" + v - } - if !semver.IsValid(v) { + // sanity check + // a valid semantic version should not have prefix `v` + // Reference: https://semver.org/#semantic-versioning-200 + if !IsVersionSemverValid(v) { return 0, fmt.Errorf("%s is not a valid semantic version", v) } - if !strings.HasPrefix(w, "v") { - w = "v" + w - } - if !semver.IsValid(w) { + if !IsVersionSemverValid(w) { return 0, fmt.Errorf("%s is not a valid semantic version", w) } - return semver.Compare(v, w), nil + + // golang.org/x/mod/semver requires semantic version strings must begin + // with a leading "v". Adding prefix "v" to the inputs. + // Reference: https://pkg.go.dev/golang.org/x/mod/semver#pkg-overview + return semver.Compare("v"+v, "v"+w), nil } diff --git a/plugin/errors.go b/plugin/errors.go index 248a07b7..b661cf2f 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -22,13 +22,13 @@ var ErrNotCompliant = errors.New("plugin not compliant") // ErrNotRegularFile is returned when the plugin file is not an regular file. var ErrNotRegularFile = errors.New("not regular file") -// ErrInstallLowerVersion is returned when installing a plugin with version +// ErrPluginDowngrade is returned when installing a plugin with version // lower than the exisiting plugin version. -type ErrInstallLowerVersion struct { +type ErrPluginDowngrade struct { Msg string } -func (e ErrInstallLowerVersion) Error() string { +func (e ErrPluginDowngrade) Error() string { if e.Msg != "" { return e.Msg } diff --git a/plugin/manager.go b/plugin/manager.go index 9f14922f..16dc0849 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -83,17 +83,19 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { } // Install installs a plugin at filePath to the system. -// -// It returns the new plugin metadata and a nil eror if and only if +// It returns the new plugin metadata and a nil error if and only if // the installation succeeded. // -// If plugin already exists, and overwrite is not set, then the new plugin +// If plugin does not exist, directly install from filePath. +// On success, the new plugin metadata is returned. +// +// If plugin already exists: +// +// If overwrite is not set, then the new plugin // version MUST be higher than the existing plugin version. -// If overwrite is set, version check is skipped. -// On sucess, existing plugin metadata and new plugin metadata are returned. // -// If plugin does not exist, directly install the plugin from filePath. -// On success, the new plugin metadata is returned. +// If overwrite is set, version check is skipped. If existing +// plugin is malfunctioning, it will be overwritten as well. func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { // validate and get new plugin metadata pluginFile := filepath.Base(filePath) @@ -122,7 +124,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo } else { // plugin already exists var err error existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) - if err != nil { + if err != nil && !overwrite { // fail only if overwrite is not set return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) } if !overwrite { // overwrite is not set, check version @@ -132,7 +134,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo } switch { case comp < 0: - return nil, nil, ErrInstallLowerVersion{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} + return nil, nil, ErrPluginDowngrade{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} case comp == 0: return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)} } @@ -142,8 +144,7 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - _, err = file.CopyToDir(filePath, pluginDirPath) - if err != nil { + if err := file.CopyToDir(filePath, pluginDirPath); err != nil { return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", filePath, pluginDirPath, err) } // plugin is always executable diff --git a/plugin/manager_test.go b/plugin/manager_test.go index cc6f7673..f3d91a99 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -273,7 +273,7 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadataBar), } _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-bar, but got notation-bar.exe" + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -330,6 +330,19 @@ func TestManager_Install(t *testing.T) { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) + + t.Run("success to install with overwrite and invalid existing plugin metadata", func(t *testing.T) { + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadataBar), + newPluginStdout: metadataJSON(validMetadata), + } + _, _, err = mgr.Install(context.Background(), newPluginFilePath, true) + if err != nil { + t.Fatalf("expecting error to be nil, but got %v", err) + } + }) } func TestManager_Uninstall(t *testing.T) { diff --git a/plugin/plugin.go b/plugin/plugin.go index 97938960..6c70aeb1 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -270,10 +270,10 @@ func validate(metadata *proto.GetMetadataResponse) error { // // On windows, `.exe` extension is required. // On other OS, MUST not have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(fileName string, pluginName string) error { +func validatePluginFileExtensionAgainstOS(fileName, pluginName string) error { expectedPluginFile := binName(pluginName) if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) { - return fmt.Errorf("invalid plugin file name extension. Expecting file %s, but got %s", expectedPluginFile, fileName) + return fmt.Errorf("invalid plugin file extension. Expecting file %s, but got %s", expectedPluginFile, fileName) } return nil } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 2329ebe0..85b8a862 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -310,7 +310,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { } err = validatePluginFileExtensionAgainstOS("notation-foo", "foo") - expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo.exe, but got notation-foo" + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -322,7 +322,7 @@ func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { } err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") - expectedErrorMsg := "invalid plugin file name extension. Expecting file notation-foo, but got notation-foo.exe" + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } diff --git a/verifier/helpers.go b/verifier/helpers.go index dfabadb0..8e56530e 100644 --- a/verifier/helpers.go +++ b/verifier/helpers.go @@ -18,12 +18,12 @@ import ( "crypto/x509" "errors" "fmt" - "regexp" "strings" "github.com/notaryproject/notation-core-go/signature" "github.com/notaryproject/notation-go" set "github.com/notaryproject/notation-go/internal/container" + notationsemver "github.com/notaryproject/notation-go/internal/semver" "github.com/notaryproject/notation-go/internal/slices" "github.com/notaryproject/notation-go/verifier/trustpolicy" "github.com/notaryproject/notation-go/verifier/truststore" @@ -47,9 +47,6 @@ var VerificationPluginHeaders = []string{ var errExtendedAttributeNotExist = errors.New("extended attribute not exist") -// semVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string -var semVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) - func loadX509TrustStores(ctx context.Context, scheme signature.SigningScheme, policy *trustpolicy.TrustPolicy, x509TrustStore truststore.X509TrustStore) ([]*x509.Certificate, error) { var typeToLoad truststore.Type switch scheme { @@ -152,12 +149,8 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string, if strings.TrimSpace(version) == "" { return "", fmt.Errorf("%v from extended attribute is an empty string", HeaderVerificationPluginMinVersion) } - if !isVersionSemverValid(version) { + if !notationsemver.IsVersionSemverValid(version) { return "", fmt.Errorf("%v from extended attribute is not a valid SemVer", HeaderVerificationPluginMinVersion) } return version, nil } - -func isVersionSemverValid(version string) bool { - return semVerRegEx.MatchString(version) -} diff --git a/verifier/verifier.go b/verifier/verifier.go index bf76494e..758bd2b5 100644 --- a/verifier/verifier.go +++ b/verifier/verifier.go @@ -32,6 +32,7 @@ import ( "github.com/notaryproject/notation-go/dir" "github.com/notaryproject/notation-go/internal/envelope" "github.com/notaryproject/notation-go/internal/pkix" + notationsemver "github.com/notaryproject/notation-go/internal/semver" "github.com/notaryproject/notation-go/internal/slices" trustpolicyInternal "github.com/notaryproject/notation-go/internal/trustpolicy" "github.com/notaryproject/notation-go/log" @@ -231,7 +232,7 @@ func (v *verifier) processSignature(ctx context.Context, sigBlob []byte, envelop pluginVersion := metadata.Version //checking if the plugin version is in valid semver format - if !isVersionSemverValid(pluginVersion) { + if !notationsemver.IsVersionSemverValid(pluginVersion) { return notation.ErrorVerificationInconclusive{Msg: fmt.Sprintf("plugin %s has pluginVersion %s which is not in valid semver format", verificationPluginName, pluginVersion)} } From c2f7ef881b946b1942f8c5451d79ec0c889ea804 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 6 Dec 2023 13:22:38 +0800 Subject: [PATCH 09/36] fix tests Signed-off-by: Patrick Zheng --- internal/semver/semver_test.go | 37 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/internal/semver/semver_test.go b/internal/semver/semver_test.go index fd91dea4..9ccd5b6d 100644 --- a/internal/semver/semver_test.go +++ b/internal/semver/semver_test.go @@ -16,24 +16,25 @@ package semver import "testing" func TestComparePluginVersion(t *testing.T) { - comp, err := ComparePluginVersion("v1.0.0", "v1.0.1") - if err != nil || comp >= 0 { - t.Fatal("expected nil err and negative comp") - } + t.Run("compare with lower version", func(t *testing.T) { + comp, err := ComparePluginVersion("1.0.0", "1.0.1") + if err != nil || comp >= 0 { + t.Fatal("expected nil err and negative comp") + } + }) - comp, err = ComparePluginVersion("1.0.0", "1.0.1") - if err != nil || comp >= 0 { - t.Fatal("expected nil err and negative comp") - } + t.Run("compare with equal version", func(t *testing.T) { + comp, err := ComparePluginVersion("1.0.1", "1.0.1") + if err != nil || comp != 0 { + t.Fatal("expected nil err and comp equal to 0") + } + }) - comp, err = ComparePluginVersion("1.0.1", "1.0.1") - if err != nil || comp != 0 { - t.Fatal("expected nil err and comp equal to 0") - } - - expectedErrMsg := "vabc is not a valid semantic version" - _, err = ComparePluginVersion("abc", "1.0.1") - if err == nil || err.Error() != expectedErrMsg { - t.Fatalf("expected err %s, got %s", expectedErrMsg, err) - } + t.Run("failed due to invalid semantic version", func(t *testing.T) { + expectedErrMsg := "v1.0.0 is not a valid semantic version" + _, err := ComparePluginVersion("v1.0.0", "1.0.1") + if err == nil || err.Error() != expectedErrMsg { + t.Fatalf("expected err %s, but got %s", expectedErrMsg, err) + } + }) } From 371eaa12cc6d92df64af9bf8c179bee0e3f36ed5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 6 Dec 2023 14:09:21 +0800 Subject: [PATCH 10/36] updated function doc Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index 16dc0849..8ea7b1f5 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -95,7 +95,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { // version MUST be higher than the existing plugin version. // // If overwrite is set, version check is skipped. If existing -// plugin is malfunctioning, it will be overwritten as well. +// plugin is malfunctioning, it will be overwritten. func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { // validate and get new plugin metadata pluginFile := filepath.Base(filePath) From b8451155ea367afc4cd82b334d36244926f33aaf Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 6 Dec 2023 14:17:02 +0800 Subject: [PATCH 11/36] updated function doc Signed-off-by: Patrick Zheng --- plugin/manager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index 8ea7b1f5..6aa25bf2 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -87,7 +87,6 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { // the installation succeeded. // // If plugin does not exist, directly install from filePath. -// On success, the new plugin metadata is returned. // // If plugin already exists: // From 002fd519ebbf60651295bab89b203c87268afe84 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 10:29:16 +0800 Subject: [PATCH 12/36] update Signed-off-by: Patrick Zheng --- internal/file/file.go | 4 ++-- internal/file/file_test.go | 2 +- plugin/plugin.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index a9eab3fd..ee53c8a6 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -61,13 +61,13 @@ func CopyToDir(src, dst string) error { return err } -// FileNameWithoutExtension returns the file name without extension. +// TrimFileExtension returns the file name without extension. // // For example, // // when input is xyz.exe, output is xyz // // when input is xyz.tar.gz, output is xyz.tar -func FileNameWithoutExtension(fileName string) string { +func TrimFileExtension(fileName string) string { return strings.TrimSuffix(fileName, filepath.Ext(fileName)) } diff --git a/internal/file/file_test.go b/internal/file/file_test.go index 678758cf..a108d0da 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -155,7 +155,7 @@ func TestCopyToDir(t *testing.T) { func TestFileNameWithoutExtension(t *testing.T) { input := "testfile.tar.gz" expectedOutput := "testfile.tar" - actualOutput := FileNameWithoutExtension(input) + actualOutput := TrimFileExtension(input) if actualOutput != expectedOutput { t.Errorf("expected '%s', but got '%s'", expectedOutput, actualOutput) } diff --git a/plugin/plugin.go b/plugin/plugin.go index 6c70aeb1..349316f2 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -228,7 +228,7 @@ func (c execCommander) Output(ctx context.Context, name string, command proto.Co // ExtractPluginNameFromFileName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation func ExtractPluginNameFromFileName(fileName string) (string, error) { - fname := file.FileNameWithoutExtension(fileName) + fname := file.TrimFileExtension(fileName) pluginName, found := strings.CutPrefix(fname, proto.Prefix) if !found { return "", fmt.Errorf("invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got %s", fname) From 14aaaa93f11f43a6eb88d11bca050a248c4d6fe5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 19:12:54 +0800 Subject: [PATCH 13/36] updated per code review Signed-off-by: Patrick Zheng --- internal/file/file.go | 41 ++++++++- internal/semver/semver.go | 20 +++-- plugin/errors.go | 12 +-- plugin/manager.go | 128 +++++++++++++++++++++++++---- plugin/manager_test.go | 109 +++++++++++++++++++++--- plugin/manager_unix.go | 19 ++++- plugin/manager_windows.go | 20 ++++- plugin/plugin.go | 4 +- plugin/plugin_test.go | 8 +- plugin/testdata/plugins/foo/libfoo | 0 verifier/helpers.go | 2 +- verifier/verifier.go | 2 +- 12 files changed, 308 insertions(+), 57 deletions(-) create mode 100644 plugin/testdata/plugins/foo/libfoo diff --git a/internal/file/file.go b/internal/file/file.go index ee53c8a6..34021988 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -14,14 +14,21 @@ package file import ( - "fmt" + "errors" "io" + "io/fs" "os" "path/filepath" "regexp" "strings" ) +// ErrNotRegularFile is returned when the file is not an regular file. +var ErrNotRegularFile = errors.New("not regular file") + +// ErrNotDirectory is returned when the path is not a directory. +var ErrNotDirectory = errors.New("not directory") + // IsValidFileName checks if a file name is cross-platform compatible func IsValidFileName(fileName string) bool { return regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`).MatchString(fileName) @@ -35,7 +42,7 @@ func CopyToDir(src, dst string) error { } if !sourceFileStat.Mode().IsRegular() { - return fmt.Errorf("%s is not a regular file", src) + return ErrNotRegularFile } source, err := os.Open(src) @@ -61,6 +68,36 @@ func CopyToDir(src, dst string) error { return err } +// CopyDirToDir copies contents in src dir to dst dir. Only regular files are +// copied. Existing files will be overwritten. +func CopyDirToDir(src, dst string) error { + fi, err := os.Stat(src) + if err != nil { + return err + } + if !fi.Mode().IsDir() { + return ErrNotDirectory + } + return filepath.WalkDir(src, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + // skip sub-directories + if d.IsDir() && d.Name() != filepath.Base(path) { + return fs.SkipDir + } + info, err := d.Info() + if err != nil { + return err + } + // only copy regular files + if info.Mode().IsRegular() { + return CopyToDir(path, dst) + } + return nil + }) +} + // TrimFileExtension returns the file name without extension. // // For example, diff --git a/internal/semver/semver.go b/internal/semver/semver.go index 55e7efaf..6d914558 100644 --- a/internal/semver/semver.go +++ b/internal/semver/semver.go @@ -17,17 +17,19 @@ package semver import ( "fmt" - "regexp" + "strings" "golang.org/x/mod/semver" ) -// SemVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string -var SemVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) - -// IsVersionSemverValid returns true if version is a valid semantic version -func IsVersionSemverValid(version string) bool { - return SemVerRegEx.MatchString(version) +// IsSemverValid returns true if version is a valid semantic version +func IsSemverValid(version string) bool { + // a valid semanic version MUST not have prefix 'v' + if strings.HasPrefix(version, "v") { + return false + } + // golang package "golang.org/x/mod/semver" requires prefix 'v' + return semver.IsValid("v" + version) } // ComparePluginVersion validates and compares two plugin semantic versions. @@ -37,10 +39,10 @@ func ComparePluginVersion(v, w string) (int, error) { // sanity check // a valid semantic version should not have prefix `v` // Reference: https://semver.org/#semantic-versioning-200 - if !IsVersionSemverValid(v) { + if !IsSemverValid(v) { return 0, fmt.Errorf("%s is not a valid semantic version", v) } - if !IsVersionSemverValid(w) { + if !IsSemverValid(w) { return 0, fmt.Errorf("%s is not a valid semantic version", w) } diff --git a/plugin/errors.go b/plugin/errors.go index b661cf2f..2a87f140 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -22,26 +22,26 @@ var ErrNotCompliant = errors.New("plugin not compliant") // ErrNotRegularFile is returned when the plugin file is not an regular file. var ErrNotRegularFile = errors.New("not regular file") -// ErrPluginDowngrade is returned when installing a plugin with version +// PluginDowngradeError is returned when installing a plugin with version // lower than the exisiting plugin version. -type ErrPluginDowngrade struct { +type PluginDowngradeError struct { Msg string } -func (e ErrPluginDowngrade) Error() string { +func (e PluginDowngradeError) Error() string { if e.Msg != "" { return e.Msg } return "installing plugin with version lower than the existing plugin version" } -// ErrInstallEqualVersion is returned when installing a plugin with version +// InstallEqualVersionError is returned when installing a plugin with version // equal to the exisiting plugin version. -type ErrInstallEqualVersion struct { +type InstallEqualVersionError struct { Msg string } -func (e ErrInstallEqualVersion) Error() string { +func (e InstallEqualVersionError) Error() string { if e.Msg != "" { return e.Msg } diff --git a/plugin/manager.go b/plugin/manager.go index 6aa25bf2..c79a5bd1 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -82,11 +82,26 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { return plugins, nil } -// Install installs a plugin at filePath to the system. -// It returns the new plugin metadata and a nil error if and only if -// the installation succeeded. +// CLIInstallOptions provides user customized options for plugin installation +type CLIInstallOptions struct { + // PluginPath can be path of: + // + // 1. A directory which contains plugin related files. Sub-directories are + // ignored. It MUST contain one and only one valid plugin executable file + // following spec: https://github.com/notaryproject/specifications/blob/v1.0.0/specs/plugin-extensibility.md#installation + // + // 2. A single valid plugin executalbe file. + PluginPath string + + // Overwrite is a boolean flag. When set, always install the new plugin. + Overwrite bool +} + +// Install installs a plugin to the system. It returns existing +// plugin metadata, new plugin metadata, and error. It returns nil error +// if and only if the installation succeeded. // -// If plugin does not exist, directly install from filePath. +// If plugin does not exist, directly install from PluginPath. // // If plugin already exists: // @@ -95,17 +110,32 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { // // If overwrite is set, version check is skipped. If existing // plugin is malfunctioning, it will be overwritten. -func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite bool) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { - // validate and get new plugin metadata - pluginFile := filepath.Base(filePath) - pluginName, err := ExtractPluginNameFromFileName(pluginFile) +func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) (*proto.GetMetadataResponse, *proto.GetMetadataResponse, error) { + // initialization + overwrite := installOpts.Overwrite + if installOpts.PluginPath == "" { + return nil, nil, errors.New("plugin path cannot be empty") + } + var installFromSingleFile bool + var pluginFile, pluginName string + pluginFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) if err != nil { - return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) + if !errors.Is(err, file.ErrNotDirectory) { + return nil, nil, fmt.Errorf("failed to validate plugin directory: %w", err) + } + // input is not a dir + installFromSingleFile = true + pluginFile = installOpts.PluginPath + pluginName, err = ParsePluginName(filepath.Base(pluginFile)) + if err != nil { + return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) + } } - if err := validatePluginFileExtensionAgainstOS(pluginFile, pluginName); err != nil { + // validate and get new plugin metadata + if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginFile), pluginName); err != nil { return nil, nil, err } - newPlugin, err := NewCLIPlugin(ctx, pluginName, filePath) + newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) } @@ -126,16 +156,16 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo if err != nil && !overwrite { // fail only if overwrite is not set return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) } - if !overwrite { // overwrite is not set, check version + if !overwrite { // err is nil, and overwrite is not set, check version comp, err := semver.ComparePluginVersion(newPluginMetadata.Version, existingPluginMetadata.Version) if err != nil { return nil, nil, fmt.Errorf("failed to compare plugin versions: %w", err) } switch { case comp < 0: - return nil, nil, ErrPluginDowngrade{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} + return nil, nil, PluginDowngradeError{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} case comp == 0: - return nil, nil, ErrInstallEqualVersion{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)} + return nil, nil, InstallEqualVersionError{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)} } } } @@ -143,11 +173,17 @@ func (m *CLIManager) Install(ctx context.Context, filePath string, overwrite boo if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - if err := file.CopyToDir(filePath, pluginDirPath); err != nil { - return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", filePath, pluginDirPath, err) + if installFromSingleFile { + if err := file.CopyToDir(pluginFile, pluginDirPath); err != nil { + return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginFile, pluginDirPath, err) + } + } else { + if err := file.CopyDirToDir(installOpts.PluginPath, pluginDirPath); err != nil { + return nil, nil, fmt.Errorf("failed to copy plugin files from %s to %s: %w", installOpts.PluginPath, pluginDirPath, err) + } } - // plugin is always executable - pluginFilePath := path.Join(pluginDirPath, pluginFile) + // plugin binary file is always executable + pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginFile)) if err := os.Chmod(pluginFilePath, 0700); err != nil { return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) } @@ -166,3 +202,59 @@ func (m *CLIManager) Uninstall(ctx context.Context, name string) error { } return os.RemoveAll(pluginDirPath) } + +// parsePluginFromDir checks if a dir is a valid plugin dir which contains +// one and only one valid plugin executable file. Sub-directories are ignored. +// +// On success, the plugin executable file path, plugin name and +// nil error are returned. +func parsePluginFromDir(path string) (string, string, error) { + fi, err := os.Stat(path) + if err != nil { + return "", "", err + } + if !fi.Mode().IsDir() { + return "", "", file.ErrNotDirectory + } + // walk the path + var pluginExecutableFile string + var pluginName string + var foundPluginExecutableFile bool + if err := filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + // skip sub-directories + if d.IsDir() && d.Name() != filepath.Base(path) { + return fs.SkipDir + } + info, err := d.Info() + if err != nil { + return err + } + if info.Mode().IsRegular() { + if pluginName, err = ParsePluginName(d.Name()); err != nil { + // file name does not follow the notation-{plugin-name} format + return nil + } + isExec, err := isExecutableFile(p) + if err != nil { + return err + } + if isExec { + if foundPluginExecutableFile { + return fmt.Errorf("found more than one valid plugin executable files under %s", path) + } + foundPluginExecutableFile = true + pluginExecutableFile = p + } + } + return nil + }); err != nil { + return "", "", err + } + if !foundPluginExecutableFile { + return "", "", fmt.Errorf("no valid plugin executable file was found under %s", path) + } + return pluginExecutableFile, pluginName, nil +} diff --git a/plugin/manager_test.go b/plugin/manager_test.go index f3d91a99..f79d8b08 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -157,7 +157,10 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadata), newPluginStdout: metadataJSON(validMetadataHigherVersion), } - existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) if err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } @@ -176,7 +179,11 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadata), newPluginStdout: metadataJSON(validMetadataLowerVersion), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, true) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + Overwrite: true, + } + _, _, err = mgr.Install(context.Background(), installOpts) if err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } @@ -201,7 +208,10 @@ func TestManager_Install(t *testing.T) { newPluginStdout: metadataJSON(validMetadataBar), } defer mgr.Uninstall(context.Background(), "bar") - existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) if err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } @@ -220,7 +230,10 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadata), newPluginStdout: metadataJSON(validMetadata), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "plugin foo with version 1.0.0 already exists" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -234,7 +247,10 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadata), newPluginStdout: metadataJSON(validMetadataLowerVersion), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "the installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -259,7 +275,10 @@ func TestManager_Install(t *testing.T) { newPluginFilePath: newPluginFilePath, newPluginStdout: metadataJSON(validMetadataBar), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get plugin name from file path: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -268,11 +287,26 @@ func TestManager_Install(t *testing.T) { t.Run("fail to install due to invalid new plugin file extension", func(t *testing.T) { newPluginFilePath := "testdata/bar/notation-bar.exe" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } executor = testInstallCommander{ newPluginFilePath: newPluginFilePath, newPluginStdout: metadataJSON(validMetadataBar), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -285,8 +319,11 @@ func TestManager_Install(t *testing.T) { newPluginFilePath: newPluginFilePath, newPluginStdout: metadataJSON(validMetadataBar), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) - expectedErrorMsg := "failed to create new CLI plugin: stat testdata/bar/notation-bar: no such file or directory" + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) + expectedErrorMsg := "failed to validate plugin directory: stat testdata/bar/notation-bar: no such file or directory" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -310,7 +347,10 @@ func TestManager_Install(t *testing.T) { newPluginFilePath: newPluginFilePath, newPluginStdout: metadataJSON(invalidMetadataName), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\"" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -324,7 +364,10 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadataBar), newPluginStdout: metadataJSON(validMetadata), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, false) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -338,7 +381,49 @@ func TestManager_Install(t *testing.T) { existedPluginStdout: metadataJSON(validMetadataBar), newPluginStdout: metadataJSON(validMetadata), } - _, _, err = mgr.Install(context.Background(), newPluginFilePath, true) + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + Overwrite: true, + } + _, _, err = mgr.Install(context.Background(), installOpts) + if err != nil { + t.Fatalf("expecting error to be nil, but got %v", err) + } + }) + + t.Run("success to install from plugin dir", func(t *testing.T) { + existedPluginFilePath := "testdata/plugins/foo/notation-foo" + newPluginFilePath := "testdata/foo/notation-foo" + newPluginLibPath := "testdata/foo/libfoo" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + pluginFile, err := os.Create(newPluginFilePath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } + pluginLibFile, err := os.Create(newPluginLibPath) + if err != nil { + t.Fatalf("failed to create %s: %v", newPluginLibPath, err) + } + if err := pluginLibFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginLibPath, err) + } + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataHigherVersion), + } + installOpts := CLIInstallOptions{ + PluginPath: newPluginDir, + } + _, _, err = mgr.Install(context.Background(), installOpts) if err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 389180fd..2dccf2ad 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -16,8 +16,25 @@ package plugin -import "github.com/notaryproject/notation-go/plugin/proto" +import ( + "errors" + "os" + + "github.com/notaryproject/notation-go/plugin/proto" +) func binName(name string) string { return proto.Prefix + name } + +// isExecutableFile checks if a file at filePath is executable +func isExecutableFile(filePath string) (bool, error) { + fi, err := os.Stat(filePath) + if err != nil { + return false, err + } + if !fi.Mode().IsRegular() { + return false, errors.New("not regular file") + } + return fi.Mode()&0100 != 0, nil +} diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index 3b5bf0be..e9d1dde4 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -13,8 +13,26 @@ package plugin -import "github.com/notaryproject/notation-go/plugin/proto" +import ( + "errors" + "os" + "path/filepath" + + "github.com/notaryproject/notation-go/plugin/proto" +) func binName(name string) string { return proto.Prefix + name + ".exe" } + +// isExecutableFile checks if a file at filePath is executable +func isExecutableFile(filePath string) (bool, error) { + fi, err := os.Stat(filePath) + if err != nil { + return false, err + } + if !fi.Mode().IsRegular() { + return false, errors.New("not regular file") + } + return filepath.Ext(filepath.Base(filePath)) == ".exe", nil +} diff --git a/plugin/plugin.go b/plugin/plugin.go index 349316f2..0afb9ea6 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -225,9 +225,9 @@ func (c execCommander) Output(ctx context.Context, name string, command proto.Co return stdout.Bytes(), nil, nil } -// ExtractPluginNameFromFileName checks if fileName is a valid plugin file name +// ParsePluginName checks if fileName is a valid plugin file name // and gets plugin name from it based on spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md#installation -func ExtractPluginNameFromFileName(fileName string) (string, error) { +func ParsePluginName(fileName string) (string, error) { fname := file.TrimFileExtension(fileName) pluginName, found := strings.CutPrefix(fname, proto.Prefix) if !found { diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 85b8a862..bf592663 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -273,7 +273,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { } func TestExtractPluginNameFromExecutableFileName(t *testing.T) { - pluginName, err := ExtractPluginNameFromFileName("notation-my-plugin") + pluginName, err := ParsePluginName("notation-my-plugin") if err != nil { t.Fatalf("expected nil err, got %v", err) } @@ -281,7 +281,7 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - pluginName, err = ExtractPluginNameFromFileName("notation-my-plugin.exe") + pluginName, err = ParsePluginName("notation-my-plugin.exe") if err != nil { t.Fatalf("expected nil err, got %v", err) } @@ -289,13 +289,13 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected plugin name my-plugin, but got %s", pluginName) } - _, err = ExtractPluginNameFromFileName("myPlugin") + _, err = ParsePluginName("myPlugin") expectedErrorMsg := "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got myPlugin" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } - _, err = ExtractPluginNameFromFileName("my-plugin") + _, err = ParsePluginName("my-plugin") expectedErrorMsg = "invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got my-plugin" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) diff --git a/plugin/testdata/plugins/foo/libfoo b/plugin/testdata/plugins/foo/libfoo new file mode 100644 index 00000000..e69de29b diff --git a/verifier/helpers.go b/verifier/helpers.go index 8e56530e..600d9234 100644 --- a/verifier/helpers.go +++ b/verifier/helpers.go @@ -149,7 +149,7 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string, if strings.TrimSpace(version) == "" { return "", fmt.Errorf("%v from extended attribute is an empty string", HeaderVerificationPluginMinVersion) } - if !notationsemver.IsVersionSemverValid(version) { + if !notationsemver.IsSemverValid(version) { return "", fmt.Errorf("%v from extended attribute is not a valid SemVer", HeaderVerificationPluginMinVersion) } return version, nil diff --git a/verifier/verifier.go b/verifier/verifier.go index 758bd2b5..2a77c531 100644 --- a/verifier/verifier.go +++ b/verifier/verifier.go @@ -232,7 +232,7 @@ func (v *verifier) processSignature(ctx context.Context, sigBlob []byte, envelop pluginVersion := metadata.Version //checking if the plugin version is in valid semver format - if !notationsemver.IsVersionSemverValid(pluginVersion) { + if !notationsemver.IsSemverValid(pluginVersion) { return notation.ErrorVerificationInconclusive{Msg: fmt.Sprintf("plugin %s has pluginVersion %s which is not in valid semver format", verificationPluginName, pluginVersion)} } From b179d54ad2a90a3d5117359694868ab7012b33de Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 21:56:29 +0800 Subject: [PATCH 14/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 54 ++++++++++++++++++++++----------------- plugin/manager_unix.go | 5 ++-- plugin/manager_windows.go | 3 +-- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index c79a5bd1..4b9a2198 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -89,8 +89,10 @@ type CLIInstallOptions struct { // 1. A directory which contains plugin related files. Sub-directories are // ignored. It MUST contain one and only one valid plugin executable file // following spec: https://github.com/notaryproject/specifications/blob/v1.0.0/specs/plugin-extensibility.md#installation + // It may contain extra lib files and LICENSE files. + // On success, these files will be installed as well. // - // 2. A single valid plugin executalbe file. + // 2. A single plugin executalbe file following the spec. PluginPath string // Overwrite is a boolean flag. When set, always install the new plugin. @@ -101,7 +103,7 @@ type CLIInstallOptions struct { // plugin metadata, new plugin metadata, and error. It returns nil error // if and only if the installation succeeded. // -// If plugin does not exist, directly install from PluginPath. +// If plugin does not exist, directly install the new plugin. // // If plugin already exists: // @@ -116,26 +118,27 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if installOpts.PluginPath == "" { return nil, nil, errors.New("plugin path cannot be empty") } - var installFromSingleFile bool - var pluginFile, pluginName string - pluginFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) + var installFromNonDir bool + var pluginExecutableFile, pluginName string + var err error + pluginExecutableFile, pluginName, err = parsePluginFromDir(installOpts.PluginPath) if err != nil { if !errors.Is(err, file.ErrNotDirectory) { - return nil, nil, fmt.Errorf("failed to validate plugin directory: %w", err) + return nil, nil, fmt.Errorf("failed to parse plugin from directory %s: %w", installOpts.PluginPath, err) } // input is not a dir - installFromSingleFile = true - pluginFile = installOpts.PluginPath - pluginName, err = ParsePluginName(filepath.Base(pluginFile)) + installFromNonDir = true + pluginExecutableFile = installOpts.PluginPath + pluginName, err = ParsePluginName(filepath.Base(pluginExecutableFile)) if err != nil { - return nil, nil, fmt.Errorf("failed to get plugin name from file path: %w", err) + return nil, nil, fmt.Errorf("failed to get plugin name from file path %s: %w", pluginExecutableFile, err) } } // validate and get new plugin metadata - if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginFile), pluginName); err != nil { + if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { return nil, nil, err } - newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginFile) + newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) } @@ -151,12 +154,12 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to check plugin existence: %w", err) } } else { // plugin already exists - var err error existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) if err != nil && !overwrite { // fail only if overwrite is not set return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) } - if !overwrite { // err is nil, and overwrite is not set, check version + // existing plugin is valid, and overwrite is not set, check version + if !overwrite { comp, err := semver.ComparePluginVersion(newPluginMetadata.Version, existingPluginMetadata.Version) if err != nil { return nil, nil, fmt.Errorf("failed to compare plugin versions: %w", err) @@ -173,9 +176,9 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - if installFromSingleFile { - if err := file.CopyToDir(pluginFile, pluginDirPath); err != nil { - return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginFile, pluginDirPath, err) + if installFromNonDir { + if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { + return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginExecutableFile, pluginDirPath, err) } } else { if err := file.CopyDirToDir(installOpts.PluginPath, pluginDirPath); err != nil { @@ -183,7 +186,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // plugin binary file is always executable - pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginFile)) + pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginExecutableFile)) if err := os.Chmod(pluginFilePath, 0700); err != nil { return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) } @@ -204,11 +207,13 @@ func (m *CLIManager) Uninstall(ctx context.Context, name string) error { } // parsePluginFromDir checks if a dir is a valid plugin dir which contains -// one and only one valid plugin executable file. Sub-directories are ignored. +// one and only one plugin executable file. The dir may contain extra lib files +// and LICENSE files. Sub-directories are ignored. // // On success, the plugin executable file path, plugin name and // nil error are returned. func parsePluginFromDir(path string) (string, string, error) { + // sanity check fi, err := os.Stat(path) if err != nil { return "", "", err @@ -217,8 +222,7 @@ func parsePluginFromDir(path string) (string, string, error) { return "", "", file.ErrNotDirectory } // walk the path - var pluginExecutableFile string - var pluginName string + var pluginExecutableFile, pluginName string var foundPluginExecutableFile bool if err := filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { if err != nil { @@ -232,9 +236,11 @@ func parsePluginFromDir(path string) (string, string, error) { if err != nil { return err } + // only take regular files if info.Mode().IsRegular() { if pluginName, err = ParsePluginName(d.Name()); err != nil { - // file name does not follow the notation-{plugin-name} format + // file name does not follow the notation-{plugin-name} format, + // continue return nil } isExec, err := isExecutableFile(p) @@ -243,7 +249,7 @@ func parsePluginFromDir(path string) (string, string, error) { } if isExec { if foundPluginExecutableFile { - return fmt.Errorf("found more than one valid plugin executable files under %s", path) + return fmt.Errorf("found more than one plugin executable files under %s", path) } foundPluginExecutableFile = true pluginExecutableFile = p @@ -254,7 +260,7 @@ func parsePluginFromDir(path string) (string, string, error) { return "", "", err } if !foundPluginExecutableFile { - return "", "", fmt.Errorf("no valid plugin executable file was found under %s", path) + return "", "", fmt.Errorf("no plugin executable file was found under %s", path) } return pluginExecutableFile, pluginName, nil } diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index 2dccf2ad..b0b74bb1 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -17,7 +17,6 @@ package plugin import ( - "errors" "os" "github.com/notaryproject/notation-go/plugin/proto" @@ -27,14 +26,14 @@ func binName(name string) string { return proto.Prefix + name } -// isExecutableFile checks if a file at filePath is executable +// isExecutableFile checks if a file at filePath is user executable func isExecutableFile(filePath string) (bool, error) { fi, err := os.Stat(filePath) if err != nil { return false, err } if !fi.Mode().IsRegular() { - return false, errors.New("not regular file") + return false, ErrNotRegularFile } return fi.Mode()&0100 != 0, nil } diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index e9d1dde4..c8cc8cca 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -14,7 +14,6 @@ package plugin import ( - "errors" "os" "path/filepath" @@ -32,7 +31,7 @@ func isExecutableFile(filePath string) (bool, error) { return false, err } if !fi.Mode().IsRegular() { - return false, errors.New("not regular file") + return false, ErrNotRegularFile } return filepath.Ext(filepath.Base(filePath)) == ".exe", nil } From c44b73805588ff331b3990616d310a5df32d8a7f Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 21:59:29 +0800 Subject: [PATCH 15/36] update Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index f79d8b08..1344ace4 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -279,7 +279,7 @@ func TestManager_Install(t *testing.T) { PluginPath: newPluginFilePath, } _, _, err = mgr.Install(context.Background(), installOpts) - expectedErrorMsg := "failed to get plugin name from file path: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + expectedErrorMsg := "failed to get plugin name from file path testdata/bar/bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -323,7 +323,7 @@ func TestManager_Install(t *testing.T) { PluginPath: newPluginFilePath, } _, _, err = mgr.Install(context.Background(), installOpts) - expectedErrorMsg := "failed to validate plugin directory: stat testdata/bar/notation-bar: no such file or directory" + expectedErrorMsg := "failed to parse plugin from directory testdata/bar/notation-bar: stat testdata/bar/notation-bar: no such file or directory" if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } From c9aa8f1fb36e8f4dad26a0a753792e19014411ec Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 22:20:17 +0800 Subject: [PATCH 16/36] fix tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 1344ace4..f14c9f82 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -407,6 +407,9 @@ func TestManager_Install(t *testing.T) { if err := pluginFile.Close(); err != nil { t.Fatalf("failed to close %s: %v", newPluginFilePath, err) } + if err := pluginFile.Chmod(0700); err != nil { + t.Fatalf("failed to chmod %s: %v", newPluginFilePath, err) + } pluginLibFile, err := os.Create(newPluginLibPath) if err != nil { t.Fatalf("failed to create %s: %v", newPluginLibPath, err) From 6fb5ac66707537d79994096a3f37b4dffdaabd4d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 11 Dec 2023 22:22:15 +0800 Subject: [PATCH 17/36] fix test Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index f14c9f82..1d6e53df 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -404,12 +404,12 @@ func TestManager_Install(t *testing.T) { if err != nil { t.Fatalf("failed to create %s: %v", newPluginFilePath, err) } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) - } if err := pluginFile.Chmod(0700); err != nil { t.Fatalf("failed to chmod %s: %v", newPluginFilePath, err) } + if err := pluginFile.Close(); err != nil { + t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + } pluginLibFile, err := os.Create(newPluginLibPath) if err != nil { t.Fatalf("failed to create %s: %v", newPluginLibPath, err) From 3f4aa318a5739ccb78f1fdf3a17dc0653edbba04 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 08:47:38 +0800 Subject: [PATCH 18/36] add tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 146 +++++++++++++++++++++++++++-------------- 1 file changed, 97 insertions(+), 49 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 1d6e53df..8bc4eeda 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -141,12 +141,8 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) - } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + t.Fatal(err) } mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "testdata/plugins")) @@ -183,8 +179,7 @@ func TestManager_Install(t *testing.T) { PluginPath: newPluginFilePath, Overwrite: true, } - _, _, err = mgr.Install(context.Background(), installOpts) - if err != nil { + if _, _, err := mgr.Install(context.Background(), installOpts); err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } }) @@ -196,12 +191,8 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) - } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + t.Fatal(err) } executor = testInstallCommander{ newPluginFilePath: newPluginFilePath, @@ -233,8 +224,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "plugin foo with version 1.0.0 already exists" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -250,8 +241,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "the installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -264,12 +255,8 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) - } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + t.Fatal(err) } executor = testInstallCommander{ newPluginFilePath: newPluginFilePath, @@ -278,8 +265,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get plugin name from file path testdata/bar/bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -292,12 +279,8 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) - } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + t.Fatal(err) } executor = testInstallCommander{ newPluginFilePath: newPluginFilePath, @@ -306,8 +289,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -322,8 +305,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to parse plugin from directory testdata/bar/notation-bar: stat testdata/bar/notation-bar: no such file or directory" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -350,8 +333,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\"" + _, _, err = mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -367,8 +350,8 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - _, _, err = mgr.Install(context.Background(), installOpts) expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -385,7 +368,7 @@ func TestManager_Install(t *testing.T) { PluginPath: newPluginFilePath, Overwrite: true, } - _, _, err = mgr.Install(context.Background(), installOpts) + _, _, err := mgr.Install(context.Background(), installOpts) if err != nil { t.Fatalf("expecting error to be nil, but got %v", err) } @@ -400,22 +383,44 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) } - if err := pluginFile.Chmod(0700); err != nil { - t.Fatalf("failed to chmod %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginLibPath, 0666); err != nil { + t.Fatal(err) } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataHigherVersion), } - pluginLibFile, err := os.Create(newPluginLibPath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginLibPath, err) + installOpts := CLIInstallOptions{ + PluginPath: newPluginDir, + } + expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" + existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + if existingPluginMetadata.Version != "1.0.0" { + t.Fatalf("expecting existing plugin metadata to be 1.0.0, but got %s", existingPluginMetadata.Version) + } + if newPluginMetadata.Version != "1.1.0" { + t.Fatalf("expecting new plugin metadata to be 1.1.0, but got %s", newPluginMetadata.Version) } - if err := pluginLibFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginLibPath, err) + }) + + t.Run("fail to install from plugin dir due to no plugin executable file", func(t *testing.T) { + existedPluginFilePath := "testdata/plugins/foo/notation-foo" + newPluginFilePath := "testdata/foo/foo" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + t.Fatal(err) } executor = testInstallCommander{ existedPluginFilePath: existedPluginFilePath, @@ -426,9 +431,41 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - _, _, err = mgr.Install(context.Background(), installOpts) - if err != nil { - t.Fatalf("expecting error to be nil, but got %v", err) + expectedErrorMsg := "no plugin executable file was found under testdata/foo" + _, _, err := mgr.Install(context.Background(), installOpts) + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + }) + + t.Run("fail to install from plugin dir due to more than one plugin executable file", func(t *testing.T) { + existedPluginFilePath := "testdata/plugins/foo/notation-foo" + newPluginFilePath := "testdata/foo/notation-foo1" + newPluginFilePath2 := "testdata/foo/notation-foo2" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) + } + if err := createFileAndChmod(newPluginFilePath2, 0700); err != nil { + t.Fatal(err) + } + executor = testInstallCommander{ + existedPluginFilePath: existedPluginFilePath, + newPluginFilePath: newPluginFilePath, + existedPluginStdout: metadataJSON(validMetadata), + newPluginStdout: metadataJSON(validMetadataHigherVersion), + } + installOpts := CLIInstallOptions{ + PluginPath: newPluginDir, + } + expectedErrorMsg := "found more than one plugin executable files under testdata/foo" + _, _, err := mgr.Install(context.Background(), installOpts) + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } }) } @@ -468,3 +505,14 @@ func metadataJSON(m proto.GetMetadataResponse) []byte { } return d } + +func createFileAndChmod(path string, mode fs.FileMode) error { + f, err := os.Create(path) + if err != nil { + return err + } + if err := f.Chmod(mode); err != nil { + return err + } + return f.Close() +} From dd240437a0e4613f65578a10c234c0a28d1da40e Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 08:53:13 +0800 Subject: [PATCH 19/36] fix tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 8bc4eeda..2ae032d9 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -398,10 +398,9 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\"" existingPluginMetadata, newPluginMetadata, err := mgr.Install(context.Background(), installOpts) - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + if err != nil { + t.Fatalf("expecting nil error, but got %v", err) } if existingPluginMetadata.Version != "1.0.0" { t.Fatalf("expecting existing plugin metadata to be 1.0.0, but got %s", existingPluginMetadata.Version) @@ -431,7 +430,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "no plugin executable file was found under testdata/foo" + expectedErrorMsg := "failed to parse plugin from directory testdata/foo: no plugin executable file was found under testdata/foo" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -462,7 +461,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "found more than one plugin executable files under testdata/foo" + expectedErrorMsg := "failed to parse plugin from directory testdata/foo: found more than one plugin executable files under testdata/foo" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From 1fca1b4af09fa8d1b0b60609759abac161ce71db Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 17:30:25 +0800 Subject: [PATCH 20/36] update error msg Signed-off-by: Patrick Zheng --- plugin/manager.go | 4 ++-- plugin/manager_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 4b9a2198..81df53c0 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -249,7 +249,7 @@ func parsePluginFromDir(path string) (string, string, error) { } if isExec { if foundPluginExecutableFile { - return fmt.Errorf("found more than one plugin executable files under %s", path) + return errors.New("found more than one plugin executable files") } foundPluginExecutableFile = true pluginExecutableFile = p @@ -260,7 +260,7 @@ func parsePluginFromDir(path string) (string, string, error) { return "", "", err } if !foundPluginExecutableFile { - return "", "", fmt.Errorf("no plugin executable file was found under %s", path) + return "", "", errors.New("no plugin executable file was found") } return pluginExecutableFile, pluginName, nil } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 2ae032d9..dd0d9baf 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -430,7 +430,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to parse plugin from directory testdata/foo: no plugin executable file was found under testdata/foo" + expectedErrorMsg := "failed to parse plugin from directory testdata/foo: no plugin executable file was found" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -461,7 +461,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to parse plugin from directory testdata/foo: found more than one plugin executable files under testdata/foo" + expectedErrorMsg := "failed to parse plugin from directory testdata/foo: found more than one plugin executable files" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From e8ee2ba0613d3fe245f82256b1b05d72902b3d48 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 18:04:33 +0800 Subject: [PATCH 21/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 12 ++++++++---- plugin/manager_test.go | 4 ++-- plugin/plugin.go | 13 ------------- plugin/plugin_test.go | 27 --------------------------- 4 files changed, 10 insertions(+), 46 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 81df53c0..aada7ad6 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -126,18 +126,22 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if !errors.Is(err, file.ErrNotDirectory) { return nil, nil, fmt.Errorf("failed to parse plugin from directory %s: %w", installOpts.PluginPath, err) } - // input is not a dir + // input is not a dir, check if it's a single plugin executable file installFromNonDir = true pluginExecutableFile = installOpts.PluginPath pluginName, err = ParsePluginName(filepath.Base(pluginExecutableFile)) if err != nil { return nil, nil, fmt.Errorf("failed to get plugin name from file path %s: %w", pluginExecutableFile, err) } + isExec, err := isExecutableFile(pluginExecutableFile) + if err != nil { + return nil, nil, fmt.Errorf("failed to check if file %s is executable: %w", pluginExecutableFile, err) + } + if !isExec { + return nil, nil, fmt.Errorf("file %s is not executable: %w", pluginExecutableFile, err) + } } // validate and get new plugin metadata - if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { - return nil, nil, err - } newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index dd0d9baf..6e819a1c 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -279,7 +279,7 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0066); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -289,7 +289,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" + expectedErrorMsg := "file notation-bar.exe is not executable" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) diff --git a/plugin/plugin.go b/plugin/plugin.go index 0afb9ea6..5abdc3a4 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -264,16 +264,3 @@ func validate(metadata *proto.GetMetadataResponse) error { } return nil } - -// validatePluginFileExtensionAgainstOS validates if plugin executable file -// extension aligns with the runtime OS. -// -// On windows, `.exe` extension is required. -// On other OS, MUST not have the `.exe` extension. -func validatePluginFileExtensionAgainstOS(fileName, pluginName string) error { - expectedPluginFile := binName(pluginName) - if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) { - return fmt.Errorf("invalid plugin file extension. Expecting file %s, but got %s", expectedPluginFile, fileName) - } - return nil -} diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index bf592663..df9166ba 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -20,7 +20,6 @@ import ( "fmt" "os" "reflect" - "runtime" "strconv" "strings" "testing" @@ -301,29 +300,3 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } } - -func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { - if runtime.GOOS == "windows" { - err := validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFileExtensionAgainstOS("notation-foo", "foo") - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } - return - } - err := validatePluginFileExtensionAgainstOS("notation-foo", "foo") - if err != nil { - t.Fatalf("expecting nil error, but got %s", err) - } - - err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") - expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe" - if err == nil || err.Error() != expectedErrorMsg { - t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) - } -} From 79ea6b7d0ff7f0cb3294e8275231a036fcdc23ee Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 18:25:19 +0800 Subject: [PATCH 22/36] fix tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 6e819a1c..12c1afcc 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -141,7 +141,7 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { t.Fatal(err) } mgr := NewCLIManager(mockfs.NewSysFSWithRootMock(fstest.MapFS{}, "testdata/plugins")) @@ -191,7 +191,7 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -255,7 +255,7 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -272,14 +272,14 @@ func TestManager_Install(t *testing.T) { } }) - t.Run("fail to install due to invalid new plugin file extension", func(t *testing.T) { - newPluginFilePath := "testdata/bar/notation-bar.exe" + t.Run("fail to install due to wrong plugin file permission", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar" newPluginDir := filepath.Dir(newPluginFilePath) if err := os.MkdirAll(newPluginDir, 0777); err != nil { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0066); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0600); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -289,7 +289,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "file notation-bar.exe is not executable" + expectedErrorMsg := "file notation-bar is not executable" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -319,12 +319,8 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - pluginFile, err := os.Create(newPluginFilePath) - if err != nil { - t.Fatalf("failed to create %s: %v", newPluginFilePath, err) - } - if err := pluginFile.Close(); err != nil { - t.Fatalf("failed to close %s: %v", newPluginFilePath, err) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) } executor = testInstallCommander{ newPluginFilePath: newPluginFilePath, @@ -334,7 +330,7 @@ func TestManager_Install(t *testing.T) { PluginPath: newPluginFilePath, } expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\"" - _, _, err = mgr.Install(context.Background(), installOpts) + _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) } @@ -386,7 +382,7 @@ func TestManager_Install(t *testing.T) { if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { t.Fatal(err) } - if err := createFileAndChmod(newPluginLibPath, 0666); err != nil { + if err := createFileAndChmod(newPluginLibPath, 0600); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -418,7 +414,7 @@ func TestManager_Install(t *testing.T) { t.Fatalf("failed to create %s: %v", newPluginDir, err) } defer os.RemoveAll(newPluginDir) - if err := createFileAndChmod(newPluginFilePath, 0666); err != nil { + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { t.Fatal(err) } executor = testInstallCommander{ @@ -437,7 +433,7 @@ func TestManager_Install(t *testing.T) { } }) - t.Run("fail to install from plugin dir due to more than one plugin executable file", func(t *testing.T) { + t.Run("fail to install from plugin dir due to more than one plugin executable files", func(t *testing.T) { existedPluginFilePath := "testdata/plugins/foo/notation-foo" newPluginFilePath := "testdata/foo/notation-foo1" newPluginFilePath2 := "testdata/foo/notation-foo2" From a611ee9b2c9ebb50afded2cc7af2c03c1a6f2f73 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 18:29:13 +0800 Subject: [PATCH 23/36] fix tests Signed-off-by: Patrick Zheng --- plugin/manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 12c1afcc..d50270a5 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -289,7 +289,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "file notation-bar is not executable" + expectedErrorMsg := "file testdata/bar/notation-bar is not executable" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From edd0166ca327551aeba3a5854b408f7509a23ab6 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 18:31:45 +0800 Subject: [PATCH 24/36] fix tests Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index aada7ad6..fd115d8c 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -138,7 +138,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to check if file %s is executable: %w", pluginExecutableFile, err) } if !isExec { - return nil, nil, fmt.Errorf("file %s is not executable: %w", pluginExecutableFile, err) + return nil, nil, fmt.Errorf("file %s is not executable", pluginExecutableFile) } } // validate and get new plugin metadata From 50ba6749b57c8d87a8e555cf0409ac95a01da94c Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 19:11:39 +0800 Subject: [PATCH 25/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 3 +++ plugin/manager_test.go | 24 ++++++++++++++++++++++++ plugin/plugin.go | 13 +++++++++++++ plugin/plugin_test.go | 27 +++++++++++++++++++++++++++ 4 files changed, 67 insertions(+) diff --git a/plugin/manager.go b/plugin/manager.go index fd115d8c..6b4c38b2 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -142,6 +142,9 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } // validate and get new plugin metadata + if err := validatePluginFileExtensionAgainstOS(filepath.Base(pluginExecutableFile), pluginName); err != nil { + return nil, nil, err + } newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile) if err != nil { return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index d50270a5..d00cf5b7 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -296,6 +296,30 @@ func TestManager_Install(t *testing.T) { } }) + t.Run("fail to install due to invalid new plugin file extension", func(t *testing.T) { + newPluginFilePath := "testdata/bar/notation-bar.exe" + newPluginDir := filepath.Dir(newPluginFilePath) + if err := os.MkdirAll(newPluginDir, 0777); err != nil { + t.Fatalf("failed to create %s: %v", newPluginDir, err) + } + defer os.RemoveAll(newPluginDir) + if err := createFileAndChmod(newPluginFilePath, 0700); err != nil { + t.Fatal(err) + } + executor = testInstallCommander{ + newPluginFilePath: newPluginFilePath, + newPluginStdout: metadataJSON(validMetadataBar), + } + installOpts := CLIInstallOptions{ + PluginPath: newPluginFilePath, + } + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-bar, but got notation-bar.exe" + _, _, err := mgr.Install(context.Background(), installOpts) + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + }) + t.Run("fail to install due to new plugin executable file does not exist", func(t *testing.T) { newPluginFilePath := "testdata/bar/notation-bar" executor = testInstallCommander{ diff --git a/plugin/plugin.go b/plugin/plugin.go index 5abdc3a4..0afb9ea6 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -264,3 +264,16 @@ func validate(metadata *proto.GetMetadataResponse) error { } return nil } + +// validatePluginFileExtensionAgainstOS validates if plugin executable file +// extension aligns with the runtime OS. +// +// On windows, `.exe` extension is required. +// On other OS, MUST not have the `.exe` extension. +func validatePluginFileExtensionAgainstOS(fileName, pluginName string) error { + expectedPluginFile := binName(pluginName) + if filepath.Ext(fileName) != filepath.Ext(expectedPluginFile) { + return fmt.Errorf("invalid plugin file extension. Expecting file %s, but got %s", expectedPluginFile, fileName) + } + return nil +} diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index df9166ba..bf592663 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "reflect" + "runtime" "strconv" "strings" "testing" @@ -300,3 +301,29 @@ func TestExtractPluginNameFromExecutableFileName(t *testing.T) { t.Fatalf("expected %s, got %v", expectedErrorMsg, err) } } + +func TestValidatePluginFileExtensionAgainstOS(t *testing.T) { + if runtime.GOOS == "windows" { + err := validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo", "foo") + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo.exe, but got notation-foo" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } + return + } + err := validatePluginFileExtensionAgainstOS("notation-foo", "foo") + if err != nil { + t.Fatalf("expecting nil error, but got %s", err) + } + + err = validatePluginFileExtensionAgainstOS("notation-foo.exe", "foo") + expectedErrorMsg := "invalid plugin file extension. Expecting file notation-foo, but got notation-foo.exe" + if err == nil || err.Error() != expectedErrorMsg { + t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) + } +} From 90bfb955ecb329e54757f9864d36467c5a315965 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 20:20:45 +0800 Subject: [PATCH 26/36] added clean up before installation Signed-off-by: Patrick Zheng --- plugin/manager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugin/manager.go b/plugin/manager.go index 6b4c38b2..83d1d063 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -179,10 +179,15 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } } + // core process pluginDirPath, err := m.pluginFS.SysPath(pluginName) if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } + // clean up before installation + if err := os.RemoveAll(pluginDirPath); err != nil { + return nil, nil, fmt.Errorf("failed to clean up %s before installation: %w", pluginDirPath, err) + } if installFromNonDir { if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginExecutableFile, pluginDirPath, err) From c8b1d6e74fbefda7270b4b011723df2a48b508a0 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Tue, 12 Dec 2023 20:23:25 +0800 Subject: [PATCH 27/36] updated comments Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/manager.go b/plugin/manager.go index 83d1d063..48753626 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -184,7 +184,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - // clean up before installation + // clean up before installation, this guarantees idempotent for install if err := os.RemoveAll(pluginDirPath); err != nil { return nil, nil, fmt.Errorf("failed to clean up %s before installation: %w", pluginDirPath, err) } From f93996479e60f160b79feaa94e56f945df20c65b Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 14 Dec 2023 16:35:31 +0800 Subject: [PATCH 28/36] added clean up on failure Signed-off-by: Patrick Zheng --- plugin/manager.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 48753626..06ebacb3 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -154,6 +154,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to get metadata of new plugin: %w", err) } // check plugin existence and get existing plugin metadata + var pluginExists bool var existingPluginMetadata *proto.GetMetadataResponse existingPlugin, err := m.Get(ctx, pluginName) if err != nil { @@ -161,6 +162,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to check plugin existence: %w", err) } } else { // plugin already exists + pluginExists = true existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) if err != nil && !overwrite { // fail only if overwrite is not set return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) @@ -184,24 +186,33 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - // clean up before installation, this guarantees idempotent for install - if err := os.RemoveAll(pluginDirPath); err != nil { - return nil, nil, fmt.Errorf("failed to clean up %s before installation: %w", pluginDirPath, err) + // back up existing plugin + if pluginExists { + if err := os.Rename(pluginDirPath, pluginDirPath+".bak"); err != nil { + return nil, nil, fmt.Errorf("failed to backup existing plugin %s before installation: %w", pluginDirPath, err) + } } if installFromNonDir { if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { + defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginExecutableFile, pluginDirPath, err) } } else { if err := file.CopyDirToDir(installOpts.PluginPath, pluginDirPath); err != nil { + defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to copy plugin files from %s to %s: %w", installOpts.PluginPath, pluginDirPath, err) } } // plugin binary file is always executable pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginExecutableFile)) if err := os.Chmod(pluginFilePath, 0700); err != nil { + defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) } + // on success, remove the old plugin's back-up if eixsts + if pluginExists { + defer os.RemoveAll(pluginDirPath + ".bak") + } return existingPluginMetadata, newPluginMetadata, nil } @@ -276,3 +287,15 @@ func parsePluginFromDir(path string) (string, string, error) { } return pluginExecutableFile, pluginName, nil } + +// cleanupOnInstallFailure is called when CLIManager.Install failed at core +// process, in this case, the plugin's original stage is recovered. +func cleanupOnInstallFailure(pluginDirPath string, pluginBackupPath string, pluginExists bool) error { + // clean up on failure + _ = os.RemoveAll(pluginDirPath) + // recover if old plugin exists + if pluginExists { + return os.Rename(pluginBackupPath, pluginDirPath) + } + return nil +} From c7ded4a8a2da943c261844f5d5557e4396286cd6 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 14 Dec 2023 17:10:59 +0800 Subject: [PATCH 29/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 29 +++-------------------------- 1 file changed, 3 insertions(+), 26 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 06ebacb3..48753626 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -154,7 +154,6 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to get metadata of new plugin: %w", err) } // check plugin existence and get existing plugin metadata - var pluginExists bool var existingPluginMetadata *proto.GetMetadataResponse existingPlugin, err := m.Get(ctx, pluginName) if err != nil { @@ -162,7 +161,6 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to check plugin existence: %w", err) } } else { // plugin already exists - pluginExists = true existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) if err != nil && !overwrite { // fail only if overwrite is not set return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) @@ -186,33 +184,24 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) if err != nil { return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } - // back up existing plugin - if pluginExists { - if err := os.Rename(pluginDirPath, pluginDirPath+".bak"); err != nil { - return nil, nil, fmt.Errorf("failed to backup existing plugin %s before installation: %w", pluginDirPath, err) - } + // clean up before installation, this guarantees idempotent for install + if err := os.RemoveAll(pluginDirPath); err != nil { + return nil, nil, fmt.Errorf("failed to clean up %s before installation: %w", pluginDirPath, err) } if installFromNonDir { if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { - defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginExecutableFile, pluginDirPath, err) } } else { if err := file.CopyDirToDir(installOpts.PluginPath, pluginDirPath); err != nil { - defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to copy plugin files from %s to %s: %w", installOpts.PluginPath, pluginDirPath, err) } } // plugin binary file is always executable pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginExecutableFile)) if err := os.Chmod(pluginFilePath, 0700); err != nil { - defer cleanupOnInstallFailure(pluginDirPath, pluginDirPath+".bak", pluginExists) return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) } - // on success, remove the old plugin's back-up if eixsts - if pluginExists { - defer os.RemoveAll(pluginDirPath + ".bak") - } return existingPluginMetadata, newPluginMetadata, nil } @@ -287,15 +276,3 @@ func parsePluginFromDir(path string) (string, string, error) { } return pluginExecutableFile, pluginName, nil } - -// cleanupOnInstallFailure is called when CLIManager.Install failed at core -// process, in this case, the plugin's original stage is recovered. -func cleanupOnInstallFailure(pluginDirPath string, pluginBackupPath string, pluginExists bool) error { - // clean up on failure - _ = os.RemoveAll(pluginDirPath) - // recover if old plugin exists - if pluginExists { - return os.Rename(pluginBackupPath, pluginDirPath) - } - return nil -} From d6eb530fa32aaf67260fcee96dc74bd8390df128 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 14 Dec 2023 18:58:01 +0800 Subject: [PATCH 30/36] update Signed-off-by: Patrick Zheng --- plugin/manager.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 48753626..f92e50f8 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -92,7 +92,7 @@ type CLIInstallOptions struct { // It may contain extra lib files and LICENSE files. // On success, these files will be installed as well. // - // 2. A single plugin executalbe file following the spec. + // 2. A single plugin executable file following the spec. PluginPath string // Overwrite is a boolean flag. When set, always install the new plugin. @@ -119,9 +119,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, errors.New("plugin path cannot be empty") } var installFromNonDir bool - var pluginExecutableFile, pluginName string - var err error - pluginExecutableFile, pluginName, err = parsePluginFromDir(installOpts.PluginPath) + pluginExecutableFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) if err != nil { if !errors.Is(err, file.ErrNotDirectory) { return nil, nil, fmt.Errorf("failed to parse plugin from directory %s: %w", installOpts.PluginPath, err) From f623ed96bb52dbddc310567c368f5f2fc74e9cc5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 15 Dec 2023 10:38:34 +0800 Subject: [PATCH 31/36] updated per code review Signed-off-by: Patrick Zheng --- plugin/manager.go | 11 ++++++----- plugin/manager_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index f92e50f8..28b98bc8 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -116,20 +116,20 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) // initialization overwrite := installOpts.Overwrite if installOpts.PluginPath == "" { - return nil, nil, errors.New("plugin path cannot be empty") + return nil, nil, errors.New("plugin source path cannot be empty") } var installFromNonDir bool pluginExecutableFile, pluginName, err := parsePluginFromDir(installOpts.PluginPath) if err != nil { if !errors.Is(err, file.ErrNotDirectory) { - return nil, nil, fmt.Errorf("failed to parse plugin from directory %s: %w", installOpts.PluginPath, err) + return nil, nil, fmt.Errorf("failed to read plugin from directory %s: %w", installOpts.PluginPath, err) } // input is not a dir, check if it's a single plugin executable file installFromNonDir = true pluginExecutableFile = installOpts.PluginPath pluginName, err = ParsePluginName(filepath.Base(pluginExecutableFile)) if err != nil { - return nil, nil, fmt.Errorf("failed to get plugin name from file path %s: %w", pluginExecutableFile, err) + return nil, nil, fmt.Errorf("failed to read plugin name from file path %s: %w", pluginExecutableFile, err) } isExec, err := isExecutableFile(pluginExecutableFile) if err != nil { @@ -155,7 +155,8 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) var existingPluginMetadata *proto.GetMetadataResponse existingPlugin, err := m.Get(ctx, pluginName) if err != nil { - if !errors.Is(err, os.ErrNotExist) { + // fail only if overwrite is not set + if !errors.Is(err, os.ErrNotExist) && !overwrite { return nil, nil, fmt.Errorf("failed to check plugin existence: %w", err) } } else { // plugin already exists @@ -203,7 +204,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return existingPluginMetadata, newPluginMetadata, nil } -// Uninstall uninstalls a plugin on the system by its name +// Uninstall uninstalls a plugin on the system by its name. // If the plugin dir does not exist, os.ErrNotExist is returned. func (m *CLIManager) Uninstall(ctx context.Context, name string) error { pluginDirPath, err := m.pluginFS.SysPath(name) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index d00cf5b7..93518570 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -265,7 +265,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to get plugin name from file path testdata/bar/bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" + expectedErrorMsg := "failed to read plugin name from file path testdata/bar/bar: invalid plugin executable file name. Plugin file name requires format notation-{plugin-name}, but got bar" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -329,7 +329,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "failed to parse plugin from directory testdata/bar/notation-bar: stat testdata/bar/notation-bar: no such file or directory" + expectedErrorMsg := "failed to read plugin from directory testdata/bar/notation-bar: stat testdata/bar/notation-bar: no such file or directory" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -450,7 +450,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to parse plugin from directory testdata/foo: no plugin executable file was found" + expectedErrorMsg := "failed to read plugin from directory testdata/foo: no plugin executable file was found" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) @@ -481,7 +481,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginDir, } - expectedErrorMsg := "failed to parse plugin from directory testdata/foo: found more than one plugin executable files" + expectedErrorMsg := "failed to read plugin from directory testdata/foo: found more than one plugin executable files" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err) From f339741d5dcefb89aed20e34094c45000b098f4f Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 15 Dec 2023 13:35:04 +0800 Subject: [PATCH 32/36] updated per code review Signed-off-by: Patrick Zheng --- internal/semver/semver.go | 22 +++++++++------------- verifier/helpers.go | 2 +- verifier/verifier.go | 2 +- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/internal/semver/semver.go b/internal/semver/semver.go index 6d914558..9c9c8e5d 100644 --- a/internal/semver/semver.go +++ b/internal/semver/semver.go @@ -17,19 +17,17 @@ package semver import ( "fmt" - "strings" + "regexp" "golang.org/x/mod/semver" ) -// IsSemverValid returns true if version is a valid semantic version -func IsSemverValid(version string) bool { - // a valid semanic version MUST not have prefix 'v' - if strings.HasPrefix(version, "v") { - return false - } - // golang package "golang.org/x/mod/semver" requires prefix 'v' - return semver.IsValid("v" + version) +// semVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string +var semVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) + +// IsValidSemver returns true if version is a valid semantic version +func IsValidSemver(version string) bool { + return semVerRegEx.MatchString(version) } // ComparePluginVersion validates and compares two plugin semantic versions. @@ -37,12 +35,10 @@ func IsSemverValid(version string) bool { // The result will be 0 if v == w, -1 if v < w, or +1 if v > w. func ComparePluginVersion(v, w string) (int, error) { // sanity check - // a valid semantic version should not have prefix `v` - // Reference: https://semver.org/#semantic-versioning-200 - if !IsSemverValid(v) { + if !IsValidSemver(v) { return 0, fmt.Errorf("%s is not a valid semantic version", v) } - if !IsSemverValid(w) { + if !IsValidSemver(w) { return 0, fmt.Errorf("%s is not a valid semantic version", w) } diff --git a/verifier/helpers.go b/verifier/helpers.go index 600d9234..5b34440e 100644 --- a/verifier/helpers.go +++ b/verifier/helpers.go @@ -149,7 +149,7 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string, if strings.TrimSpace(version) == "" { return "", fmt.Errorf("%v from extended attribute is an empty string", HeaderVerificationPluginMinVersion) } - if !notationsemver.IsSemverValid(version) { + if !notationsemver.IsValidSemver(version) { return "", fmt.Errorf("%v from extended attribute is not a valid SemVer", HeaderVerificationPluginMinVersion) } return version, nil diff --git a/verifier/verifier.go b/verifier/verifier.go index 2a77c531..617cd7b5 100644 --- a/verifier/verifier.go +++ b/verifier/verifier.go @@ -232,7 +232,7 @@ func (v *verifier) processSignature(ctx context.Context, sigBlob []byte, envelop pluginVersion := metadata.Version //checking if the plugin version is in valid semver format - if !notationsemver.IsSemverValid(pluginVersion) { + if !notationsemver.IsValidSemver(pluginVersion) { return notation.ErrorVerificationInconclusive{Msg: fmt.Sprintf("plugin %s has pluginVersion %s which is not in valid semver format", verificationPluginName, pluginVersion)} } From c16b6c3cbc1329b87a85cebede15194e08dd8a0d Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 15 Dec 2023 16:35:07 +0800 Subject: [PATCH 33/36] updated per code review Signed-off-by: Patrick Zheng --- internal/semver/semver.go | 10 +++++----- plugin/manager.go | 19 +++++++++++-------- plugin/manager_unix.go | 5 +++-- plugin/manager_windows.go | 3 ++- verifier/helpers.go | 2 +- verifier/verifier.go | 2 +- 6 files changed, 23 insertions(+), 18 deletions(-) diff --git a/internal/semver/semver.go b/internal/semver/semver.go index 9c9c8e5d..e2eb3406 100644 --- a/internal/semver/semver.go +++ b/internal/semver/semver.go @@ -22,11 +22,11 @@ import ( "golang.org/x/mod/semver" ) -// semVerRegEx is takenfrom https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string +// semVerRegEx is taken from https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string var semVerRegEx = regexp.MustCompile(`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`) -// IsValidSemver returns true if version is a valid semantic version -func IsValidSemver(version string) bool { +// IsValid returns true if version is a valid semantic version +func IsValid(version string) bool { return semVerRegEx.MatchString(version) } @@ -35,10 +35,10 @@ func IsValidSemver(version string) bool { // The result will be 0 if v == w, -1 if v < w, or +1 if v > w. func ComparePluginVersion(v, w string) (int, error) { // sanity check - if !IsValidSemver(v) { + if !IsValid(v) { return 0, fmt.Errorf("%s is not a valid semantic version", v) } - if !IsValidSemver(w) { + if !IsValid(w) { return 0, fmt.Errorf("%s is not a valid semantic version", w) } diff --git a/plugin/manager.go b/plugin/manager.go index 28b98bc8..09af2d1c 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -184,8 +184,10 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) } // clean up before installation, this guarantees idempotent for install - if err := os.RemoveAll(pluginDirPath); err != nil { - return nil, nil, fmt.Errorf("failed to clean up %s before installation: %w", pluginDirPath, err) + if err := m.Uninstall(ctx, pluginName); err != nil { + if !errors.Is(err, os.ErrNotExist) { + return nil, nil, fmt.Errorf("failed to clean up plugin %s before installation: %w", pluginName, err) + } } if installFromNonDir { if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { @@ -258,13 +260,14 @@ func parsePluginFromDir(path string) (string, string, error) { if err != nil { return err } - if isExec { - if foundPluginExecutableFile { - return errors.New("found more than one plugin executable files") - } - foundPluginExecutableFile = true - pluginExecutableFile = p + if !isExec { + return nil + } + if foundPluginExecutableFile { + return errors.New("found more than one plugin executable files") } + foundPluginExecutableFile = true + pluginExecutableFile = p } return nil }); err != nil { diff --git a/plugin/manager_unix.go b/plugin/manager_unix.go index b0b74bb1..6d173383 100644 --- a/plugin/manager_unix.go +++ b/plugin/manager_unix.go @@ -32,8 +32,9 @@ func isExecutableFile(filePath string) (bool, error) { if err != nil { return false, err } - if !fi.Mode().IsRegular() { + mode := fi.Mode() + if !mode.IsRegular() { return false, ErrNotRegularFile } - return fi.Mode()&0100 != 0, nil + return mode.Perm()&0100 != 0, nil } diff --git a/plugin/manager_windows.go b/plugin/manager_windows.go index c8cc8cca..c8504a0e 100644 --- a/plugin/manager_windows.go +++ b/plugin/manager_windows.go @@ -16,6 +16,7 @@ package plugin import ( "os" "path/filepath" + "strings" "github.com/notaryproject/notation-go/plugin/proto" ) @@ -33,5 +34,5 @@ func isExecutableFile(filePath string) (bool, error) { if !fi.Mode().IsRegular() { return false, ErrNotRegularFile } - return filepath.Ext(filepath.Base(filePath)) == ".exe", nil + return strings.EqualFold(filepath.Ext(filepath.Base(filePath)), ".exe"), nil } diff --git a/verifier/helpers.go b/verifier/helpers.go index 5b34440e..d432f0c1 100644 --- a/verifier/helpers.go +++ b/verifier/helpers.go @@ -149,7 +149,7 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string, if strings.TrimSpace(version) == "" { return "", fmt.Errorf("%v from extended attribute is an empty string", HeaderVerificationPluginMinVersion) } - if !notationsemver.IsValidSemver(version) { + if !notationsemver.IsValid(version) { return "", fmt.Errorf("%v from extended attribute is not a valid SemVer", HeaderVerificationPluginMinVersion) } return version, nil diff --git a/verifier/verifier.go b/verifier/verifier.go index 617cd7b5..c340f8e9 100644 --- a/verifier/verifier.go +++ b/verifier/verifier.go @@ -232,7 +232,7 @@ func (v *verifier) processSignature(ctx context.Context, sigBlob []byte, envelop pluginVersion := metadata.Version //checking if the plugin version is in valid semver format - if !notationsemver.IsValidSemver(pluginVersion) { + if !notationsemver.IsValid(pluginVersion) { return notation.ErrorVerificationInconclusive{Msg: fmt.Sprintf("plugin %s has pluginVersion %s which is not in valid semver format", verificationPluginName, pluginVersion)} } From cb3218292bdf351a0ab56c314daed86cf5db52a5 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 15 Dec 2023 18:42:20 +0800 Subject: [PATCH 34/36] updated per code review Signed-off-by: Patrick Zheng --- internal/file/file.go | 10 ++++------ plugin/manager.go | 15 +++++---------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index 34021988..2da781fe 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -34,24 +34,22 @@ func IsValidFileName(fileName string) bool { return regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`).MatchString(fileName) } -// CopyToDir copies the src file to dst. Existing file will be overwritten. +// CopyToDir copies the src file to dst dir. Existing file will be overwritten. +// src file permission is preserved. func CopyToDir(src, dst string) error { sourceFileStat, err := os.Stat(src) if err != nil { return err } - if !sourceFileStat.Mode().IsRegular() { return ErrNotRegularFile } - source, err := os.Open(src) if err != nil { return err } defer source.Close() - - if err := os.MkdirAll(dst, 0700); err != nil { + if err := os.MkdirAll(dst, 0777); err != nil { return err } dstFile := filepath.Join(dst, filepath.Base(src)) @@ -60,7 +58,7 @@ func CopyToDir(src, dst string) error { return err } defer destination.Close() - err = destination.Chmod(0600) + err = destination.Chmod(sourceFileStat.Mode()) if err != nil { return err } diff --git a/plugin/manager.go b/plugin/manager.go index 09af2d1c..5f40c74f 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -178,17 +178,17 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } } } - // core process - pluginDirPath, err := m.pluginFS.SysPath(pluginName) - if err != nil { - return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) - } // clean up before installation, this guarantees idempotent for install if err := m.Uninstall(ctx, pluginName); err != nil { if !errors.Is(err, os.ErrNotExist) { return nil, nil, fmt.Errorf("failed to clean up plugin %s before installation: %w", pluginName, err) } } + // core process + pluginDirPath, err := m.pluginFS.SysPath(pluginName) + if err != nil { + return nil, nil, fmt.Errorf("failed to get the system path of plugin %s: %w", pluginName, err) + } if installFromNonDir { if err := file.CopyToDir(pluginExecutableFile, pluginDirPath); err != nil { return nil, nil, fmt.Errorf("failed to copy plugin executable file from %s to %s: %w", pluginExecutableFile, pluginDirPath, err) @@ -198,11 +198,6 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) return nil, nil, fmt.Errorf("failed to copy plugin files from %s to %s: %w", installOpts.PluginPath, pluginDirPath, err) } } - // plugin binary file is always executable - pluginFilePath := path.Join(pluginDirPath, filepath.Base(pluginExecutableFile)) - if err := os.Chmod(pluginFilePath, 0700); err != nil { - return nil, nil, fmt.Errorf("failed to change the plugin executable file mode: %w", err) - } return existingPluginMetadata, newPluginMetadata, nil } From 67d267e3c892d31f5be173dd6bceeef4630b6386 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 18 Dec 2023 10:09:12 +0800 Subject: [PATCH 35/36] updated per code review Signed-off-by: Patrick Zheng --- internal/file/file.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/file/file.go b/internal/file/file.go index 2da781fe..920fa640 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -34,14 +34,18 @@ func IsValidFileName(fileName string) bool { return regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`).MatchString(fileName) } -// CopyToDir copies the src file to dst dir. Existing file will be overwritten. -// src file permission is preserved. +// CopyToDir copies the src file to dst dir. All parent directories are created +// with permissions 0755. +// +// Source file's read and execute permissions are preserved for everyone. +// Write permission is preserved for owner. Group and others cannot write. +// Existing file will be overwritten. func CopyToDir(src, dst string) error { - sourceFileStat, err := os.Stat(src) + sourceFileInfo, err := os.Stat(src) if err != nil { return err } - if !sourceFileStat.Mode().IsRegular() { + if !sourceFileInfo.Mode().IsRegular() { return ErrNotRegularFile } source, err := os.Open(src) @@ -49,7 +53,7 @@ func CopyToDir(src, dst string) error { return err } defer source.Close() - if err := os.MkdirAll(dst, 0777); err != nil { + if err := os.MkdirAll(dst, 0755); err != nil { return err } dstFile := filepath.Join(dst, filepath.Base(src)) @@ -58,7 +62,7 @@ func CopyToDir(src, dst string) error { return err } defer destination.Close() - err = destination.Chmod(sourceFileStat.Mode()) + err = destination.Chmod(sourceFileInfo.Mode() & os.FileMode(0755)) if err != nil { return err } From d6ab20f72f9911aaa52aa4ab46ac3e16a65b251f Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 18 Dec 2023 11:55:38 +0800 Subject: [PATCH 36/36] updated error message Signed-off-by: Patrick Zheng --- plugin/manager.go | 2 +- plugin/manager_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 5f40c74f..65aea0bd 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -172,7 +172,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } switch { case comp < 0: - return nil, nil, PluginDowngradeError{Msg: fmt.Sprintf("the installing plugin version %s is lower than the existing plugin version %s", newPluginMetadata.Version, existingPluginMetadata.Version)} + return nil, nil, PluginDowngradeError{Msg: fmt.Sprintf("failed to install plugin %s. The installing plugin version %s is lower than the existing plugin version %s", pluginName, newPluginMetadata.Version, existingPluginMetadata.Version)} case comp == 0: return nil, nil, InstallEqualVersionError{Msg: fmt.Sprintf("plugin %s with version %s already exists", pluginName, existingPluginMetadata.Version)} } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 93518570..8aa92f88 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -241,7 +241,7 @@ func TestManager_Install(t *testing.T) { installOpts := CLIInstallOptions{ PluginPath: newPluginFilePath, } - expectedErrorMsg := "the installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0" + expectedErrorMsg := "failed to install plugin foo. The installing plugin version 0.1.0 is lower than the existing plugin version 1.0.0" _, _, err := mgr.Install(context.Background(), installOpts) if err == nil || err.Error() != expectedErrorMsg { t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)