Repository: deislabs/ratify
Pull request #150 (2022-03-22 17:28:08)

Bug fix and improvement to loading notary verification certs
- Update GetCertificatesFromPath with null check to handle invalid path
- Added ReplaceHomeShortcut to address issue [78](https://github.com/deislabs/ratify/issues/78)
- Adding more logging to GetCertificatesFromPath

susanshi: @mnltejaswini , @akashsinghal 

susanshi: > LGTM as long as it invokes `cryptoutil.ReadCertificateFile()`.
> 
> BTW, what's the purpose of `GetCertificatesFromPath()` as its documentation is missing? If this function is to read certificates from all cert files of a certain directory, what if there are multiple cert files containing duplicated certs?

Yes, this method reads all unique physical path, will add doc to method. thanks!  if there is dups of the same cert in separate files, all the copies will be loaded.
Curious, will notary dedup the certs? 

shizhMSFT: > Curious, will notary dedup the certs?

No, it won't.
mnltejaswini: LGTM, except for few minor comments
susanshi: Hi @shizhMSFT, hope you are well.  please review latest revision, I think I need your collaborator access to merge.  :)
shizhMSFT: @susanshi Oops, I don't have permissions either.
susanshi: > @susanshi Oops, I don't have permissions either.

No Worries, @shizhMSFT .
 @sajayantony , could you please provide a signoff to unblock merge?

Patch:
diff --git a/pkg/utils/certificateUtils.go b/pkg/utils/certificateUtils.go
index 3ba82f9..94bf764 100644
--- a/pkg/utils/certificateUtils.go
+++ b/pkg/utils/certificateUtils.go
@@ -17,27 +17,63 @@ package utils
 
 import (
 	"crypto/x509"
+	"io/fs"
 	"os"
+	"strings"
 
 	"path/filepath"
 
+	"github.com/deislabs/ratify/pkg/homedir"
 	"github.com/notaryproject/notation-go-lib/crypto/cryptoutil"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
+// Return list of certificates loaded from path
+// when path is a directory, this method loads all certs in directory and resolve symlink if needed
 func GetCertificatesFromPath(path string) ([]*x509.Certificate, error) {
-
 	var certs []*x509.Certificate
+	fileMap := map[string]bool{} //a map to track path of physical files
+
+	path = ReplaceHomeShortcut(path)
 
 	err := filepath.Walk(path, func(file string, info os.FileInfo, err error) error {
-		if !info.IsDir() {
-			cert, certError := cryptoutil.ReadCertificateFile(file) // ReadCertificateFile returns empty if file was not a certificate
-			if certError != nil {
-				return certError
+
+		targetFileInfo := info
+		targetFilePath := file
+
+		if info == nil || err != nil {
+			logrus.Warnf("Invalid path '%v' skipped, error %v", file, err)
+			return nil
+		}
+
+		// In a cluster environment, each mounted file results in a physical file and a symlink
+		// check if file is a link and get the actual file path
+		if isSymbolicLink(info) {
+			targetFilePath, err = filepath.EvalSymlinks(file)
+			if err != nil || len(targetFilePath) == 0 {
+				logrus.Errorf("Unable to resolve symbolic link %v , error '%v'", file, err)
+				return nil
 			}
-			if cert != nil {
-				certs = append(certs, cert...)
+
+			targetFileInfo, err = os.Lstat(targetFilePath)
+
+			if err != nil {
+				logrus.Errorf("error getting file info for path '%v', error '%v'", targetFilePath, err)
+				return nil
+			}
+		}
+
+		// if filepath.EvalSymlinks fails to resolve multi level sym link, skip this file
+		if targetFileInfo != nil && !targetFileInfo.IsDir() && !isSymbolicLink(targetFileInfo) {
+			if _, ok := fileMap[targetFilePath]; !ok {
+				certs, err = loadCertFile(targetFileInfo, targetFilePath, certs, fileMap)
+				if err != nil {
+					return errors.Wrap(err, "error reading certificate file "+targetFilePath)
+				}
 			}
 		}
+
 		return nil
 	})
 
@@ -45,5 +81,38 @@ func GetCertificatesFromPath(path string) ([]*x509.Certificate, error) {
 		return nil, err
 	}
 
+	logrus.Infof("%v notary verification certificates loaded from path '%v'", len(certs), path)
 	return certs, nil
 }
+
+func isSymbolicLink(info fs.FileInfo) bool {
+	return info.Mode()&os.ModeSymlink != 0
+}
+
+func loadCertFile(fileInfo fs.FileInfo, filePath string, certificate []*x509.Certificate, fileMap map[string]bool) ([]*x509.Certificate, error) {
+	cert, certError := cryptoutil.ReadCertificateFile(filePath) // ReadCertificateFile returns empty if file was not a certificate
+	if certError != nil {
+		return certificate, certError
+	}
+	if cert != nil {
+		certificate = append(certificate, cert...)
+		fileMap[filePath] = true
+	}
+
+	return certificate, nil
+}
+
+// Replace the shortcut prefix in a path with the home directory
+// For example in a unix os, ~/.config/ becomes /home/azureuser/.config after replacement
+func ReplaceHomeShortcut(path string) string {
+	shortcutPrefix := homedir.GetShortcutString() + string(os.PathSeparator)
+	if strings.HasPrefix(path, shortcutPrefix) {
+		home := homedir.Get()
+		if len(home) > 0 {
+			return strings.Replace(path, homedir.GetShortcutString(), home, 1) // replace 1 instance
+		} else {
+			logrus.Warningf("Path '%v' replacement failed , value of Home dir '%v'", path, home)
+		}
+	}
+	return path
+}
diff --git a/plugins/verifier/cosign/cosign.go b/plugins/verifier/cosign/cosign.go
index 4a4111c..ffe91cc 100644
--- a/plugins/verifier/cosign/cosign.go
+++ b/plugins/verifier/cosign/cosign.go
@@ -26,6 +26,7 @@ import (
 	"github.com/deislabs/ratify/pkg/common"
 	"github.com/deislabs/ratify/pkg/ocispecs"
 	"github.com/deislabs/ratify/pkg/referrerstore"
+	"github.com/deislabs/ratify/pkg/utils"
 	"github.com/deislabs/ratify/pkg/verifier"
 	"github.com/deislabs/ratify/pkg/verifier/plugin/skel"
 	"github.com/google/go-containerregistry/pkg/authn"
@@ -133,7 +134,8 @@ func signatures(ctx context.Context, img string, keyRef string, config *PluginIn
 }
 
 func loadPublicKey(ctx context.Context, keyRef string) (verifier signature.Verifier, err error) {
-	raw, err := ioutil.ReadFile(filepath.Clean(keyRef))
+	keyPath := filepath.Clean(utils.ReplaceHomeShortcut(keyRef))
+	raw, err := ioutil.ReadFile(keyPath)
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/pkg/utils/certificateUtil_test.go b/pkg/utils/certificateUtil_test.go
index cb0e07a..7ab5c24 100644
--- a/pkg/utils/certificateUtil_test.go
+++ b/pkg/utils/certificateUtil_test.go
@@ -18,8 +18,34 @@ import (
 	"io/ioutil"
 	"os"
 	"testing"
+
+	"github.com/deislabs/ratify/pkg/homedir"
 )
 
+func TestHomepathReplacement(t *testing.T) {
+
+	sampleFoldername := "test"
+	testPath := homedir.GetShortcutString() + string(os.PathSeparator) + sampleFoldername
+
+	result := ReplaceHomeShortcut(testPath)
+	home, err := os.UserHomeDir()
+	expectedPath := home + "/" + sampleFoldername
+
+	if result != expectedPath {
+		t.Fatalf("sample input %v ,result expected to be %v, actual result %v, error %v", testPath, expectedPath, result, err)
+	}
+}
+
+func TestReadCertificatesFromPath_InvalidPath(t *testing.T) {
+
+	files, err := GetCertificatesFromPath("/invalid/path")
+
+	expectedFileCount := 0
+	if len(files) != expectedFileCount || err != nil {
+		t.Fatalf("response length expected to be %v, actual %v, error %v", expectedFileCount, len(files), err)
+	}
+}
+
 func TestReadCertificatesFromPath_NestedDirectory(t *testing.T) {
 	// Setup to create nested directory structure
 	testDir := "TestDirectory"
@@ -50,6 +76,56 @@ func TestReadCertificatesFromPath_NestedDirectory(t *testing.T) {
 	}
 }
 
+func TestReadFilesFromPath_SymbolicLink(t *testing.T) {
+
+	// Setup
+	currPath, _ := os.Getwd()
+	testDirName := "TestDirectory"
+	absTestDirPath := currPath + string(os.PathSeparator) + testDirName + string(os.PathSeparator)
+	testFile1 := absTestDirPath + "file1.Crt"
+
+	setupDirectoryForTesting(t, testDirName)
+	createCertFile(t, testFile1)
+
+	symlink := absTestDirPath + "symlink"
+	os.Symlink(testFile1, symlink)
+	files, err := GetCertificatesFromPath(symlink)
+
+	// Teardown
+	os.RemoveAll(absTestDirPath)
+
+	// Validate
+	if len(files) != 1 || err != nil {
+		t.Fatalf("response length expected to be 1, actual %v, error %v", len(files), err)
+	}
+}
+
+func TestReadFilesFromPath_MultilevelSymbolicLink(t *testing.T) {
+
+	// Setup
+	currPath, _ := os.Getwd()
+	testDirName := "TestDirectory"
+	absTestDirPath := currPath + string(os.PathSeparator) + testDirName + string(os.PathSeparator)
+	testFile1 := absTestDirPath + "file1.Crt"
+
+	setupDirectoryForTesting(t, testDirName)
+	createCertFile(t, testFile1)
+
+	symlink := absTestDirPath + "symlink"
+	twoLevelSymlink := absTestDirPath + "symlink2"
+	os.Symlink(testFile1, symlink)
+	os.Symlink(symlink, twoLevelSymlink)
+	files, err := GetCertificatesFromPath(twoLevelSymlink)
+
+	// Teardown
+	os.RemoveAll(absTestDirPath)
+
+	// validate
+	if len(files) != 1 || err != nil {
+		t.Fatalf("response length expected to be 1, actual %v, error %v", len(files), err)
+	}
+}
+
 func TestReadFilesFromPath_SingleFile(t *testing.T) {
 	// Setup
 	testDir := "TestDirectory"