Repository: notaryproject/notation (go)

Issue:
Delete the named store dir if empty after `notation cert delete`
              After deleting the certificate, do we need to delete the trust store folder of the cert if it is empty? Otherwise, there will be many empty folders left there.

_Originally posted by @JeyJeyGao in https://github.com/notaryproject/notation/pull/1231#discussion_r2013297657_
            

Pull request:
fix: remove empty trust store directory after `notation cert delete` and `notation cert cleanup-test`
Resolves #1239 

Patch:
diff --git a/cmd/notation/cert/delete.go b/cmd/notation/cert/delete.go
index 0717416..a885e1c 100644
--- a/cmd/notation/cert/delete.go
+++ b/cmd/notation/cert/delete.go
@@ -96,6 +96,7 @@ func deleteCerts(opts *certDeleteOpts) error {
 		}
 		return nil
 	}
+
 	// Delete a certain certificate with path storeType/namedStore/cert
 	cert := opts.cert
 	if cert == "" {
diff --git a/cmd/notation/internal/truststore/truststore.go b/cmd/notation/internal/truststore/truststore.go
index 73452d7..881a312 100644
--- a/cmd/notation/internal/truststore/truststore.go
+++ b/cmd/notation/internal/truststore/truststore.go
@@ -71,6 +71,7 @@ func AddCert(path, storeType, namedStore string, display bool) error {
 	if _, err := os.Stat(filepath.Join(trustStorePath, filepath.Base(certPath))); err == nil {
 		return errors.New("certificate already exists in the Trust Store")
 	}
+
 	// add cert to trust store
 	_, err = osutil.CopyToDir(certPath, trustStorePath)
 	if err != nil {
@@ -81,7 +82,6 @@ func AddCert(path, storeType, namedStore string, display bool) error {
 	if display {
 		fmt.Printf("Successfully added %s to named store %s of type %s\n", filepath.Base(certPath), namedStore, storeType)
 	}
-
 	return nil
 }
 
@@ -114,7 +114,6 @@ func ListCerts(root string, depth int) ([]string, error) {
 	}); err != nil {
 		return nil, err
 	}
-
 	return certPaths, nil
 }
 
@@ -163,6 +162,7 @@ func DeleteAllCerts(storeType, namedStore string, confirmed bool) error {
 	if err = os.RemoveAll(path); err != nil {
 		return err
 	}
+
 	// write out on success
 	fmt.Printf("Successfully deleted %s\n", path)
 	return nil
@@ -189,6 +189,20 @@ func DeleteCert(storeType, namedStore, cert string, confirmed bool) error {
 	if err = os.Remove(path); err != nil {
 		return err
 	}
+
+	// remove the trust store directory if empty
+	storePath := filepath.Dir(path)
+	empty, err := osutil.IsDirEmpty(storePath)
+	if err != nil {
+		// in this case, empty is always false
+		fmt.Fprintf(os.Stderr, "Warning: failed to check if the trust store directory %s is empty: %v\n", storePath, err)
+	}
+	if empty {
+		if err := os.Remove(storePath); err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: failed to remove the empty trust store directory %s: %v\n", storePath, err)
+		}
+	}
+
 	// write out on success
 	fmt.Printf("Successfully deleted %s from trust store %s of type %s\n", cert, namedStore, storeType)
 	return nil
diff --git a/internal/osutil/file.go b/internal/osutil/file.go
index 06f6979..3475314 100644
--- a/internal/osutil/file.go
+++ b/internal/osutil/file.go
@@ -16,6 +16,7 @@ package osutil
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"io"
 	"io/fs"
@@ -154,3 +155,19 @@ func ValidateSHA256Sum(path string, checksum string) error {
 	}
 	return nil
 }
+
+// IsDirEmpty checks if directory dir is empty.
+// It returns true and nil error if and only if dir is empty.
+func IsDirEmpty(dir string) (bool, error) {
+	d, err := os.Open(dir)
+	if err != nil {
+		return false, err
+	}
+	defer d.Close()
+
+	_, err = d.Readdirnames(1)
+	if errors.Is(err, io.EOF) {
+		return true, nil
+	}
+	return false, err
+}


Test patch:
diff --git a/internal/osutil/file_test.go b/internal/osutil/file_test.go
index 59226f9..523955d 100644
--- a/internal/osutil/file_test.go
+++ b/internal/osutil/file_test.go
@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 )
 
@@ -269,3 +270,41 @@ func TestValidateChecksum(t *testing.T) {
 		t.Fatalf("expected nil err, but got %v", err)
 	}
 }
+
+func TestIsDirEmpty(t *testing.T) {
+	t.Run("empty dir", func(t *testing.T) {
+		tempDir := t.TempDir()
+		isEmpty, err := IsDirEmpty(tempDir)
+		if err != nil {
+			t.Fatalf("expected nil error, but got %s", err)
+		}
+		if !isEmpty {
+			t.Fatal("should be empty")
+		}
+	})
+
+	t.Run("dir does not exist", func(t *testing.T) {
+		nonExistDir := "invalid"
+		_, err := IsDirEmpty(nonExistDir)
+		if err == nil {
+			t.Fatalf("expected error, but got nil")
+		}
+	})
+
+	t.Run("failed to read content of dir", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("skipping test on Windows")
+		}
+		tempDir := t.TempDir()
+		if err := os.Chmod(tempDir, 0000); err != nil {
+			t.Fatal(err)
+		}
+		defer os.Chmod(tempDir, 0700)
+
+		expectedErrorMsg := "permission denied"
+		_, err := IsDirEmpty(tempDir)
+		if err == nil || !strings.Contains(err.Error(), expectedErrorMsg) {
+			t.Fatalf("expected permission denied, but got %s", err)
+		}
+	})
+}
diff --git a/test/e2e/suite/command/cert.go b/test/e2e/suite/command/cert.go
index ee5ea8f..7d3c7d0 100644
--- a/test/e2e/suite/command/cert.go
+++ b/test/e2e/suite/command/cert.go
@@ -40,15 +40,42 @@ var _ = Describe("notation cert", func() {
 				MatchKeyWords(
 					"Successfully deleted",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
-	It("delete a specfic cert", func() {
+	It("delete a specific cert and the empty trust store directory", func() {
 		Host(BaseOptions(), func(notation *utils.ExecOpts, artifact *Artifact, vhost *utils.VirtualHost) {
 			notation.Exec("cert", "delete", "--type", "ca", "--store", "e2e", "e2e.crt", "-y").
 				MatchKeyWords(
 					"Successfully deleted e2e.crt from trust store e2e of type ca",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
+		})
+	})
+
+	It("delete a specific cert from trust store containing more than one certificates", func() {
+		Host(BaseOptions(), func(notation *utils.ExecOpts, artifact *Artifact, vhost *utils.VirtualHost) {
+			notation.Exec("cert", "add", "--type", "ca", "--store", "e2e", filepath.Join(NotationE2ELocalKeysDir, "expired_e2e.crt")).
+				MatchKeyWords("Successfully added following certificates")
+
+			notation.Exec("cert", "delete", "--type", "ca", "--store", "e2e", "expired_e2e.crt", "-y").
+				MatchKeyWords(
+					"Successfully deleted expired_e2e.crt from trust store e2e of type ca",
+				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")
+			if _, err := os.Stat(trustStorePath); err != nil {
+				Fail(fmt.Sprintf("trust store directory %s should still exist", trustStorePath))
+			}
 		})
 	})
 
@@ -58,6 +85,32 @@ var _ = Describe("notation cert", func() {
 				MatchErrKeyWords(
 					"failed to delete the certificate file",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")
+			if _, err := os.Stat(trustStorePath); err != nil {
+				Fail(fmt.Sprintf("trust store directory %s should still exist", trustStorePath))
+			}
+		})
+	})
+
+	It("delete a specific cert but failed to delete the empty trust store directory", func() {
+		Host(BaseOptions(), func(notation *utils.ExecOpts, artifact *Artifact, vhost *utils.VirtualHost) {
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e")
+
+			// Remove read permission for trustStorePath
+			if err := os.Chmod(trustStorePath, 0300); err != nil {
+				Fail(err.Error())
+			}
+			defer os.Chmod(trustStorePath, 0700)
+
+			notation.Exec("cert", "delete", "--type", "ca", "--store", "e2e", "e2e.crt", "-y").
+				MatchKeyWords(
+					"Successfully deleted e2e.crt from trust store e2e of type ca",
+				).
+				MatchErrKeyWords(
+					fmt.Sprintf("Warning: failed to check if the trust store directory %s is empty", trustStorePath),
+					"permission denied",
+				)
 		})
 	})
 
@@ -129,6 +182,11 @@ var _ = Describe("notation cert", func() {
 					"Successfully deleted certificate file:", "e2e-test.crt",
 					"Cleanup completed successfully",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -153,6 +211,11 @@ var _ = Describe("notation cert", func() {
 					"Successfully deleted certificate file:", "e2e-test.crt",
 					"Cleanup completed successfully",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -195,6 +258,11 @@ var _ = Describe("notation cert", func() {
 					"Successfully deleted certificate file:", "e2e-test.crt",
 					"Cleanup completed successfully",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -221,6 +289,10 @@ var _ = Describe("notation cert", func() {
 				MatchKeyWords(
 					"Successfully deleted e2e-test.crt from trust store e2e-test of type ca",
 				)
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 
 			notation.Exec("cert", "cleanup-test", "e2e-test", "-y").
 				MatchKeyWords(
@@ -249,6 +321,11 @@ var _ = Describe("notation cert", func() {
 					"Successfully deleted certificate file:", "e2e-test.crt",
 					"Cleanup completed successfully",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -268,6 +345,11 @@ var _ = Describe("notation cert", func() {
 					fmt.Sprintf("Certificate file %s does not exist", localCertPath),
 					"Cleanup completed successfully",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -302,6 +384,11 @@ var _ = Describe("notation cert", func() {
 					"failed to delete certificate e2e-test.crt from trust store e2e-test of type ca",
 					"permission denied",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err != nil {
+				Fail(fmt.Sprintf("trust store directory %s should still exist", trustStorePath))
+			}
 		})
 	})
 
@@ -318,6 +405,11 @@ var _ = Describe("notation cert", func() {
 					"failed to remove key e2e-test from the key list",
 					"permission denied",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 
@@ -334,6 +426,11 @@ var _ = Describe("notation cert", func() {
 					fmt.Sprintf("failed to delete key file %s", filepath.Join(localKeysDir, "e2e-test.key")),
 					"permission denied",
 				)
+
+			trustStorePath := vhost.AbsolutePath(NotationDirName, TrustStoreDirName, "x509", TrustStoreTypeCA, "e2e-test")
+			if _, err := os.Stat(trustStorePath); err == nil {
+				Fail(fmt.Sprintf("empty trust store directory %s should be deleted", trustStorePath))
+			}
 		})
 	})
 })