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

Refactor in-memory signing CAs to use a single implementation #644

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Jun 10, 2022

Summary

There is no difference between the CreateCertificate implementation for x509ca and intermediateca, so I did a few refactors to merge the two implementations:

  • Moved intermediateca to baseca package
  • Moved extensions.go and common.go out of x509ca, since x509ca is just for pkcs11, and this caused an import cycle
  • Refactored x509ca and ephemeralca to embed baseca.BaseCA, removing the duplicate CreateCertificate/Root implementations
  • Renamed x509ca to pkcs11ca
  • Moved VerifyCertChain to common.go, since it didn't need to be a function of baseca

The primary benefit of this refactor is that all CA implementations that use the base CA implementation will support embedded SCTs. googleca is a special case.

Ticket Link

Fixes

Release Note

Refactored ephemeralca and pkcs11ca to support embedded SCTs

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #644 (9b5ee7d) into main (ce8d2fb) will increase coverage by 0.87%.
The diff coverage is 85.05%.

@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   62.02%   62.90%   +0.87%     
==========================================
  Files          29       28       -1     
  Lines        1646     1596      -50     
==========================================
- Hits         1021     1004      -17     
+ Misses        552      520      -32     
+ Partials       73       72       -1     
Impacted Files Coverage Δ
pkg/ca/fileca/fileca.go 48.48% <ø> (ø)
pkg/ca/googleca/v1/googleca.go 52.25% <0.00%> (ø)
pkg/ca/baseca/baseca.go 56.09% <66.66%> (ø)
pkg/ca/common.go 85.93% <85.93%> (ø)
pkg/ca/ephemeralca/ephemeral.go 70.73% <100.00%> (+0.73%) ⬆️
pkg/ca/extensions.go 100.00% <100.00%> (ø)
pkg/ca/fileca/load.go 58.62% <100.00%> (-10.35%) ⬇️
pkg/ca/kmsca/kmsca.go 53.57% <100.00%> (ø)
pkg/identity/email/principal.go 90.90% <100.00%> (ø)
pkg/identity/github/principal.go 90.47% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce8d2fb...9b5ee7d. Read the comment docs.

@haydentherapper haydentherapper marked this pull request as ready for review June 10, 2022 19:16
@haydentherapper
Copy link
Contributor Author

@dlorenc @bobcallaway Bumping for review. Each of the commits is a refactor.

Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

lgtm, sorry for the delay

@haydentherapper haydentherapper merged commit d54330c into sigstore:main Jun 17, 2022
@haydentherapper haydentherapper deleted the ephem branch June 17, 2022 21:40
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.

None yet

3 participants