[Spike] Changes needed to support GitHub Actions direct authentication to Minder #4317
Conversation
| return nil, fmt.Errorf("failed to decode JWT payload: %w", err) | ||
| } | ||
|
|
||
| parsed, err := jwt.Parse(jwtPayload, jwt.WithVerify(false), jwt.WithToken(openid.New())) |
There was a problem hiding this comment.
Oop, this doesn't check audience, and should.
| } | ||
|
|
||
| // Now that we've got the issuer, we can validate the token | ||
| keySet, err := m.getKeySet(parsed.Issuer()) |
There was a problem hiding this comment.
There's a possible resource exhaustion attack here if you got a lot of OAuth issuers. We can probably narrow this down with an allow-list, e.g. from IdentityProvider API (get all the allowed issuers, drop the others early).
| return m.jwks.Get(context.Background(), jwksUrl) | ||
| } | ||
|
|
||
| func getJWKSUrlForOpenId(issuer string) (string, error) { |
There was a problem hiding this comment.
You would think this function existed in github.com/lestrrat-go/jwx, but you'd be mistaken, except for in one test.
| // TODO: wire this in to IdentityProvider interface. Alternatively, have a different version | ||
| // for authzClient.Check that is IdentityProvider aware | ||
|
|
||
| if token.Issuer() == "https://token.actions.githubusercontent.com" { | ||
| return fmt.Sprintf("githubactions/%s", strings.ReplaceAll(token.Subject(), ":", "+")) | ||
| } |
There was a problem hiding this comment.
Hahaha, this is gross. We should probably put all of this mangling / un-mangling in authzClient, rather than putting some inside and some outside (which is the current state of play).
| // Enable one or the other. | ||
| // This is temporary until we deprecate it completely in favor of email-based role assignments | ||
| if !flags.Bool(ctx, s.featureFlags, flags.UserManagement) { | ||
| if flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) || !flags.Bool(ctx, s.featureFlags, flags.UserManagement) { |
There was a problem hiding this comment.
Since machine accounts can't accept ToS or invitations, I'm extending the lifetime of this branch. Sorry-not-sorry!
| // TODO: this assumes that we store all users in the database, and that we don't | ||
| // need to namespace identify providers. We should revisit these assumptions. | ||
| // | ||
| if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, util.UserVisibleError(codes.NotFound, "User not found") | ||
| if identity.Provider.String() == "" { | ||
| if _, err := qtx.GetUserBySubject(ctx, identity.String()); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, util.UserVisibleError(codes.NotFound, "User not found") | ||
| } | ||
| return nil, status.Errorf(codes.Internal, "error getting user: %v", err) | ||
| } | ||
| return nil, status.Errorf(codes.Internal, "error getting user: %v", err) | ||
| } |
There was a problem hiding this comment.
Oh look:
TODO: ... We should revisit these assumptions.
Visited!
|
This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days. |
|
My estimate is fixing the above highlighted issues is probably ~1 week. There would be additional UI work needed as well to enable the grants to GitHub actions; This also doesn't allow for any sort of wild-card matching on the subject, which may or may not be a feature. |
|
This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days. |
Summary
This is a SPIKE. It is not ready to merge, is more for discussion about how to approach managing Minder rules using IaC.
This change enables Minder to leverage GitHub Actions OIDC identities directly as authorizable identities without creating Keycloak entities for them. I suspect this may be desirable given the differences between machine identities (ephemeral, can be created & destroyed in seconds via automation) and human identities (generally long-lived, can accept contracts, etc).
A short version of the setup:
make run-dockerand set up KeyCloak with GitHub authentication.minder auth loginas a human, which calls CreateUser and creates a project.(set up
ngrokor some other internet -> docker connectivity)Run
minder project role grant -s githubactions/repo:evankanderson/actions-id-token-testing:ref:refs/heads/main -r adminor the equivalent with your own action name.Note that this currently only supports exact-match action names (of the form repo:$SLUG:ref:$REFNAME)
Set up a workflow like the following: https://github.com/evankanderson/actions-id-token-testing/blob/d4cc940fd69e3de78d09af7d95a936c14b69eb3c/.github/workflows/minder-auth-token-test.yaml
Run the workflow (the example uses a
workflow_dispatch, but you could also use apushtomainto trigger this automatically.Your empty project will now have a
codeql_enabledrule:(Workflow run that created the rule -- see the "Apply Minder ruletypes" step)
Note that at no point was there a need to touch GitHub Secrets or otherwise expose credentials off the local machine -- only
minder project role grant.Fixes https://github.com/stacklok/minder-stories/issues/10
Change Type
Mark the type of change your PR introduces:
Testing
This was tested manually.
Review Checklist: