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

Consume identity.Principal in CA abstraction #570

Merged
merged 1 commit into from May 12, 2022

Conversation

nsmith5
Copy link
Contributor

@nsmith5 nsmith5 commented May 11, 2022

Summary

As part of #275, we'd like to move away from the challenges.ChallengeResult data structure and use identity.Principal and identity.Issuer instead. This PR changes the ca.CertificateAuthority to consume identity.Principal and a public key instead of challenges.ChallengeResult.

Ticket Link

Related to #275

Release Note

NONE

@nsmith5 nsmith5 force-pushed the challengeresult-as-principal branch from ea2b10e to e8b998e Compare May 12, 2022 15:35
@nsmith5 nsmith5 marked this pull request as ready for review May 12, 2022 15:35
@nsmith5
Copy link
Contributor Author

nsmith5 commented May 12, 2022

@znewman01 for refactoring mentorship eyeballs 🙏🏻

pkg/challenges/challenges.go Outdated Show resolved Hide resolved
pkg/challenges/challenges.go Show resolved Hide resolved
pkg/api/grpc_server.go Show resolved Hide resolved
pkg/challenges/challenges.go Outdated Show resolved Hide resolved
@nsmith5 nsmith5 force-pushed the challengeresult-as-principal branch 2 times, most recently from a1cf313 to a1ca128 Compare May 12, 2022 16:44
@nsmith5 nsmith5 force-pushed the challengeresult-as-principal branch from a1ca128 to 7c9db98 Compare May 12, 2022 17:57
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Looks good!

pkg/ca/googleca/v1/googleca.go Outdated Show resolved Hide resolved
Modifies the CA abstraction to consume identity.Principal instead of
challenges.ChallengeResult. Also implements the indentity.Principal
methods for ChallengeResult.

Signed-off-by: Nathan Smith <nathan@chainguard.dev>
@nsmith5
Copy link
Contributor Author

nsmith5 commented May 12, 2022

(Just cleaning up that merge conflict)

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Leaving open for a bit if @znewman01 has any comments

@codecov-commenter
Copy link

Codecov Report

Merging #570 (380c64c) into main (8d307b5) will decrease coverage by 0.34%.
The diff coverage is 29.68%.

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
- Coverage   44.29%   43.95%   -0.35%     
==========================================
  Files          21       21              
  Lines        1517     1513       -4     
==========================================
- Hits          672      665       -7     
- Misses        771      777       +6     
+ Partials       74       71       -3     
Impacted Files Coverage Δ
pkg/ca/googleca/v1/googleca.go 52.25% <0.00%> (ø)
pkg/ca/x509ca/common.go 0.00% <0.00%> (-32.61%) ⬇️
pkg/challenges/challenges.go 29.71% <30.90%> (+0.93%) ⬆️
pkg/api/grpc_server.go 52.97% <50.00%> (ø)
pkg/ca/intermediateca/intermediateca.go 72.30% <50.00%> (ø)
pkg/ca/fileca/watch.go 100.00% <0.00%> (+50.00%) ⬆️

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 8d307b5...380c64c. Read the comment docs.

@znewman01
Copy link
Contributor

Leaving open for a bit if @znewman01 has any comments

LGTM, thanks!

@haydentherapper haydentherapper merged commit abfea57 into sigstore:main May 12, 2022
@nsmith5 nsmith5 deleted the challengeresult-as-principal branch May 12, 2022 20:53
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

4 participants