From bd92473ef4d2f1e42e6f93da9c426b599c9f43ea Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Wed, 6 Dec 2023 13:13:51 +0800 Subject: [PATCH] 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)} }