Customisable claim name for groups

[danielloader]

Checklist

• [x] I've searched the issue queue to verify this is not a duplicate feature request.
• [x] I've pasted the output of kargo version, if applicable.
• [x] I've pasted logs, if applicable.

Proposed Feature

Add a configurable custom claim name for looking at a nominated claim for roles.

Motivation

I have a custom claim name in argocd for mapping groups, rather than the default "groups" claim in the token.

Suggested Implementation

Helm chart configuration for overriding the default claim looked at for group membership roles.

[krancour]

Rather than just make the name of the groups claim overridable, I'd like to see us just add support for any arbitrary claim names the operator wishes.

[danielloader]

Sounds good to me.

[BenHesketh21]

I've had a look at this and the helm chart changes are simple enough, but I'm unsure on the best way make the values in api/rbac/v1alpha1/annotations.go configurable by the operator, would we pull in config packages and use environment variables to set the claim names?

[krancour]

I'm unsure on the best way make the values in api/rbac/v1alpha1/annotations.go configurable by the operator

I think I would count the existing annotations for sub, email, and group claims as "special" on account of the fact that Kargo does specific things with each of those.

All other claims, will have to be handled more generically.

When parsing an identity token, all claims other than those three will have to be popped into a map of "additional claims" or something.

The auth filter that finds all ServiceAccounts a user is entitled to use would have to be updated to look at all annotations with keys of the form rbac.kargo.akuity.io/claim/<name> and consult the user's additional claims map to see if the indicated claim has a value that entitles the user to that ServiceAccount.

As I think about this more deeply, it's clear not all claims can be handled generically. Even taking the original three -- sub, email, and groups -- as examples, two of those claims contain scalar values, and one contains a list. Kargo knows groups is a list.

How will the generic logic know whether it is dealing with a claim that is a scalar value or one that is a list?

This gets tricky rather fast. This isn't a trivial issue.

[krancour]

After some more thought, here's how I would do this...

In the authn filter, I would unpack all claims into a map[string]any.

The logic that determines what SAs a user is federated to gets more gnarly. We would have to list all ServiceAccounts in the cluster, filter the results to include only those in project namespaces + designated "global" namespaces. Then we'd have to step through every annotation on those SAs of the form rbac.kargo.akuity.io/claim/<name>. For each of those, retrieve a claim by name from the map[string]any, use a type assertion to figure out if the value is a string or a []string, and once that's known, evaluate whether the claim is a match for what's in the annotation.

That probably sounds worse than it is. Do keep in mind that at least the k8s client we use has a built-in cache.

One silver lining is we could eliminate special treatment for the three currently supported claims. The scheme above would handle them just fine.

I'd be willing to do this, but it's not a high priority. If someone beats me to it, great!

[BenHesketh21]

I'd like to give it a go, I may be biting off more than I can chew but you've got to start somewhere I suppose and I really want to get involved in this project. I'll see how I get on with it.

[krancour]

Corner case to look out for. I know some identity providers (AAD/Entra comes to mind) may have numbers as subject or group identifiers. So in addition to what I wrote before, we might have to check for claims of numeric types as well. Although, so far, I haven't seen it trip up the existing logic. I'm not sure what the OIDC spec says about this. Will have to dig that up a little later.

[BenHesketh21]

I'm making progress with this, slowly. Getting used to the codebase and getting my head around OIDC and the requirements. I've stumbled across something that I think I understand but I may be missing something.

So we use the oidc package to unmarshal the the token into the struct we defined, one of the values in that struct being "Groups" but when I look at the standard format of that token I don't see any examples where the groups value is used (going off this), just sub and emails along with others. Am I missing something? Or was the group claim added because of a certain provider that sets a "group" key in the token?

[krancour]

but when I look at the standard format of that token I don't see any examples where the groups value is used (going off this), just sub and emails along with others

"groups" is, indeed, not a standard claim, however, it is rather widely supported.

You may, however, run into the unpleasant situation of a user having so many groups that they cannot all fit in the token (since the token itself needs to fit in an HTTP header). Identity providers may have their own proprietary scheme for how to handle that, so a one-size-fits-all-solution is probably going to be elusive and may require some provider-specific logic. 😢 Here is how Entra handles it, for instance. It basically gives you a URL so that you can make a separate API call to get the user's (large set of) groups.

[BenHesketh21]

Perfect, I am on the right track then at least. Would we be happy if it supported just a []string for groups and potentially others for now? Since that's what is supported currently and maybe in another issue we look at doing specific stuff if/when needed?

[krancour]

and potentially others

In the authn filter, I would unpack all claims into a map[string]any.

Short of the group overage problem, that should cover all claims.

btw... it also just occurred to me that to get certain non-standard claims from a given identity provider, you may need to request additional scopes.

The scopes are currently hard-coded here:

https://github.com/akuity/kargo/blob/34d1bfe976175b85bef6b90da8214dac656ecfce/internal/api/oidc/config.go#L32

They would need to be made configurable via the Chart, but that's easy compared to the rest of this.