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: adding OCSP revocation implementation #134

Merged
merged 32 commits into from
Apr 20, 2023

Conversation

kody-kimberl
Copy link
Contributor

This PR adds a new package that will perform OCSP revocation checking for a certificate chain.

Based on my design from #132 and the specification here.

This PR addresses part of the following issue:

Once it is approved and merged, a second PR will implement this functionality into notation-go.

Signed-off-by: Kody Kimberl [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2023

Codecov Report

Merging #134 (2a643ff) into main (010204d) will decrease coverage by 1.00%.
The diff coverage is 82.72%.

📣 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     #134      +/-   ##
==========================================
- Coverage   90.32%   89.32%   -1.00%     
==========================================
  Files          16       21       +5     
  Lines        1457     1677     +220     
==========================================
+ Hits         1316     1498     +182     
- Misses        110      142      +32     
- Partials       31       37       +6     
Impacted Files Coverage Δ
signature/types.go 0.00% <0.00%> (ø)
revocation/ocsp/ocsp.go 82.69% <82.69%> (ø)
revocation/ocsp/errors.go 100.00% <100.00%> (ø)
revocation/result/errors.go 100.00% <100.00%> (ø)
revocation/result/results.go 100.00% <100.00%> (ø)
revocation/revocation.go 100.00% <100.00%> (ø)

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

@kody-kimberl
Copy link
Contributor Author

I've made a new commit that address some of the unresolved comments. It may not perfectly address them (as a final decision had not been made for everything), but it should bring many aspects of the implementation closer to everyone's desired solution.

@kody-kimberl
Copy link
Contributor Author

1.18 is failing due to the incorporation of url.JoinPath (which was added in 1.19) to construct the url instead of adding the strings together. Rather than changing this back (since JoinPath is the better option), @shizhMSFT recommended a new PR to update the Go support window (Issue: #115).

The PR can be found here: #135

Signed-off-by: Kody Kimberl <[email protected]>
Signed-off-by: Kody Kimberl <[email protected]>
Signed-off-by: Kody Kimberl <[email protected]>
Signed-off-by: Kody Kimberl <[email protected]>
@kody-kimberl
Copy link
Contributor Author

Signing issue is resolved. Somehow two were unsigned. I rebased reworded to add signatures then force pushed it. The diff befor and after the force push is the same (as seen here), so shouldn't require extensive re-review.

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

Copy link

@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

@priteshbandi priteshbandi merged commit cefe2ef into notaryproject:main Apr 20, 2023
priteshbandi pushed a commit to notaryproject/notation-go that referenced this pull request Apr 20, 2023
This PR adds OCSP revocation checking to the Verify function using the notation-core-go's revocation package.
This PR addresses part of the following issue: notaryproject/notation-core-go#124. It is dependent on notaryproject/notation-core-go#134

Signed-off-by: Kody Kimberl <[email protected]>
Signed-off-by: Kody Kimberl <[email protected]>
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.

8 participants