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 crl basic functionality with built-in cache #1890

Merged
merged 22 commits into from
Nov 8, 2024

Conversation

junczhu
Copy link
Collaborator

@junczhu junczhu commented Oct 23, 2024

Description

Enables CRL revocation with file cache during verification

What this PR does / why we need it:

The implementation is from notation cli covering CRL support CRL file-based cache support.
Removed HTTP timeout.
Related PR: notaryproject/notation#1043
Related File: https://github.com/notaryproject/notation/commits/main/cmd/notation/verify.go

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 #1889

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • This change requires a documentation update

How Has This Been Tested?

Manual test scenario: verify a signature with revocation check enabled.
image

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

Signed-off-by: Juncheng Zhu <[email protected]>
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
@junczhu
Copy link
Collaborator Author

junczhu commented Oct 25, 2024

Thanks for the review and as discussed, I would use a wrapper to reuse the notation-core-go fetcher to save us from maintain work.

Signed-off-by: Juncheng Zhu <[email protected]>
Signed-off-by: Juncheng Zhu <[email protected]>
Signed-off-by: Juncheng Zhu <[email protected]>
@junczhu junczhu marked this pull request as ready for review October 28, 2024 17:06
Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR JunCheng. In general i think we need to add more detail description on the method and input/output.

pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/provider.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/crl_fetcher.go Outdated Show resolved Hide resolved
pkg/crlprovider/provider.go Outdated Show resolved Hide resolved
pkg/crlprovider/provider.go Outdated Show resolved Hide resolved
pkg/crlprovider/provider.go Outdated Show resolved Hide resolved
pkg/crlprovider/provider.go Outdated Show resolved Hide resolved
Signed-off-by: Juncheng Zhu <[email protected]>
@junczhu junczhu marked this pull request as draft October 31, 2024 00:48
@junczhu
Copy link
Collaborator Author

junczhu commented Oct 31, 2024

Thank you for all the comments and I am working on them in the implementation PR #1900
cc\ @binbin-li @susanshi @akashsinghal @shahramk64

@junczhu junczhu changed the title feat: crl fetcher feat: crl basic functionality Oct 31, 2024
@junczhu junczhu marked this pull request as ready for review October 31, 2024 05:16
@junczhu junczhu changed the title feat: crl basic functionality feat: add support for crl basic functionality with built-in cache Oct 31, 2024
@junczhu junczhu marked this pull request as draft November 1, 2024 02:32
@junczhu junczhu changed the base branch from crl to dev November 4, 2024 02:47
@junczhu junczhu marked this pull request as ready for review November 4, 2024 03:36
@junczhu
Copy link
Collaborator Author

junczhu commented Nov 4, 2024

Newly introduce error returns are wrapped by return nil, re.ErrorCodePluginInitFailure.WithDetail("Failed to create the Notation Verifier").WithError(err) on line 115
image

@junczhu junczhu enabled auto-merge (squash) November 4, 2024 03:43
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/verifier/notation/notationrevocationfactory.go 73.91% 4 Missing and 2 partials ⚠️
Files with missing lines Coverage Δ
pkg/verifier/notation/notation.go 98.05% <100.00%> (+0.30%) ⬆️
pkg/verifier/notation/notationrevocationfactory.go 73.91% <73.91%> (ø)

@junczhu junczhu marked this pull request as draft November 5, 2024 07:29
auto-merge was automatically disabled November 5, 2024 07:29

Pull request was converted to draft

@junczhu junczhu marked this pull request as ready for review November 6, 2024 08:48
pkg/verifier/notation/notationrevocationfactory.go Outdated Show resolved Hide resolved
pkg/verifier/notation/revocationfactory.go Outdated Show resolved Hide resolved
pkg/verifier/notation/revocationfactory.go Outdated Show resolved Hide resolved
@junczhu junczhu mentioned this pull request Nov 8, 2024
12 tasks
@susanshi
Copy link
Collaborator

susanshi commented Nov 8, 2024

Hi @junczhu , in the PR description you mentioned "Under manual testing." Could you add the manual validation scenarios? thanks.

Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

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

LGTM. thanks Juncheng

@junczhu junczhu merged commit 4510dd8 into ratify-project:dev Nov 8, 2024
20 checks passed
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.

feat: CRL fetcher
5 participants