Skip to content

fix: update notation verify error messages #771

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.

Sign up for GitHub

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

Jump to bottom
Closed
ghost wants to merge 16 commits into from
Closed

fix: update notation verify error messages #771

ghost wants to merge 16 commits into from

Conversation

@ghost
Copy link

@ghost ghost commented Aug 29, 2023 •
edited by ghost
Loading

In this PR:

  1. Updated notation verify error message to display failure reasons for each signature WITHOUT the need to enable -v or -d flags. This is to improve Notation CLI's usability during verification failures.
  2. Improved trust store related error messages readability.

This PR should only be reviewed/merged after its corresponding notation-go PR is merged: notaryproject/notation-go#345.

Based on discussions from issues #699, #700, and #701, the error messages become:

# no certificate in the trust store
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, no x509 certificates were found in trust store "alpine" of type "ca". Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, no x509 certificates were found in trust store "alpine" of type "ca". Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

# the trust store is missing
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, the trust store "alpine" of type "ca" doesn't exist. Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, the trust store "alpine" of type "ca" doesn't exist. Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

# no permission to read the certificate from the trust store
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, failed to read the trusted certificate "/home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt": open /home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt: permission denied
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, failed to read the trusted certificate "/home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt": open /home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt: permission denied
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

Patrick Zheng added 7 commits August 25, 2023 15:25
initial commits
21bee9e 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Merge branch 'notaryproject:main' into errMsg
3fead85
e2e test
5243d1d 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
fix
ae3703f 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
initial commits
e3ef191 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
go mod tidy
76d4c62 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
updated error message
d818c0a 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost added this to the 1.1.0 milestone Aug 29, 2023
This was linked to issues Aug 29, 2023
Misleading error message when trust store is not configured #699
Closed
Misleading error message if no permission to read certificates from trust store #700
Closed
Misleading error message when no certificates were stored in the trust store #701
Closed
@ghost ghost mentioned this pull request Aug 29, 2023
Merged
@ghost ghost changed the title fix: update notation verify error message fix: update notation verify error messages Aug 29, 2023
updated notation-go
920747b 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@priteshbandi
Copy link
Contributor

priteshbandi commented Sep 4, 2023

IMO showing error or warning for each signature in a successful verification scenario is too much information which might confuse user.

@ghost
Copy link
Author

ghost commented Sep 4, 2023

IMO showing error or warning for each signature in a successful verification scenario is too much information which might confuse user.

@priteshbandi Yes, I agree. For a successful verification, none of the above error will be printed out.
On success, the output message of Notation CLI would just be:

$ notation verify testnotation.azurecr.io/hello@sha256:abcdef
Successfully verified signature for testnotation.azurecr.io/hello@sha256:abcdef

/cc: @yizha1 @FeynmanZhou

updated based on notation-go
04a1525 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023 •
edited
Loading

Codecov Report

Merging #771 (aea48fe) into main (b7e5a6f) will increase coverage by 0.22%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #771      +/-   ##
==========================================
+ Coverage   63.98%   64.21%   +0.22%     
==========================================
  Files          40       40              
  Lines        2252     2266      +14     
==========================================
+ Hits         1441     1455      +14     
  Misses        689      689              
  Partials      122      122
Files Changed Coverage Δ
cmd/notation/verify.go 87.07% <100.00%> (+1.36%) ⬆️

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

Patrick Zheng added 3 commits September 18, 2023 13:44
updated notation-go
4747695 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
updated notation-go
215dddf 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
resolved conflicts
08a9dc7 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
shizhMSFT
shizhMSFT reviewed Sep 19, 2023
View reviewed changes
go.mod Outdated
golang.org/x/sys v0.12.0 // indirect
)

replace github.com/notaryproject/notation-go => github.com/Two-Hearts/notation-go v0.0.0-20230918074034-2606b29ba9dc
Copy link
Contributor

@shizhMSFT shizhMSFT Sep 19, 2023

Choose a reason for hiding this comment

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

This line need to be removed after merging notaryproject/notation-go#345

updated per code review
aea48fe 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost requested a review from shizhMSFT September 19, 2023 07:09
shizhMSFT
shizhMSFT reviewed Sep 19, 2023
View reviewed changes
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 but waiting for notaryproject/notation-go#345

@priteshbandi
Copy link
Contributor

priteshbandi commented Sep 22, 2023 

# no certificate in the trust store
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, no x509 certificates were found in trust store "alpine" of type "ca". Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, no x509 certificates were found in trust store "alpine" of type "ca". Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

Ideal experience would be to not even retrieve signature if we know there are no certificates in trust store. Thus the error message becomes.

notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: signature verification failed because no x509 certificates were found in trust store "alpine" of type "ca".

# the trust store is missing
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, the trust store "alpine" of type "ca" doesn't exist. Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, the trust store "alpine" of type "ca" doesn't exist. Use command 'notation cert add' to create and add trusted certificates to the trust store.
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

Ditto, don't even retrieve signature if trust store is missing. Thus the error message becomes.

notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: signature verification failed because the trust store "alpine" of type "ca" doesnot exist.

# no permission to read the certificate from the trust store
$ notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: failed to verify signature with digest sha256:f1356cb804623506ded33ecb78fe413a01ea1ce9b643b13456bf4ac7d2ff9698, failed to read the trusted certificate "/home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt": open /home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt: permission denied
Error: failed to verify signature with digest sha256:f7b98d8402ad46668884c73444b10ae787cbec882670303d4c1e733379eeab51, failed to read the trusted certificate "/home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt": open /home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt: permission denied
Error: signature verification failed for all the signatures associated with testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0

Ditto, don't even retrieve signature if notation cannot read certificate. Thus the error message becomes.

notation verify testnotation.azurecr.io/alpine@sha256:93d5a28ff72d288d69b5997b8ba47396d2cbb62a72b5d87cd3351094b5d578a0
Error: signature verification failed because unable to read the trusted certificate "/home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt": open /home/patrickzheng/.config/notation/truststore/x509/ca/alpine/alpine.crt: permission denied

@ghost
Copy link
Author

ghost commented Sep 23, 2023 •
edited by ghost
Loading

@priteshbandi Thanks for the suggestion, but according to the Notary Project spec, signatures are verified before the signing identity. You could find the spec here, see the last question of FAQ.
This PR is for updating error messages only. If we want to change the verification workflow itself, then that should be another PR in notation-go.

@priteshbandi priteshbandi mentioned this pull request Sep 26, 2023
Open
@priteshbandi
Copy link
Contributor

priteshbandi commented Sep 28, 2023

@priteshbandi Thanks for the suggestion, but according to the Notary Project spec, signatures are verified before the signing identity. You could find the spec here, see the last question of FAQ. This PR is for updating error messages only. If we want to change the verification workflow itself, then that should be another PR in notation-go.

Opened issue to amend spec #790

@ghost ghost closed this Oct 27, 2023
Patrick Zheng added 3 commits October 27, 2023 11:23
resolved conflicts
967c1e7 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
updated notation-go
a97e8b2 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
updated errMsg
e3776e8 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost reopened this Oct 27, 2023
@ghost ghost closed this Oct 27, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@JeyJeyGao JeyJeyGao Awaiting requested review from JeyJeyGao

@justincormack justincormack Awaiting requested review from justincormack

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@priteshbandi priteshbandi Awaiting requested review from priteshbandi priteshbandi is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.1.0

3 participants

@priteshbandi @codecov-commenter @shizhMSFT