Added certificate management commands to the docker nv2 plugin.#70
Added certificate management commands to the docker nv2 plugin.#70sajayantony wants to merge 1 commit intonotaryproject:prototype-2from
Conversation
Currently adding and removing certificates from the verification list is really error prone. Providing an ability to add and remove the certificates would improve the user experience ``` ❯ docker nv2 certificates --help NAME: docker nv2 certificates - Manage certificates used for signing and verification USAGE: docker nv2 certificates command [command options] [arguments...] COMMANDS: add Add certificate to verification list list, ls List cerificates used for verification remove, rm Remove certificate from verification list help, h Shows a list of commands or help for one command OPTIONS: --help, -h show help (default: false) ``` ``` ❯ docker nv2 certificates ls /home/sajay/.notary/keys/wabbit-networks.crt /home/sajay/code/src/github.com/notaryproject/nv2/cmd/nv2/cert/wabbit-networks.crt ``` ``` ❯ docker nv2 certificates add /home/sajay/.notary/keys/wabbit-networks.crt Added /home/sajay/.notary/keys/wabbit-networks.crt to verification certificates ``` ``` ❯ docker nv2 certificates rm /home/sajay/code/src/github.com/notaryproject/nv2/cmd/nv2/cert/wabbit-networks.crt Removed /home/sajay/code/src/github.com/notaryproject/nv2/cmd/nv2/cert/wabbit-networks.crt from list of verification certificates ``` Signed-off-by: Sajay Antony <sajaya@microsoft.com>
|
/cc @shizhMSFT |
| ) | ||
|
|
||
| var certsCommand = &cli.Command{ | ||
| Name: "certificates", |
There was a problem hiding this comment.
Should we shorten the command to cert or certs?
There was a problem hiding this comment.
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.
| if !ctx.Args().Present() { | ||
| return errors.New("Required argument, certificate path not specified") | ||
| } | ||
| cert := ctx.Args().First() |
There was a problem hiding this comment.
Can we add multiple certs at once?
There was a problem hiding this comment.
Similar to above if we can start with one then we can support multiple as a additive issue.
| fmt.Printf("Added %s to verification certificates\n", cert) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Should return error if there is an error on cfg.Save().
| } | ||
|
|
||
| for _, s := range cfg.VerificationCerts { | ||
| fmt.Printf("%s\n", s) |
|
|
||
| cfg, err := config.Load() | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { |
There was a problem hiding this comment.
if it is os.IsNotExist(err), we return nil. Otherwise, we should return err.
| if !ctx.Args().Present() { | ||
| return errors.New("Required argument, certificate path not specified") | ||
| } | ||
| cert := ctx.Args().First() |
There was a problem hiding this comment.
Can we remove multiple certs at once?
| fmt.Printf("Removed %s from list of verification certificates\n", cert) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Should return error if there is an error on cfg.Save().
|
From a refactoring to the notation-go-lib, how much should be reflected there? |
|
Please see: #78 for some cli commands. |
| var certsAddCommand = &cli.Command{ | ||
| Name: "add", | ||
| Usage: "Add certificate to verification list", | ||
| ArgsUsage: "[cert]", |
There was a problem hiding this comment.
should this be certificate path?
| 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 | ||
| } |
There was a problem hiding this comment.
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) |
|
|
||
| cfg, err := config.Load() | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { |
There was a problem hiding this comment.
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.
|
@priteshbandi #83 PR adds all this capability into notation alpha. @shizhMSFT - can see if there are inputs here you might not have already considered in your PR? |
|
Closing since #83 supersedes this. |
4 participants
Currently adding and removing certificates from the verification list is really error prone.
Providing an ability to add and remove the certificates would improve the user experience
Signed-off-by: Sajay Antony sajaya@microsoft.com
Fixes #69