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

feat: add support for signed user metadata in notation sign and verify cmds #507

Merged
merged 13 commits into from
Feb 8, 2023

Conversation

byronchien
Copy link
Contributor

@byronchien byronchien commented Jan 13, 2023

Adds support for signed user metadata in notation sign and notation verify. Relevant spec

This PR depends on notaryproject/notation-go#242 please review notation-go/pull/242 first

example sign usage:

chienb@a07817b52895 notation % notation sign $IMAGE --user-metadata io.wabbit-networks.buildId=123 --user-metadata io.wabbit-networks.buildTime=123
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.
Resolved artifact tag `v1` to digest `sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b` before signing.
Successfully signed localhost:5000/net-monitor@sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b

example metadata displayed on verification (without metadata flag)

chienb@a07817b52895 notation % notation verify $IMAGE
Resolved artifact tag `v1` to digest `sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b` before verification.
Warning: The resolved digest may not point to the same signed artifact, since tags are mutable.
Successfully verified signature for localhost:5000/net-monitor@sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b

The artifact was signed with the following user metadata.

KEY                          VALUE
io.wabbit-networks.buildTime   123
io.wabbit-networks.buildId     123

example verification:

chienb@a07817b52895 notation % notation verify $IMAGE --user-metadata io.wabbit-networks.buildTime=123
Resolved artifact tag `v1` to digest `sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b` before verification.
Warning: The resolved digest may not point to the same signed artifact, since tags are mutable.
Successfully verified signature for localhost:5000/net-monitor@sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b

The artifact was signed with the following user metadata.

KEY                            VALUE
io.wabbit-networks.buildTime   123
io.wabbit-networks.buildId     123

example verification failure

chienb@a07817b52895 notation % notation verify $IMAGE --user-metadata foo=bar
Resolved artifact tag `v1` to digest `sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b` before verification.
Warning: The resolved digest may not point to the same signed artifact, since tags are mutable.
Error: signature verification failed: signature verification failed

Not in this PR: error messaging. In the spec update, we mentioned that there should be an error message if the verification fails due to the metadata not being present, but if there are multiple verifications that fail for different reasons, is there a desired order to prioritize the what the end error message displayed is?

Signed-off-by: Byron Chien [email protected]

@byronchien
Copy link
Contributor Author

related: notaryproject/notation-go#242

internal/cmd/flags.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
Signed-off-by: Byron Chien <[email protected]>
cmd/notation/sign.go Show resolved Hide resolved
cmd/notation/verify.go 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

@priteshbandi priteshbandi changed the title Adds additional flag and passes values to notation-go Adds support for signed user metadata in notation sign and verify cmds Feb 1, 2023
@JeyJeyGao
Copy link
Contributor

The pipeline was failed. Please update the code.

@priteshbandi
Copy link
Contributor

priteshbandi commented Feb 1, 2023

The pipeline was failed. Please update the code.

This PR depends on notaryproject/notation-go#242 and that's the reason build is failing.

@JeyJeyGao In the spirit of expediting the rc2 release, can you please review the code, we can make a small update when notaryproject/notation-go#242 is merged ?

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 with one comment.

cmd/notation/sign.go Outdated Show resolved Hide resolved
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

byronchien and others added 2 commits February 6, 2023 16:28
Co-authored-by: Patrick Zheng <[email protected]>

Signed-off-by: Byron Chien <[email protected]>
internal/cmd/flags.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
@yizha1
Copy link
Contributor

yizha1 commented Feb 7, 2023

@byronchien could you fix the conflict?

@byronchien
Copy link
Contributor Author

resolved conflict

@shizhMSFT shizhMSFT changed the title Adds support for signed user metadata in notation sign and verify cmds feat: add support for signed user metadata in notation sign and verify cmds Feb 8, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #507 (de7057b) into main (5c27944) will increase coverage by 0.90%.
The diff coverage is 5.26%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   35.15%   36.06%   +0.90%     
==========================================
  Files          29       29              
  Lines        1502     1528      +26     
==========================================
+ Hits          528      551      +23     
- Misses        955      958       +3     
  Partials       19       19              
Impacted Files Coverage Δ
cmd/notation/key.go 31.95% <0.00%> (ø)
cmd/notation/verify.go 24.13% <5.26%> (-3.14%) ⬇️
cmd/notation/sign.go 41.48% <5.55%> (-2.19%) ⬇️
pkg/configutil/once.go 100.00% <0.00%> (ø)
pkg/configutil/util.go 100.00% <0.00%> (+100.00%) ⬆️

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

@priteshbandi priteshbandi merged commit 51951af into notaryproject:main Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants