Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spec: update cli verify spec for UX improvement #440

Merged
merged 11 commits into from
Dec 2, 2022

Conversation

yizha1
Copy link
Contributor

@yizha1 yizha1 commented Nov 8, 2022

Update cli verify spec for UX improvement

Signed-off-by: Yi Zha yizha1@microsoft.com

Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1 yizha1 added UX User experience changes spec Specifications to define the product requirements labels Nov 8, 2022
@yizha1 yizha1 self-assigned this Nov 8, 2022
@yizha1 yizha1 linked an issue Nov 8, 2022 that may be closed by this pull request
Signed-off-by: Yi Zha <yizha1@microsoft.com>
@FeynmanZhou
Copy link
Member

I saw this PR has also improved the output of notation verify when failing to verify a signature in beta.1 as follows, this is great.

$ notation verify localhost:5000/library/hello-world:latest

ERROR: signature verification failed

Signature verification failed for all the 1 signatures associated with digest: sha256:92c7f9c92844bbbb5d0a101b22f7c2a7949e40f8ea90c8b3bc396879d95e899a

Signature #1 : signature is not produced by a trusted signer

Error: signature verification failed

Copy link
Contributor

@toddysm toddysm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that the cat experience for the trust policy is the best user experience but it seems this is what we can do for now.

specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1 yizha1 requested review from toddysm and removed request for dtzar November 18, 2022 06:17
@yizha1
Copy link
Contributor Author

yizha1 commented Nov 18, 2022

Remove the updates for debug option, which will be solved by separate issue. @toddysm @priteshbandi

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #440 (c0de485) into main (5732b76) will decrease coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
- Coverage   32.82%   32.48%   -0.35%     
==========================================
  Files          25       25              
  Lines        1237     1250      +13     
==========================================
  Hits          406      406              
- Misses        819      832      +13     
  Partials       12       12              
Impacted Files Coverage Δ
cmd/notation/verify.go 22.07% <0.00%> (-2.93%) ⬇️
cmd/notation/cert/generateTest.go 16.66% <0.00%> (-1.26%) ⬇️
cmd/notation/list.go 23.28% <0.00%> (-0.66%) ⬇️
cmd/notation/plugin.go 0.00% <0.00%> (ø)
cmd/notation/registry.go 0.00% <0.00%> (ø)
cmd/notation/key.go 24.75% <0.00%> (+0.24%) ⬆️
cmd/notation/sign.go 45.45% <0.00%> (+8.22%) ⬆️

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

@yizha1 yizha1 added this to the RC-1 milestone Nov 18, 2022
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1
Copy link
Contributor Author

yizha1 commented Nov 23, 2022

@priteshbandi @toddysm New updates:

  • Added a table for example values of trust policy properties.
  • Added directory examples for different OS users.

Signed-off-by: Yi Zha <yizha1@microsoft.com>
Copy link
Contributor

@vaninrao10 vaninrao10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
| registryScopes | "*" | The policy applies to all the artifacts stored in any repositories. |
| signatureVerification | "level": "strict" | Signature verification is performed at strict level, which enforces all validations: `integrity`, `authenticity`, `authentic timestamp`, `expiry` and `revocation`.|
| signatureVerification | "level": "permissive" | The permissive level enforces most validations, but will only logs failures for `revocation` and `expiry`. |
| signatureVerification | "level": "audit" | The audit level only enforces signature `integrity` if a signature is present. Failure of all other validations are only logged. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have log implemented.

specs/commandline/verify.md Show resolved Hide resolved
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Signed-off-by: Yi Zha <yizha1@microsoft.com>
Copy link
Contributor

@vaninrao10 vaninrao10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iamsamirzon
Copy link
Contributor

Suggested two enhancements

Signed-off-by: Yi Zha <yizha1@microsoft.com>
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with notes

Comment on lines 14 to 15
Resolved artifact tag '<tag>' to digest '<digest>' before verification.
Warning: The resolved digest may not point to the same signed artifact, since tags are mutable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: These outputs will be in the form of logs.

Signed-off-by: Yi Zha <yizha1@microsoft.com>
@yizha1 yizha1 dismissed toddysm’s stale review December 2, 2022 04:24

Discussed with Toddy and looks good to him.

@priteshbandi priteshbandi merged commit 2b39cf5 into notaryproject:main Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Specifications to define the product requirements UX User experience changes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

UX: Improve the help doc and output for verification
9 participants