feat: use new verify workflow by binbin-li · Pull Request #373 · notaryproject/notation

binbin-li · 2022-10-11T05:10:24Z

What?

Use the new verification workflow for the verify command. Therefore, we have to make some updates to support it.

Remove all local verification related flags and arguments, including signatures, certs, certFiles, pull, local and mediaType.
Add config flag so that users can set up verification plugin configs.
Use the new verification workflow.
Support both tag and digest reference.
Format the result.

Signed-off-by: Binbin Li libinbin@microsoft.com

cmd/notation/verify.go

shizhMSFT · 2022-10-11T06:32:58Z

cmd/notation/verify.go

-	reference  string
+	SecureFlagOpts
+	reference string
+	config    string


Suggested change

config string

pluginConfig []string

The reason for using string instead of slice is to be consistent with sign and key commands, https://github.com/notaryproject/notation/blob/main/cmd/notation/sign.go#L27 so that I can utilize https://github.com/notaryproject/notation/blob/main/internal/cmd/flags.go#L91 to parse the configs.

shizhMSFT · 2022-10-11T06:35:43Z

cmd/notation/verify.go

-	command.Flags().StringSliceVar(&opts.certFiles, cmd.PflagCertFile.Name, []string{}, "certificate files for verification")
-	command.Flags().BoolVar(&opts.pull, "pull", true, "pull remote signatures before verification")
 	opts.ApplyFlags(command.Flags())
+	command.Flags().StringVarP(&opts.config, "config", "c", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")


We need to refine this after #371

go.mod

cmd/notation/verify.go

priteshbandi · 2022-10-14T05:53:11Z

internal/ioutil/print.go

+	tw := newTabWriter(w)
+
+	if resultErr == nil {
+		fmt.Fprintf(tw, "%s\n", digest)


Suggested change

fmt.Fprintf(tw, "%s\n", digest)

fmt.Fprintf(tw, "Signature verification succeeded for %s\n", digest)

Depending upon the enforcement level in the trust policy configuration, the signature verification might succeed but some validations might have failed(where the enforcement level is logged). In such cases, we want to display the failed validations as warnings.

The signature that passed signature verification would be last signature in SignatureVerificationOutcome array.

https://github.com/notaryproject/notaryproject/blob/main/trust-store-trust-policy-specification.md#signature-verification-details

right, I was thinking to print out the digest for failed signatures with the error for user experience. But seems the returned outcome doesn't contain it. Please correct me if I miss something.

Tracking it in #304

priteshbandi · 2022-10-14T06:13:48Z

internal/ioutil/print.go

+func printOutcomes(tw *tabwriter.Writer, outcomes []*verification.SignatureVerificationOutcome) {
+	if len(outcomes) == 1 {
+		fmt.Println("1 signature failed verification, error is listed as below:")
+	} else {
+		fmt.Printf("%d signatures failed verification, errors are listed as below:\n", len(outcomes))
+	}
+
+	for _, outcome := range outcomes {
+		// TODO: print out the signature digest once the outcome contains it.
+		fmt.Printf("%s\n\n", outcome.Error.Error())
+	}


IMO this error message is not really helpful and should only be printed in debug level.

In non-debug mode, we should display some generic message like:
Signature verification failed for all the xx signatures associated with ${reference}.

In debug mode

Signature verification failed for all the xx signatures associated with ${reference}. ` The failure reasons for each signature are as follows: 1. ${signature-reference}: ${error message} 2. ${signature-reference}: ${error message}

@yizha1 Could you help check if you need to update the spec?

I can update verify CLI spec for this accordingly

Tracking UX improvements in #304

Signed-off-by: Binbin Li <libinbin@microsoft.com>

codecov-commenter · 2022-10-14T06:24:33Z

Codecov Report

Merging #373 (8329127) into main (b9f1fb5) will increase coverage by 0.24%.
The diff coverage is 12.82%.

@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
+ Coverage   32.03%   32.28%   +0.24%     
==========================================
  Files          26       26              
  Lines        1670     1623      -47     
==========================================
- Hits          535      524      -11     
+ Misses       1122     1086      -36     
  Partials       13       13

Impacted Files	Coverage Δ
cmd/notation/verify.go	`29.16% <12.82%> (+2.27%)`	⬆️

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

Signed-off-by: Binbin Li <libinbin@microsoft.com>

priteshbandi

LGTM

priteshbandi · 2022-10-17T04:44:12Z

internal/ioutil/print.go

+func printOutcomes(tw *tabwriter.Writer, outcomes []*verification.SignatureVerificationOutcome) {
+	if len(outcomes) == 1 {
+		fmt.Println("1 signature failed verification, error is listed as below:")
+	} else {
+		fmt.Printf("%d signatures failed verification, errors are listed as below:\n", len(outcomes))
+	}
+
+	for _, outcome := range outcomes {
+		// TODO: print out the signature digest once the outcome contains it.
+		fmt.Printf("%s\n\n", outcome.Error.Error())
+	}


Tracking UX improvements in #304

Signed-off-by: Binbin Li <libinbin@microsoft.com>

JeyJeyGao · 2022-10-17T06:03:45Z

cmd/notation/verify.go

-		Short: "Verify OCI artifacts",
-		Long: `Verify OCI artifacts
-
-Prerequisite: a trusted certificate needs to be generated or added using the command "notation cert". 


Do we need to delete it?

I'll keep the Prerequisite comment.

rgnote · 2022-10-18T01:27:13Z

cmd/notation/verify.go

+		Use:   "verify [flags] <reference>",
+		Short: "Verifies OCI Artifacts",
+		Long: `Verifies OCI Artifacts:
+  notation verify [--config <key>=<value>,...] [--username <username>] [--password <password>] <reference>`,


Suggested change

notation verify [--config <key>=<value>,...] [--username <username>] [--password <password>] <reference>`,

notation verify [--pluginConfig <key>=<value>,...] [--username <username>] [--password <password>] <reference>`,

rgnote · 2022-10-18T01:28:24Z

cmd/notation/verify.go

-	command.Flags().StringSliceVar(&opts.certFiles, cmd.PflagCertFile.Name, []string{}, "certificate files for verification")
-	command.Flags().BoolVar(&opts.pull, "pull", true, "pull remote signatures before verification")
 	opts.ApplyFlags(command.Flags())
+	command.Flags().StringVarP(&opts.pluginConfig, "config", "c", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")


Suggested change

command.Flags().StringVarP(&opts.pluginConfig, "config", "c", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")

command.Flags().StringVarP(&opts.pluginConfig, "pluginConfig", "pc", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")

Actually Cobra doesn't support shorthand with more than one character.

rgnote · 2022-10-18T01:45:06Z

internal/ioutil/print.go

+
+func printOutcomes(tw *tabwriter.Writer, outcomes []*verification.SignatureVerificationOutcome, digest string) {
+	fmt.Printf("Signature verification failed for all the %d signatures associated with digest: %s\n", len(outcomes), digest)
+


I understand that we are working on a spec to describe the CLI inputs and outputs, but we should print the error messages associated with the individual outcomes in the short-term to make this command more useful. Can you print the outcomes like below in the interim?

Signature verification failed for all the 3 signatures associated with digest: asdf....asdfasdf Signature #1 : <Error> Signature #2 : <Error> Signature #3 : <Error>

Good call, I'll add error messages.

rgnote · 2022-10-18T01:46:50Z

cmd/notation/verify.go

+	outcomes, err := verifier.Verify(ctx, ref.String())
+
+	// write out.
+	return ioutil.PrintVerificationResults(os.Stdout, outcomes, err, ref.Reference)


We should return the error from verifier.Verify() here so that CLI can throw a non-zero error code when signature verification fails.

patrickzheng200

LGTM

Signed-off-by: Binbin Li <libinbin@microsoft.com>

priteshbandi · 2022-10-18T04:28:28Z

cmd/notation/verify_test.go

 		"--plain-http",
-		"--pull=false",
-		"--media-type=mediaT"}); err != nil {
+		"--pluginConfig", expected.pluginConfig}); err != nil {


Suggested change

"--pluginConfig", expected.pluginConfig}); err != nil {

"--plugin-config", expected.pluginConfig}); err != nil {

Can we use kebab-case everywhere for command input?

priteshbandi · 2022-10-18T04:32:08Z

internal/ioutil/print.go

+
+	if resultErr == nil {
+		fmt.Fprintf(tw, "Signature verification succeeded for %s\n", digest)
+		// TODO: print out failed validations as warnings.


Can you please also add issue with TODO: #304
Would allow easier tracking.

Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li force-pushed the verify-cmd branch from bca6915 to 24773b3 Compare October 11, 2022 05:12

shizhMSFT reviewed Oct 11, 2022

View reviewed changes

dtzar mentioned this pull request Oct 13, 2022

update verify command to use ts/tp based verification workflow #370

Closed

priteshbandi reviewed Oct 14, 2022

View reviewed changes

binbin-li added 2 commits October 14, 2022 06:20

feat: use new verify workflow

b9b1209

Signed-off-by: Binbin Li <libinbin@microsoft.com>

refactor: refactor

819281d

Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li force-pushed the verify-cmd branch from e070e5a to 819281d Compare October 14, 2022 06:21

refactor: address pr comments

2860bb2

Signed-off-by: Binbin Li <libinbin@microsoft.com>

priteshbandi reviewed Oct 17, 2022

View reviewed changes

priteshbandi mentioned this pull request Oct 17, 2022

UX: Improve the help doc and output for verification #304

Closed

priteshbandi approved these changes Oct 17, 2022

View reviewed changes

refactor: refactor result format

6ad5608

Signed-off-by: Binbin Li <libinbin@microsoft.com>

JeyJeyGao reviewed Oct 17, 2022

View reviewed changes

rgnote reviewed Oct 18, 2022

View reviewed changes

yizha1 linked an issue Oct 18, 2022 that may be closed by this pull request

Implementing verify command #216

Closed

patrickzheng200 approved these changes Oct 18, 2022

View reviewed changes

refactor: refator input/output format

8b66938

Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li force-pushed the verify-cmd branch from 4660b79 to 8b66938 Compare October 18, 2022 04:26

priteshbandi reviewed Oct 18, 2022

View reviewed changes

priteshbandi self-requested a review October 18, 2022 04:29

priteshbandi reviewed Oct 18, 2022

View reviewed changes

refactor: todo to issue link

8329127

Signed-off-by: Binbin Li <libinbin@microsoft.com>

binbin-li merged commit 20b9fa2 into notaryproject:main Oct 18, 2022

7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025

feat: use new verify workflow (notaryproject#373)

ee9f958

Signed-off-by: Binbin Li <libinbin@microsoft.com>

FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025

feat: use new verify workflow (notaryproject#373)

9aacff1

Signed-off-by: Binbin Li <libinbin@microsoft.com>

	fmt.Fprintf(tw, "%s\n", digest)
	fmt.Fprintf(tw, "Signature verification succeeded for %s\n", digest)

	notation verify [--config <key>=<value>,...] [--username <username>] [--password <password>] <reference>`,
	notation verify [--pluginConfig <key>=<value>,...] [--username <username>] [--password <password>] <reference>`,

	command.Flags().StringVarP(&opts.pluginConfig, "config", "c", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")
	command.Flags().StringVarP(&opts.pluginConfig, "pluginConfig", "pc", "", "list of comma-separated {key}={value} pairs that are passed as is to the plugin")


		func printOutcomes(tw tabwriter.Writer, outcomes []verification.SignatureVerificationOutcome, digest string) {
		fmt.Printf("Signature verification failed for all the %d signatures associated with digest: %s\n", len(outcomes), digest)

	"--pluginConfig", expected.pluginConfig}); err != nil {
	"--plugin-config", expected.pluginConfig}); err != nil {

Comments

Conversation

binbin-li commented Oct 11, 2022

What?

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!

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!

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!

priteshbandi Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

priteshbandi left a comment

Choose a reason for hiding this comment

Uh oh!

priteshbandi Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

patrickzheng200 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

priteshbandi Oct 17, 2022 •

edited

Loading

codecov-commenter commented Oct 14, 2022 •

edited

Loading

priteshbandi Oct 17, 2022 •

edited

Loading