updated per code review

Signed-off-by: Patrick Zheng <[email protected]>
notaryproject · Oct 11, 2023 · 52213ae · 52213ae
1 parent 2b9c5fb
commit 52213ae
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 23 deletions.
diff --git a/cmd/notation/cert/list.go b/cmd/notation/cert/list.go
@@ -78,41 +78,51 @@ func listCerts(ctx context.Context, opts *certListOpts) error {
 	// List all certificates under truststore/x509, display empty if there's
 	// no certificate yet
 	if namedStore == "" && storeType == "" {
-		path, err := configFS.SysPath(dir.TrustStoreDir, "x509")
-		if err := truststore.CheckNonErrNotExistError(err); err != nil {
-			return err
-		}
-		if err := truststore.CheckNonErrNotExistError(truststore.ListCerts(path, 2, &certPaths)); err != nil {
-			logger.Debugln("Failed to complete list at path:", path)
-			return fmt.Errorf("failed to list all certificates stored in the trust store, with error: %s", err.Error())
+		for _, t := range notationgoTruststore.Types {
+			path, err := configFS.SysPath(dir.TrustStoreDir, "x509", string(t))
+			if err := truststore.CheckNonErrNotExistError(err); err != nil {
+				return err
+			}
+			certs, err := truststore.ListCerts(path, 1)
+			if err := truststore.CheckNonErrNotExistError(err); err != nil {
+				logger.Debugln("Failed to complete list at path:", path)
+				return fmt.Errorf("failed to list all certificates stored in the trust store, with error: %s", err.Error())
+			}
+			certPaths = append(certPaths, certs...)
 		}
-
 		return ioutil.PrintCertMap(os.Stdout, certPaths)
 	}
 
 	// List all certificates under truststore/x509/storeType/namedStore,
-	// display empty if there's no certificate yet
+	// display empty if store type is invalid or there's no certificate yet
 	if namedStore != "" && storeType != "" {
+		if !truststore.IsValidStoreType(storeType) {
+			return nil
+		}
 		path, err := configFS.SysPath(dir.TrustStoreDir, "x509", storeType, namedStore)
 		if err := truststore.CheckNonErrNotExistError(err); err != nil {
 			return err
 		}
-		if err := truststore.CheckNonErrNotExistError(truststore.ListCerts(path, 0, &certPaths)); err != nil {
+		certPaths, err := truststore.ListCerts(path, 0)
+		if err := truststore.CheckNonErrNotExistError(err); err != nil {
 			logger.Debugln("Failed to complete list at path:", path)
 			return fmt.Errorf("failed to list all certificates stored in the named store %s of type %s, with error: %s", namedStore, storeType, err.Error())
 		}
-
 		return ioutil.PrintCertMap(os.Stdout, certPaths)
 	}
 
-	// List all certificates under x509/storeType, display empty if
-	// there's no certificate yet
+	// List all certificates under x509/storeType, display empty if store type
+	// is invalid or there's no certificate yet
 	if storeType != "" {
+		if !truststore.IsValidStoreType(storeType) {
+			return nil
+		}
 		path, err := configFS.SysPath(dir.TrustStoreDir, "x509", storeType)
 		if err := truststore.CheckNonErrNotExistError(err); err != nil {
 			return err
 		}
-		if err := truststore.CheckNonErrNotExistError(truststore.ListCerts(path, 1, &certPaths)); err != nil {
+		certPaths, err = truststore.ListCerts(path, 1)
+		if err := truststore.CheckNonErrNotExistError(err); err != nil {
 			logger.Debugln("Failed to complete list at path:", path)
 			return fmt.Errorf("failed to list all certificates stored of type %s, with error: %s", storeType, err.Error())
 		}
@@ -124,10 +134,12 @@ func listCerts(ctx context.Context, opts *certListOpts) error {
 			if err := truststore.CheckNonErrNotExistError(err); err != nil {
 				return err
 			}
-			if err := truststore.CheckNonErrNotExistError(truststore.ListCerts(path, 0, &certPaths)); err != nil {
+			certs, err := truststore.ListCerts(path, 0)
+			if err := truststore.CheckNonErrNotExistError(err); err != nil {
 				logger.Debugln("Failed to complete list at path:", path)
 				return fmt.Errorf("failed to list all certificates stored in the named store %s, with error: %s", namedStore, err.Error())
 			}
+			certPaths = append(certPaths, certs...)
 		}
 	}
 

diff --git a/cmd/notation/internal/truststore/truststore.go b/cmd/notation/internal/truststore/truststore.go
@@ -85,14 +85,12 @@ func AddCert(path, storeType, namedStore string, display bool) error {
 	return nil
 }
 
-// ListCerts walks through root and lists all x509 certificates in it,
+// ListCerts walks through root and returns all x509 certificates in it,
 // sub-dirs are ignored.
-func ListCerts(root string, depth int, certPaths *[]string) error {
+func ListCerts(root string, depth int) ([]string, error) {
 	maxDepth := strings.Count(root, string(os.PathSeparator)) + depth
-	if certPaths == nil {
-		return errors.New("certPaths cannot be nil")
-	}
-	return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
+	var certPaths []string
+	if err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err
 		}
@@ -109,11 +107,15 @@ func ListCerts(root string, depth int, certPaths *[]string) error {
 				return err
 			}
 			if len(certs) != 0 {
-				*certPaths = append(*certPaths, path)
+				certPaths = append(certPaths, path)
 			}
 		}
 		return nil
-	})
+	}); err != nil {
+		return nil, err
+	}
+
+	return certPaths, nil
 }
 
 // ShowCerts writes out details of certificates

diff --git a/internal/ioutil/print.go b/internal/ioutil/print.go
@@ -27,6 +27,7 @@ func newTabWriter(w io.Writer) *tabwriter.Writer {
 	return tabwriter.NewWriter(w, 0, 0, 3, ' ', 0)
 }
 
+// PrintKeyMap prints out key information given array of KeySuite
 func PrintKeyMap(w io.Writer, target *string, v []config.KeySuite) error {
 	tw := newTabWriter(w)
 	fmt.Fprintln(tw, "NAME\tKEY PATH\tCERTIFICATE PATH\tID\tPLUGIN NAME\t")
@@ -48,6 +49,7 @@ func PrintKeyMap(w io.Writer, target *string, v []config.KeySuite) error {
 	return tw.Flush()
 }
 
+// PrintMetadataMap prints out metadata given the metatdata map
 func PrintMetadataMap(w io.Writer, metadata map[string]string) error {
 	tw := newTabWriter(w)
 	fmt.Fprintln(tw, "\nKEY\tVALUE\t")
@@ -59,7 +61,12 @@ func PrintMetadataMap(w io.Writer, metadata map[string]string) error {
 	return tw.Flush()
 }
 
+// PrintCertMap lists certificate files in the trust store given array of cert
+// paths
 func PrintCertMap(w io.Writer, certPaths []string) error {
+	if len(certPaths) == 0 {
+		return nil
+	}
 	tw := newTabWriter(w)
 	fmt.Fprintln(tw, "STORE TYPE\tSTORE NAME\tCERTIFICATE\t")
 	for _, cert := range certPaths {

diff --git a/specs/commandline/certificate.md b/specs/commandline/certificate.md
@@ -153,6 +153,14 @@ notation certificate list
 
 Upon successful listing, all the certificate files in the trust store are printed out with information of store type, store name and certificate file name. If the listing fails, an error message is printed out with specific reasons. Nothing is printed out if the trust store is empty.
 
+An example of the output:
+```
+STORE TYPE         STORE NAME   CERTIFICATE   
+ca                 myStore1     cert1.pem    
+ca                 myStore2     cert2.crt       
+signingAuthority   myStore1     cert3.crt    
+signingAuthority   myStore2     cert4.pem
+```
 ### List all certificate files of a certain named store
 
 ```bash