-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix: update notation verify
error messages
#771
Conversation
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
notation verify
error messagenotation verify
error messages
Signed-off-by: Patrick Zheng <[email protected]>
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
/cc: @yizha1 @FeynmanZhou |
Signed-off-by: Patrick Zheng <[email protected]>
Codecov Report
❗ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
go.mod
Outdated
golang.org/x/mod v0.12.0 // indirect | ||
golang.org/x/sync v0.3.0 // indirect | ||
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 |
There was a problem hiding this comment.
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
Signed-off-by: Patrick Zheng <[email protected]>
There was a problem hiding this 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
Ideal experience would be to not even retrieve signature if we know there are no certificates in trust store. Thus the error message becomes.
Ditto, don't even retrieve signature if trust store is missing. Thus the error message becomes.
Ditto, don't even retrieve signature if notation cannot read certificate. Thus the error message becomes.
|
@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. |
Opened issue to amend spec #790 |
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
In this PR:
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.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: