feat: allow mapping SSO context to ServiceAccount #1148
Conversation
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
👀 |
|
@devholic, overall this looks very close to what we need. I left just a few comments on how to do this with fewer permissions and fewer access reviews. |
#1102 This commit maps SSO context (user information) to ServiceAccount to authorize user to access Kubernetes API with the matching ServiceAccount's permission. Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
- Coverage 49.49% 48.81% -0.68%
==========================================
Files 115 115
Lines 8373 8536 +163
==========================================
+ Hits 4144 4167 +23
- Misses 4081 4218 +137
- Partials 148 151 +3 ☔ View full report in Codecov by Sentry. |
|
@devholic looking good. I have two changes I want to propose:
|
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
|
|
@devholic this is looking awesome. I made some more suggestions, but I think they're mostly small things -- mainly aimed at fewer/tighter loops. |
💯 agree with @devholic. What stops someone from coming along with a new/unanticipated group and thereby acquiring permissions there were not meant to have? I think if we really want that, we need to put a lot of deliberate thought into how to prevent such a scenario. I think it's right to leave it out for now. |
#1148 (review) - Reuse internal client - Rename getUserClient to reviewSubjectAccess - Use `types.NamespacedName` for ServiceAccounts in `UserInfo` Signed-off-by: Sunghoon Kang <hoon@akuity.io>
internal/api/option/auth.go
Outdated
| for ns, names := range accounts { | ||
| for name := range names { | ||
| res[ns] = append(res[ns], name) | ||
| } | ||
| } |
There was a problem hiding this comment.
How much do we really need this loop?
Can we just return the map[string]map[types.NamespacedName]struct{} instead of returning map[string][]types.NamespacedName?
I think that would be ok, because I don't think order really matters when we use this information for authz decisions later on.
wdyt?
internal/kubeclient/indexer.go
Outdated
| ServiceAccountsByGroupIndexField = "group" | ||
| ServiceAccountsBySubjectIndexField = "subject" |
There was a problem hiding this comment.
Tiniest nit: Should these be plural "groups" and "subjects" because I think both of these indices can end up being mulit-valued.
| func indexServiceAccountsByRBACGroups(obj client.Object) []string { | ||
| sa := obj.(*corev1.ServiceAccount) // nolint: forcetypeassert | ||
| rawGroups := strings.TrimSpace(sa.GetAnnotations()[kargoapi.AnnotationKeyRBACGroups]) | ||
| if rawGroups == "" { | ||
| return nil | ||
| } | ||
| groups := strings.Split(rawGroups, ",") | ||
| refinedGroups := make([]string, 0, len(groups)) | ||
| for _, g := range groups { | ||
| if group := strings.TrimSpace(g); group != "" { | ||
| refinedGroups = append(refinedGroups, group) | ||
| } | ||
| } | ||
| return refinedGroups | ||
| } |
There was a problem hiding this comment.
Had a thought about all the new indexing functions...
Should we be validating that the SA's namespace is indeed a Kargo project?
We either have to do that when building the index or after using the index.
I think it's cheaper to do it when building the index, because this function will fire only when a SA is modified and I think that should happen a lot less frequently than when we use the indices during the authn process.
There was a problem hiding this comment.
@krancour Unless the IndexerFunc allows us to return error, I think it would be better to filter after listing service accounts for fail-safety. WDYT?
|
Looking fantastic. I think there are two small things to give some consideration to, and one very tiny nit that you can choose to deal with or not. |
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
#1148 (comment) Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
#1102
This commit maps SSO context (user information) to ServiceAccount to authorize user to access Kubernetes API with the matching ServiceAccount's permission.
@jessesuen For now, this PR doesn't support glob pattern as annotation values. IMHO, it would be better to make users to configure Auth-related settings explicitly - since it will not allow unintended subjects by default. WDYT?