feat: simplify signing experience (notaryproject#579)

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 <[email protected]>
patrickzheng200 · Mar 7, 2023 · 9b714c7 · 9b714c7
1 parent 0f26496
commit 9b714c7
Show file tree

Hide file tree

Showing 5 changed files with 247 additions and 12 deletions.
diff --git a/cmd/notation/sign.go b/cmd/notation/sign.go
@@ -80,7 +80,7 @@ Example - [Experimental] Sign an OCI artifact and use OCI artifact manifest to s
 		},
 	}
 	opts.LoggingFlagOpts.ApplyFlags(command.Flags())
-	opts.SignerFlagOpts.ApplyFlags(command.Flags())
+	opts.SignerFlagOpts.ApplyFlagsToCommand(command)
 	opts.SecureFlagOpts.ApplyFlags(command.Flags())
 	cmd.SetPflagExpiry(command.Flags(), &opts.expiry)
 	cmd.SetPflagPluginConfig(command.Flags(), &opts.pluginConfig)
@@ -94,7 +94,7 @@ func runSign(command *cobra.Command, cmdOpts *signOpts) error {
 	ctx := cmdOpts.LoggingFlagOpts.SetLoggerLevel(command.Context())
 
 	// initialize
-	signer, err := cmd.GetSigner(&cmdOpts.SignerFlagOpts)
+	signer, err := cmd.GetSigner(ctx, &cmdOpts.SignerFlagOpts)
 	if err != nil {
 		return err
 	}

diff --git a/cmd/notation/sign_test.go b/cmd/notation/sign_test.go
@@ -123,6 +123,214 @@ func TestSignCommand_CorrectConfig(t *testing.T) {
 	}
 }
 
+func TestSignCommmand_OnDemandKeyOptions(t *testing.T) {
+	opts := &signOpts{}
+	command := signCommand(opts)
+	expected := &signOpts{
+		reference: "ref",
+		SecureFlagOpts: SecureFlagOpts{
+			Username: "user",
+			Password: "password",
+		},
+		SignerFlagOpts: cmd.SignerFlagOpts{
+			KeyID:           "keyID",
+			PluginName:      "pluginName",
+			SignatureFormat: envelope.JWS,
+		},
+		signatureManifest: "image",
+	}
+	if err := command.ParseFlags([]string{
+		expected.reference,
+		"-u", expected.Username,
+		"--password", expected.Password,
+		"--id", expected.KeyID,
+		"--plugin", expected.PluginName}); err != nil {
+		t.Fatalf("Parse Flag failed: %v", err)
+	}
+	if err := command.Args(command, command.Flags().Args()); err != nil {
+		t.Fatalf("Parse args failed: %v", err)
+	}
+	if !reflect.DeepEqual(*expected, *opts) {
+		t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+	}
+}
+
+func TestSignCommmand_OnDemandKeyBadOptions(t *testing.T) {
+	t.Run("error when using id and plugin options with key", func(t *testing.T) {
+		opts := &signOpts{}
+		command := signCommand(opts)
+		expected := &signOpts{
+			reference: "ref",
+			SecureFlagOpts: SecureFlagOpts{
+				Username: "user",
+				Password: "password",
+			},
+			SignerFlagOpts: cmd.SignerFlagOpts{
+				KeyID:           "keyID",
+				PluginName:      "pluginName",
+				Key:             "keyName",
+				SignatureFormat: envelope.JWS,
+			},
+			signatureManifest: "image",
+		}
+		if err := command.ParseFlags([]string{
+			expected.reference,
+			"-u", expected.Username,
+			"--password", expected.Password,
+			"--id", expected.KeyID,
+			"--plugin", expected.PluginName,
+			"--key", expected.Key}); err != nil {
+			t.Fatalf("Parse Flag failed: %v", err)
+		}
+		if err := command.Args(command, command.Flags().Args()); err != nil {
+			t.Fatalf("Parse args failed: %v", err)
+		}
+		if !reflect.DeepEqual(*expected, *opts) {
+			t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+		}
+		err := command.ValidateFlagGroups()
+		if err == nil || err.Error() != "if any flags in the group [key id] are set none of the others can be; [id key] were all set" {
+			t.Fatalf("Didn't get the expected error, but got: %v", err)
+		}
+	})
+	t.Run("error when using key and id options", func(t *testing.T) {
+		opts := &signOpts{}
+		command := signCommand(opts)
+		expected := &signOpts{
+			reference: "ref",
+			SecureFlagOpts: SecureFlagOpts{
+				Username: "user",
+				Password: "password",
+			},
+			SignerFlagOpts: cmd.SignerFlagOpts{
+				KeyID:           "keyID",
+				Key:             "keyName",
+				SignatureFormat: envelope.JWS,
+			},
+			signatureManifest: "image",
+		}
+		if err := command.ParseFlags([]string{
+			expected.reference,
+			"-u", expected.Username,
+			"--password", expected.Password,
+			"--id", expected.KeyID,
+			"--key", expected.Key}); err != nil {
+			t.Fatalf("Parse Flag failed: %v", err)
+		}
+		if err := command.Args(command, command.Flags().Args()); err != nil {
+			t.Fatalf("Parse args failed: %v", err)
+		}
+		if !reflect.DeepEqual(*expected, *opts) {
+			t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+		}
+		err := command.ValidateFlagGroups()
+		if err == nil || err.Error() != "if any flags in the group [id plugin] are set they must all be set; missing [plugin]" {
+			t.Fatalf("Didn't get the expected error, but got: %v", err)
+		}
+	})
+	t.Run("error when using key and plugin options", func(t *testing.T) {
+		opts := &signOpts{}
+		command := signCommand(opts)
+		expected := &signOpts{
+			reference: "ref",
+			SecureFlagOpts: SecureFlagOpts{
+				Username: "user",
+				Password: "password",
+			},
+			SignerFlagOpts: cmd.SignerFlagOpts{
+				PluginName:      "pluginName",
+				Key:             "keyName",
+				SignatureFormat: envelope.JWS,
+			},
+			signatureManifest: "image",
+		}
+		if err := command.ParseFlags([]string{
+			expected.reference,
+			"-u", expected.Username,
+			"--password", expected.Password,
+			"--plugin", expected.PluginName,
+			"--key", expected.Key}); err != nil {
+			t.Fatalf("Parse Flag failed: %v", err)
+		}
+		if err := command.Args(command, command.Flags().Args()); err != nil {
+			t.Fatalf("Parse args failed: %v", err)
+		}
+		if !reflect.DeepEqual(*expected, *opts) {
+			t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+		}
+		err := command.ValidateFlagGroups()
+		if err == nil || err.Error() != "if any flags in the group [id plugin] are set they must all be set; missing [id]" {
+			t.Fatalf("Didn't get the expected error, but got: %v", err)
+		}
+	})
+	t.Run("error when using id option and not plugin", func(t *testing.T) {
+		opts := &signOpts{}
+		command := signCommand(opts)
+		expected := &signOpts{
+			reference: "ref",
+			SecureFlagOpts: SecureFlagOpts{
+				Username: "user",
+				Password: "password",
+			},
+			SignerFlagOpts: cmd.SignerFlagOpts{
+				KeyID:           "keyID",
+				SignatureFormat: envelope.JWS,
+			},
+			signatureManifest: "image",
+		}
+		if err := command.ParseFlags([]string{
+			expected.reference,
+			"-u", expected.Username,
+			"--password", expected.Password,
+			"--id", expected.KeyID}); err != nil {
+			t.Fatalf("Parse Flag failed: %v", err)
+		}
+		if err := command.Args(command, command.Flags().Args()); err != nil {
+			t.Fatalf("Parse args failed: %v", err)
+		}
+		if !reflect.DeepEqual(*expected, *opts) {
+			t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+		}
+		err := command.ValidateFlagGroups()
+		if err == nil || err.Error() != "if any flags in the group [id plugin] are set they must all be set; missing [plugin]" {
+			t.Fatalf("Didn't get the expected error, but got: %v", err)
+		}
+	})
+	t.Run("error when using plugin option and not id", func(t *testing.T) {
+		opts := &signOpts{}
+		command := signCommand(opts)
+		expected := &signOpts{
+			reference: "ref",
+			SecureFlagOpts: SecureFlagOpts{
+				Username: "user",
+				Password: "password",
+			},
+			SignerFlagOpts: cmd.SignerFlagOpts{
+				PluginName:      "pluginName",
+				SignatureFormat: envelope.JWS,
+			},
+			signatureManifest: "image",
+		}
+		if err := command.ParseFlags([]string{
+			expected.reference,
+			"-u", expected.Username,
+			"--password", expected.Password,
+			"--plugin", expected.PluginName}); err != nil {
+			t.Fatalf("Parse Flag failed: %v", err)
+		}
+		if err := command.Args(command, command.Flags().Args()); err != nil {
+			t.Fatalf("Parse args failed: %v", err)
+		}
+		if !reflect.DeepEqual(*expected, *opts) {
+			t.Fatalf("Expect sign opts: %v, got: %v", expected, opts)
+		}
+		err := command.ValidateFlagGroups()
+		if err == nil || err.Error() != "if any flags in the group [id plugin] are set they must all be set; missing [id]" {
+			t.Fatalf("Didn't get the expected error, but got: %v", err)
+		}
+	})
+}
+
 func TestSignCommand_MissingArgs(t *testing.T) {
 	cmd := signCommand(nil)
 	if err := cmd.ParseFlags(nil); err != nil {

diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go
@@ -20,7 +20,7 @@ var (
 	PflagKey = &pflag.Flag{
 		Name:      "key",
 		Shorthand: "k",
-		Usage:     "signing key name, for a key previously added to notation's key list.",
+		Usage:     "signing key name, for a key previously added to notation's key list. This is mutually exclusive with the --id and --plugin flags",
 	}
 	SetPflagKey = func(fs *pflag.FlagSet, p *string) {
 		fs.StringVarP(p, PflagKey.Name, PflagKey.Shorthand, "", PflagKey.Usage)
@@ -41,13 +41,20 @@ var (
 		fs.StringVar(p, PflagSignatureFormat.Name, defaultSignatureFormat, PflagSignatureFormat.Usage)
 	}
 
-	PflagTimestamp = &pflag.Flag{
-		Name:      "timestamp",
-		Shorthand: "t",
-		Usage:     "timestamp the signed signature via the remote TSA",
+	PflagID = &pflag.Flag{
+		Name:  "id",
+		Usage: "key id (required if --plugin is set). This is mutually exclusive with the --key flag",
 	}
-	SetPflagTimestamp = func(fs *pflag.FlagSet, p *string) {
-		fs.StringVarP(p, PflagTimestamp.Name, PflagTimestamp.Shorthand, "", PflagTimestamp.Usage)
+	SetPflagID = func(fs *pflag.FlagSet, p *string) {
+		fs.StringVar(p, PflagID.Name, "", PflagID.Usage)
+	}
+
+	PflagPlugin = &pflag.Flag{
+		Name:  "plugin",
+		Usage: "signing plugin name (required if --id is set). This is mutually exclusive with the --key flag",
+	}
+	SetPflagPlugin = func(fs *pflag.FlagSet, p *string) {
+		fs.StringVar(p, PflagPlugin.Name, "", PflagPlugin.Usage)
 	}
 
 	PflagExpiry = &pflag.Flag{

diff --git a/internal/cmd/options.go b/internal/cmd/options.go
@@ -5,19 +5,28 @@ import (
 
 	"github.com/notaryproject/notation/internal/trace"
 	"github.com/sirupsen/logrus"
+	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
 
 // SignerFlagOpts cmd opts for using cmd.GetSigner
 type SignerFlagOpts struct {
 	Key             string
 	SignatureFormat string
+	KeyID           string
+	PluginName      string
 }
 
 // ApplyFlags set flags and their default values for the FlagSet
-func (opts *SignerFlagOpts) ApplyFlags(fs *pflag.FlagSet) {
+func (opts *SignerFlagOpts) ApplyFlagsToCommand(command *cobra.Command) {
+	fs := command.Flags()
 	SetPflagKey(fs, &opts.Key)
 	SetPflagSignatureFormat(fs, &opts.SignatureFormat)
+	SetPflagID(fs, &opts.KeyID)
+	SetPflagPlugin(fs, &opts.PluginName)
+	command.MarkFlagsRequiredTogether("id", "plugin")
+	command.MarkFlagsMutuallyExclusive("key", "id")
+	command.MarkFlagsMutuallyExclusive("key", "plugin")
 }
 
 // LoggingFlagOpts option struct.

diff --git a/internal/cmd/signer.go b/internal/cmd/signer.go
@@ -12,7 +12,18 @@ import (
 )
 
 // GetSigner returns a signer according to the CLI context.
-func GetSigner(opts *SignerFlagOpts) (notation.Signer, error) {
+func GetSigner(ctx context.Context, opts *SignerFlagOpts) (notation.Signer, error) {
+	// Check if using on-demand key
+	if opts.KeyID != "" && opts.PluginName != "" && opts.Key == "" {
+		// Construct a signer from on-demand key
+		mgr := plugin.NewCLIManager(dir.PluginFS())
+		plugin, err := mgr.Get(ctx, opts.PluginName)
+		if err != nil {
+			return nil, err
+		}
+		return signer.NewFromPlugin(plugin, opts.KeyID, map[string]string{})
+	}
+
 	// 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)
@@ -26,7 +37,7 @@ func GetSigner(opts *SignerFlagOpts) (notation.Signer, error) {
 	// corresponds to an external key
 	if key.ExternalKey != nil {
 		mgr := plugin.NewCLIManager(dir.PluginFS())
-		plugin, err := mgr.Get(context.Background(), key.PluginName)
+		plugin, err := mgr.Get(ctx, key.PluginName)
 		if err != nil {
 			return nil, err
 		}