Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions cmd/docker-nv2/certs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
package main

import (
"errors"
"fmt"
"os"

"github.com/notaryproject/nv2/cmd/docker-nv2/config"
"github.com/urfave/cli/v2"
)

var certsCommand = &cli.Command{
Name: "certificates",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we shorten the command to cert or certs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just add an alias.

Copy link
Contributor Author

@sajayantony sajayantony Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can start with one and enhance along the way. Also regarding shortening, is there a convention among othere tools to use certs or cert that we can reference here. I was also debating with short names but the full certificates is always less ambiguous.

Usage: "Manage certificates used for signing and verification",
Subcommands: []*cli.Command{
certsAddCommand,
certsListCommand,
certsRemoveCommand,
},
}

var certsAddCommand = &cli.Command{
Name: "add",
Usage: "Add certificate to verification list",
ArgsUsage: "[cert]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be certificate path?


Action: addCert,
}

var certsListCommand = &cli.Command{
Name: "list",
Usage: "List certificates used for verification",
Aliases: []string{"ls"},
Action: listCerts,
}

var certsRemoveCommand = &cli.Command{
Name: "remove",
Usage: "Remove certificate from verification list",
Aliases: []string{"rm"},
ArgsUsage: "[cert]",
Action: removeCerts,
}

func uniqueAppend(entries []string, e string) []string {
entries = append(entries, e)
keys := make(map[string]bool)
list := []string{}
for _, item := range entries {
if _, value := keys[item]; !value {
keys[item] = true
list = append(list, item)
}
}
return list
}
Comment on lines +45 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the input to function is slice, this method created a map from slice with slice values as keys thus removing duplicate but won't it be more efficient(both memory and compute) to just iterate over slice i.e., entries and check for duplicate and add e if none is found?

Also, by not creating intermediate map we will be preserving the order of certificates thus every invocation of certificates ls will provide predictable output to customer.

Before:
certificates add path1
certificates add path2
certificates add path3
certificates ls
> [path2, path1, path3]
certificates add path4
certificates ls
> [path2, path4, path1, path3]

After
certificates add path1
certificates add path2
certificates add path3
certificates ls
> [path1, path2, path3]
certificates add path4
certificates ls
> [path1, path2, path3, path4]


func uniqueRemove(entries []string, e string) ([]string, error) {
keys := make(map[string]bool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

list := []string{}
found := false
for _, item := range entries {
if item == e {
keys[item] = true
found = true
} else if _, value := keys[item]; !value {
keys[item] = true
list = append(list, item)
}
}

if !found {
return nil, fmt.Errorf("%s not in the list", e)
}

return list, nil
}

func addCert(ctx *cli.Context) error {

if !ctx.Args().Present() {
return errors.New("Required argument, certificate path not specified")
}
cert := ctx.Args().First()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add multiple certs at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above if we can start with one then we can support multiple as a additive issue.


cfg, err := config.Load()
if err != nil {
if !os.IsNotExist(err) {
return err
}
cfg = config.New()
}
cfg.VerificationCerts = uniqueAppend(cfg.VerificationCerts, cert)

err = cfg.Save()
if err == nil {
fmt.Printf("Added %s to verification certificates\n", cert)
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return error if there is an error on cfg.Save().

}

func listCerts(ctx *cli.Context) error {

cfg, err := config.Load()
if err != nil {
if !os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is os.IsNotExist(err), we return nil. Otherwise, we should return err.

return nil
}
}

for _, s := range cfg.VerificationCerts {
fmt.Printf("%s\n", s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fmt.Println(s)

}
return nil
}

func removeCerts(ctx *cli.Context) error {

if !ctx.Args().Present() {
return errors.New("Required argument, certificate path not specified")
}
cert := ctx.Args().First()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove multiple certs at once?


cfg, err := config.Load()
if err != nil {
if !os.IsNotExist(err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if config doesnt exists then we can short circuit here(no need to create config) and display proper error message to customer saying that it’s an invalid operation as there is zero verification certificate stored.

return err
}
cfg = config.New()
}
cfg.VerificationCerts, err = uniqueRemove(cfg.VerificationCerts, cert)
if err != nil {
return err
}

err = cfg.Save()
if err == nil {
fmt.Printf("Removed %s from list of verification certificates\n", cert)
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return error if there is an error on cfg.Save().

}
1 change: 1 addition & 0 deletions cmd/docker-nv2/nv2.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var nv2Command = &cli.Command{
notaryCommand,
pullCommand,
pushCommand,
certsCommand,
},
Action: delegateToDocker,
}
Expand Down