feat: use new verify workflow #373

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

 	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

 	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

 	}
 	if err := command.ParseFlags([]string{
 		expected.reference,
 		"--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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use new verify workflow #373

feat: use new verify workflow #373

binbin-li commented Oct 11, 2022

shizhMSFT Oct 11, 2022

binbin-li Oct 12, 2022

shizhMSFT Oct 11, 2022

priteshbandi Oct 14, 2022

priteshbandi Oct 14, 2022

binbin-li Oct 14, 2022

priteshbandi Oct 17, 2022

priteshbandi Oct 14, 2022

binbin-li Oct 14, 2022

yizha1 Oct 17, 2022

priteshbandi Oct 17, 2022 •

edited

Loading

codecov-commenter commented Oct 14, 2022 •

edited

Loading

priteshbandi left a comment

priteshbandi Oct 17, 2022 •

edited

Loading

JeyJeyGao Oct 17, 2022

binbin-li Oct 18, 2022

rgnote Oct 18, 2022

rgnote Oct 18, 2022

binbin-li Oct 18, 2022

rgnote Oct 18, 2022

binbin-li Oct 18, 2022

rgnote Oct 18, 2022

patrickzheng200 left a comment

priteshbandi Oct 18, 2022

priteshbandi Oct 18, 2022

		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")

	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>`,

		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")


		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 {

feat: use new verify workflow #373

feat: use new verify workflow #373

Conversation

binbin-li commented Oct 11, 2022

What?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 14, 2022 • edited Loading

Codecov Report

priteshbandi left a comment

Choose a reason for hiding this comment

priteshbandi Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickzheng200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi Oct 17, 2022 •

edited

Loading

codecov-commenter commented Oct 14, 2022 •

edited

Loading

priteshbandi Oct 17, 2022 •

edited

Loading