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: add spec for notation verify command #371

Merged
merged 13 commits into from
Oct 18, 2022

Conversation

yizha1
Copy link
Contributor

@yizha1 yizha1 commented Oct 8, 2022

No description provided.

@yizha1 yizha1 added cli Issue or PR released to Notation CLI spec Specifications to define the product requirements labels Oct 8, 2022
@yizha1 yizha1 self-assigned this Oct 8, 2022
@yizha1 yizha1 added this to the RC-1 milestone Oct 8, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2022

Codecov Report

Merging #371 (68489cb) into main (6e8e9c5) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
+ Coverage   30.54%   30.70%   +0.16%     
==========================================
  Files          25       26       +1     
  Lines        1614     1638      +24     
==========================================
+ Hits          493      503      +10     
- Misses       1108     1122      +14     
  Partials       13       13              
Impacted Files Coverage Δ
cmd/notation/push.go 21.91% <0.00%> (-2.70%) ⬇️
internal/cmd/flags.go 52.74% <0.00%> (-1.80%) ⬇️
cmd/notation/verify.go 17.92% <0.00%> (-0.71%) ⬇️
internal/cmd/signer.go 0.00% <0.00%> (ø)
internal/cmd/options.go 0.00% <0.00%> (ø)
cmd/notation/cert_gen.go 0.00% <0.00%> (ø)
internal/envelope/envelope.go 100.00% <0.00%> (ø)

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

@yizha1 yizha1 linked an issue Oct 8, 2022 that may be closed by this pull request
2 tasks
@yizha1 yizha1 requested a review from binbin-li October 10, 2022 02:49
Copy link
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

Thanks @yizha1! Added some feedback.

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
Copy link
Contributor

@priteshbandi priteshbandi 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 below suggestions.

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
specs/commandline/verify.md Outdated Show resolved Hide resolved
@yizha1 yizha1 requested a review from shizhMSFT October 12, 2022 12:36
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

@priteshbandi priteshbandi removed this from the RC-1 milestone Oct 12, 2022
Copy link
Contributor

@gokarnm gokarnm 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

@dtzar dtzar left a comment

Choose a reason for hiding this comment

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

Open to address these before or after merge, so mark for approve.

notation verify registry.wabbit-networks.io/software/net-monitor:v1
```

### Verify signatures on an OCI artifact stored in a registry (Trust store and trust policy are configured properly)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing as to OCI artifact versus trust * configured versus not.
I'd consider removing the distinction of OCI artifact per section. Place the tag AND digest verification options for image & OCI artifact in the same properly configured section.

I'd then have a section on what the experience is like when trust store and policy is not configured. I'd imagine you wouldn't get a verification, but show some error message(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a new PR to address this comment and the error handling parts.


# Configure trust policy by creating a JSON document named "trustpolicy.json" under directory "{NOTATION_CONFIG}"
# Example on Linux
cat <<EOF > $HOME/.config/notation/trustpolicy.json
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to vary based on the user's operating system. IMO we should consider if we can get a "bare bones" cli implementation which simply writes a template similar to what's there to the user's proper directory (i.e. notation policy create -n default) and possibly opens it up or lists where it is at. (i.e. notation policy list or notation policy open (optional --name default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a new issue for a discussion regarding this request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verify signatures associated with the artifact.

Usage:
notation verify [flags] <reference>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should explain here and in the sign command that < reference > support can be a tag or a digest. When with a tag, we do a default tag to digest translation and we only sign/verify the digest, not the tag itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a new PR to address this

@yizha1 yizha1 mentioned this pull request Oct 17, 2022
@shizhMSFT shizhMSFT merged commit ecb0708 into notaryproject:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issue or PR released to Notation CLI spec Specifications to define the product requirements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update notation verify command for RC1
7 participants