-
Notifications
You must be signed in to change notification settings - Fork 64
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: set default certstore namespace in notation verifier to uniquely identify certificate store resource #1134
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1134 +/- ##
==========================================
+ Coverage 52.31% 52.39% +0.08%
==========================================
Files 101 101
Lines 6306 6344 +38
==========================================
+ Hits 3299 3324 +25
- Misses 2688 2699 +11
- Partials 319 321 +2
☔ View full report in Codecov by Sentry. |
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.
wonder if we need to update Verifier CRD to namespaced?
I think we should eventually , but we have to think more about the usecase of having namespaced verifier. This change future proofs the certStore scenario by checking if there is a verifier namespace first. |
Hi @binbin-li , @akashsinghal , please review the latest. thanks! |
Signed-off-by: Binbin Li <[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 overall!
pkg/verifier/notation/notation.go
Outdated
defaultCertPath = "ratify-certs/notation/truststore" | ||
verifierName = "notation" | ||
defaultCertPath = "ratify-certs/notation/truststore" | ||
namespaceSeperator = "/" |
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.
nit: is this universal? should this also be moved to global constants?
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.
moved to internal const. thanks!
Co-authored-by: Akash Singhal <[email protected]> Signed-off-by: Susan Shi <[email protected]>
updates pushed, @binbin-li @akashsinghal , please review when convenient. thanks! |
Description
Fixes #1061
What this PR does / why we need it:
currently certStore defined in trust policy only contains name which means the CertStore cannot be uniquely identified. To address , the PR introduces below changes:
as a fall back.
Note: a doc PR will be created in the doc repo
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #1061
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?