Begin implementing Issuer interface for email and github identities #1005
Conversation
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
|
It might be good to chat more about this before implementing. When this was originally proposed, we decided to not continue with this because it didn't seem to offer additional benefits beyond refactoring the code.The current approach should already allow for new issuer types too. Did you have more information on what was blocked and why it's not possible to do with the current implementation? |
|
Check out #596 (comment) for context |
Codecov Report
@@ Coverage Diff @@
## main #1005 +/- ##
==========================================
+ Coverage 53.75% 54.15% +0.40%
==========================================
Files 38 41 +3
Lines 2424 2430 +6
==========================================
+ Hits 1303 1316 +13
+ Misses 1022 1020 -2
+ Partials 99 94 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| return &emailIssuer{issuerURL: issuerURL} | ||
| } | ||
|
|
||
| func (e *emailIssuer) Authenticate(ctx context.Context, token string) (identity.Principal, error) { |
There was a problem hiding this comment.
All of these implementations of Authenticate and Match are going to look the same, so I wonder if we can either implement this with generics, or implement this on the base Principal type. If the base Principal type included a PrincipalFromIDToken function, it could then implement the Issuer interface too.
There was a problem hiding this comment.
I don't think it makes sense to include PrincipalFromIDToken in the interface since it's expected that Authenticate will always call it, and once this refactor is done we won't need to call PrincipalFromIDToken outside of a specific package. I think we can make it a private helper function once this refactor is done though, since it shouldn't need to be called outside of the package at that point!
There was a problem hiding this comment.
Let's see if there's some way to reduce common implementations, but I'm fine if it's in a later PR. Maybe just have Match be implemented on the base type? And Authenticate ends up calling a private principcalFromIDToken?
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
I've been working on setting up Fulcio internally and ran into some blockers. I found this TODO in the code:
fulcio/pkg/server/grpc_server.go
Lines 79 to 81 in 13a2378
This PR is the first step to completing it! This PR is 1/n in which I'll implement the Issuer interface for each identity type, ultimately switching over to calling the interface functions for authentication. A lot of this work was already done by @nsmith5, I'm just finishing it up.
The main benefit of this work is that it'll allow users to specify custom Issuers and Issuer Types if needed, making the upstream code more extensible.