feat: simplify signing experience by kody-kimberl · Pull Request #579 · notaryproject/notation

kody-kimberl · 2023-03-03T16:24:26Z

This PR introduces a new option for users to sign using an on-demand key. Users are able to sign without needing to add a key first

This PR intends to address the following issue: #537
Based on spec changes in #553

Successful sign/verify:

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id $IMAGE
Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Successfully signed localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d

c889f3b9d811:notation kodysk$ ./bin/notation verify $IMAGE
Warning: Always verify the artifact using digest(@sha256:...) rather than a tag(:v1) because resolved digest may not point to the same signed artifact, as tags are mutable.
Successfully verified signature for localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d

Error when providing only one of the options

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin $IMAGE 
Error: incompatible options, both a key ID and plugin name are required when not using an existing key

c889f3b9d811:notation kodysk$ ./bin/notation sign --id example:test-id $IMAGE 
Error: incompatible options, both a key ID and plugin name are required when not using an existing key

Error when providing key with both plugin and id:

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id --key my-key $IMAGE 
Error: incompatible options, do not provide a key name when providing a key ID and plugin name

Error when providing key with one of the other options

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --key my-key $IMAGE 
Error: incompatible options, do not provide a key ID or plugin name when providing a key name

c889f3b9d811:notation kodysk$ ./bin/notation sign --id example:test-id --key my-key $IMAGE 
Error: incompatible options, do not provide a key ID or plugin name when providing a key name

Signed-off-by: Kody Kimberl kody.kimberl.work@gmail.com

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

internal/cmd/flags.go

internal/cmd/signer.go

priteshbandi · 2023-03-03T20:09:31Z

internal/cmd/flags.go

+		fs.StringVar(p, PflagPlugin.Name, "", PflagPlugin.Usage)
+	}
+
 	PflagTimestamp = &pflag.Flag{


nit: Can we please remove timestamp flag as we will not be supporting in rc3?

cc: @shizhMSFT @patrickzheng200 @yizha1

@patrickzheng200 @shizhMSFT I didn't see timestamp flag in the command line. Is it the flag that we should have removed it earlier?

This is a dead code not referenced anywhere.

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl

New commit addresses above comments. Option checking has been moved to PreRunE, and timestamp pflag has been removed.

JeyJeyGao · 2023-03-06T01:26:05Z

internal/cmd/signer.go

 	// Construct a signer from preconfigured key pair in config.json
 	// if key name is provided as the CLI argument
 	key, err := configutil.ResolveKey(opts.Key)
+


unnecessary empty line

JeyJeyGao · 2023-03-06T01:27:27Z

internal/cmd/signer.go

+		if err != nil {
+			return nil, err
+		}
+		return signer.NewFromPlugin(plugin, opts.KeyID, map[string]string{})


There is a pluginConfig in SignerFlagOpts

I believe that pluginConfig is only in signOpts, not SignerFlagOpts.

PluginConfig in NewFromPlugin is used only when we are reading keys which includes pluginConfig from singingkeys.json then this plugin config is then merged with pluginConfig passed via CLI in Sign and Verify operation to generate final pluginConfig.

There is a --plugin-config flag for sign command

Yes, it is one of the options passed to sign, but it doesn't get passed via the SignerFlagOpts. The pluginConfig that is used here is intended to be the one from the key, which is later merged with the values from the pluginConfig command line option you are speaking of. That's what Pritesh was referring to above.

Sure, got it.

@priteshbandi This is what I mentioned earlier in the comments of the original issue. A user might get confused on where did the key.PluginConfig go? (Although programmatically it works exactly the same, users have to remember to add those configs in notation sign's pluginConfig now)

JeyJeyGao · 2023-03-06T01:28:55Z

internal/cmd/signer.go

+	if opts.KeyID != "" && opts.PluginName != "" && opts.Key == "" {
+		// Construct a signer from on-demand key
+		mgr := plugin.NewCLIManager(dir.PluginFS())
+		plugin, err := mgr.Get(context.Background(), opts.PluginName)


We should use the context generated in runSign() because it contains the logger.

Also please update line 41 to use the context from runSign()

Added ctx (the context from runSign) to the function in latest commit

JeyJeyGao · 2023-03-06T01:33:48Z

cmd/notation/sign.go

+			if opts.KeyID != "" && opts.PluginName != "" {
+				if opts.Key == "" {
+					// Valid, use ondemand key
+					return nil
+				} else {
+					return errors.New("incompatible options, do not provide a key name when providing a key ID and plugin name")
+				}
+			} else if opts.KeyID == "" && opts.PluginName == "" {
+				// Valid, use preconfigured key
+				return nil
+			} else {
+				if opts.Key == "" {
+					return errors.New("incompatible options, both a key ID and plugin name are required when not using an existing key")
+				} else {
+					return errors.New("incompatible options, do not provide a key ID or plugin name when providing a key name")
+				}


Can we simplify the validation? In fact, we just need to check invalid cases.

This is resolved by using Command.MarkFlagsMutuallyExclusive and Command.MarkFlagsRequiredTogether as recommended by @patrickzheng200

patrickzheng200 · 2023-03-06T12:16:54Z

cmd/notation/sign.go

+			// Check if the options are valid for the key (Key is mutually exclusive with [KeyID, PluginName])
+			if opts.KeyID != "" && opts.PluginName != "" {
+				if opts.Key == "" {
+					// Valid, use ondemand key
+					return nil
+				} else {
+					return errors.New("incompatible options, do not provide a key name when providing a key ID and plugin name")
+				}
+			} else if opts.KeyID == "" && opts.PluginName == "" {
+				// Valid, use preconfigured key
+				return nil
+			} else {
+				if opts.Key == "" {
+					return errors.New("incompatible options, both a key ID and plugin name are required when not using an existing key")
+				} else {
+					return errors.New("incompatible options, do not provide a key ID or plugin name when providing a key name")
+				}


Since we are using the Cobra library as our CLI tool: github.com/spf13/cobra

For mutual exclusive flags, we could try to use: Command.MarkFlagsMutuallyExclusive.

For "Required Together" flags such as plugin and key id, we could try to use: Command.MarkFlagsRequiredTogether.

codecov-commenter · 2023-03-06T13:44:41Z

Codecov Report

Merging #579 (495a872) into main (55f0764) will decrease coverage by 0.34%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   34.36%   34.03%   -0.34%     
==========================================
  Files          32       32              
  Lines        1848     1866      +18     
==========================================
  Hits          635      635              
- Misses       1192     1210      +18     
  Partials       21       21

Impacted Files	Coverage Δ
cmd/notation/sign.go	`34.40% <0.00%> (-5.79%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl

Latest commit addresses above comments. Some of the error messages have changed as a result of using Cobra's functions for validation (see below). I have also added unit testing for each of the below scenarios.

Successful sign/verify:

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id $IMAGE
Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Successfully signed localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d

c889f3b9d811:notation kodysk$ ./bin/notation verify $IMAGE
Warning: Always verify the artifact using digest(@sha256:...) rather than a tag(:v1) because resolved digest may not point to the same signed artifact, as tags are mutable.
Successfully verified signature for localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d

Error when providing only one of the options

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin $IMAGE 
Error: if any flags in the group [id plugin] are set they must all be set; missing [id]

c889f3b9d811:notation kodysk$ ./bin/notation sign --id example:test-id $IMAGE 
Error: if any flags in the group [id plugin] are set they must all be set; missing [plugin]

Error when providing key with one of the other options

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --key my-key $IMAGE 
Error: if any flags in the group [id plugin] are set they must all be set; missing [id]

c889f3b9d811:notation kodysk$ ./bin/notation sign --id example:test-id --key my-key $IMAGE 
Error: if any flags in the group [id plugin] are set they must all be set; missing [plugin]

Error when providing key with both plugin and id:

c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id --key my-key $IMAGE 
Error: if any flags in the group [key id] are set none of the others can be; [id key] were all set

cmd/notation/sign.go

priteshbandi · 2023-03-06T22:08:49Z

internal/cmd/signer.go

+		if err != nil {
+			return nil, err
+		}
+		return signer.NewFromPlugin(plugin, opts.KeyID, map[string]string{})


PluginConfig in NewFromPlugin is used only when we are reading keys which includes pluginConfig from singingkeys.json then this plugin config is then merged with pluginConfig passed via CLI in Sign and Verify operation to generate final pluginConfig.

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

priteshbandi

LGTM

internal/cmd/flags.go

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

JeyJeyGao

LGTM

patrickzheng200

LGTM

FeynmanZhou · 2023-03-07T09:56:21Z

Hi @kody-kimberl @priteshbandi @JeyJeyGao @patrickzheng200 ,

It seems the CLI example was not added to the CLI help info. Just a kindly reminder, when we change CLI commands and flags, we need to add or update the CLI examples in notation/cmd accordingly.

I will raise another PR to add it.

@sha256

This PR introduces a new option for users to sign using an on-demand key. Users are able to sign without needing to add a key first. This PR intends to address the following issue: notaryproject#537 based on spec changes in notaryproject#553 ``` c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id $IMAGE Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed. Successfully signed localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d ``` Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com> Signed-off-by: Josh Duffney <jduffney@microsoft.com>

@sha256

This PR introduces a new option for users to sign using an on-demand key. Users are able to sign without needing to add a key first. This PR intends to address the following issue: notaryproject#537 based on spec changes in notaryproject#553 ``` c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id $IMAGE Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed. Successfully signed localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d ``` Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

@sha256

This PR introduces a new option for users to sign using an on-demand key. Users are able to sign without needing to add a key first. This PR intends to address the following issue: notaryproject#537 based on spec changes in notaryproject#553 ``` c889f3b9d811:notation kodysk$ ./bin/notation sign --plugin com.example.plugin --id example:test-id $IMAGE Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed. Successfully signed localhost:5000/net-monitor@sha256:07c30edca95116e23f6a150ba3f7ed296eca71021dba9d9569eb07820f8c0e7d ``` Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

Adding simplified signing experience functionality

ce4ccaa

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl requested review from JeyJeyGao, NiazFK, SteveLasker, gokarnm, justincormack, patrickzheng200, priteshbandi, rgnote and shizhMSFT as code owners March 3, 2023 16:24

priteshbandi reviewed Mar 3, 2023

View reviewed changes

Moving mutual exclusion logic and removing timestamp

495a872

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl commented Mar 4, 2023

View reviewed changes

JeyJeyGao reviewed Mar 6, 2023

View reviewed changes

patrickzheng200 reviewed Mar 6, 2023

View reviewed changes

Switching to cobra commands for option checking

38eeaaf

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl commented Mar 6, 2023

View reviewed changes

kody-kimberl requested review from JeyJeyGao, patrickzheng200 and priteshbandi and removed request for JeyJeyGao, patrickzheng200 and priteshbandi March 6, 2023 15:52

ningziwen mentioned this pull request Mar 6, 2023

Add notation signing and verification containerd/nerdctl#2035

Merged

priteshbandi reviewed Mar 6, 2023

View reviewed changes

Moving flag marking from sign to options

4486cb4

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

priteshbandi previously approved these changes Mar 6, 2023

View reviewed changes

patrickzheng200 reviewed Mar 7, 2023

View reviewed changes

internal/cmd/flags.go Outdated Show resolved Hide resolved

Modifying usage statement for plugin option

a11b8cf

Signed-off-by: Kody Kimberl <kody.kimberl.work@gmail.com>

kody-kimberl dismissed priteshbandi’s stale review via a11b8cf March 7, 2023 01:55

JeyJeyGao approved these changes Mar 7, 2023

View reviewed changes

priteshbandi approved these changes Mar 7, 2023

View reviewed changes

patrickzheng200 approved these changes Mar 7, 2023

View reviewed changes

priteshbandi merged commit 9b714c7 into notaryproject:main Mar 7, 2023

yizha1 mentioned this pull request Mar 7, 2023

Release Notation CLI v1.0.0-rc.3 #582

Closed

3 tasks

priteshbandi mentioned this pull request Mar 7, 2023

Simplify signing experience #537

Closed

FeynmanZhou mentioned this pull request Mar 7, 2023

Add an example to CLI help info for notation signing #584

Closed

Conversation

kody-kimberl commented Mar 3, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kody-kimberl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 6, 2023

Codecov Report

Uh oh!

kody-kimberl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priteshbandi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JeyJeyGao left a comment

Choose a reason for hiding this comment

Uh oh!

patrickzheng200 left a comment

Choose a reason for hiding this comment

Uh oh!

FeynmanZhou commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

FeynmanZhou commented Mar 7, 2023 •

edited

Loading