Enable additional identity providers for machine accounts (GitHub Actions enablement) #5385
Conversation
There was a problem hiding this comment.
Sorry about the size of the diff! I tried not to make it too large, but also not to leave too many things half-way (unlike #4317, which explicitly did that)
| // GetUserSubjectFromContext returns the user subject from the context, or nil | ||
| func GetUserSubjectFromContext(ctx context.Context) string { |
There was a problem hiding this comment.
I removed this because an ID token with a sub but without the iss context seems dangerous, so I'd prefer to have our code explicitly get auth.Identity objects and reason about them.
|
(Fixed lint and hopefully test failures) |
|
#5386 may help with the "unit test" leg of PRs timing out. |
| // | ||
| // Copyright 2024 Stacklok, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. |
There was a problem hiding this comment.
chore: The newly added files still use the old license header
| // | |
| // Copyright 2024 Stacklok, Inc. | |
| // | |
| // Licensed under the Apache License, Version 2.0 (the "License"); | |
| // you may not use this file except in compliance with the License. | |
| // You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, software | |
| // distributed under the License is distributed on an "AS IS" BASIS, | |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| // See the License for the specific language governing permissions and | |
| // limitations under the License. | |
| // SPDX-FileCopyrightText: Copyright 2025 The Minder Authors | |
| // SPDX-License-Identifier: Apache-2.0 |
| // limitations under the License. | ||
|
|
||
| // Package dynamic provides the logic for reading and validating JWT tokens | ||
| // using a JWKS URL from the token's |
There was a problem hiding this comment.
nit: Incomplete sentence here
| if err != nil { | ||
| return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err) | ||
| } | ||
| dynamicJwt := dynamic.NewDynamicValidator(ctx, cfg.Identity.Server.Audience) | ||
| jwt := merged.Validator{Validators: []jwt.Validator{staticJwt, dynamicJwt}} |
There was a problem hiding this comment.
suggestion: Wouldn't it be safer to have static validators for each identity provider? That way we would only call out to the issuers we support.
There was a problem hiding this comment.
We have a static list of identity resolvers that map iss + sub into a "user" string for OpenFGA and the database. I can add an allow-list to the dynamic validator if you'd like -- I already added metrics to see how often new resolvers were coming in (because I thought it might be interesting).
There was a problem hiding this comment.
I think an allowlist would be useful. The identity resolvers validation happens later, and it would be nice to catch this sooner.
evankanderson
force-pushed
the
github-actions-auth
branch
from
e9e12ce to
452a379
Compare
Summary
This cleans up #4317, and enables additional service account (machine identity) providers in
internal/auth, starting with GitHub Actions. It's possible that this could be generalized to AWS, GCP, etc service accounts, but I wanted to start with a concrete example which enabled a number of other stories.Usage (from #4317):
make run-dockerand set up KeyCloak with GitHub authenticationminder auth loginas a human to create a projectngrokfor port-forwarding, and update minder config with forwarding values for GitHub webhooks (optional)minder project role grant -s githubactions/repo:evankanderson/actions-id-token-testing:ref:refs/heads/main -r adminor the equivalent with your own repo.minder ruletype apply ...: https://github.com/evankanderson/actions-id-token-testing/actions/runs/13045189954/job/36394499683Fixes #5145
Change Type
Mark the type of change your PR introduces:
Testing
Added unit tests for new modules, and tested manually.
Review Checklist: