Groups returned in token should be prefixed

[foobarto]

Following example is GitHub specific but I believe it applies to all connectors.

Let's consider usecase where Dex is used with GitHub to provide authentication for Kubernetes 1.6+.

Currently the groups value in the token returned to the client contains list of GitHub teams for specified organization. This allows anyone with high enough permissions on github org to create a team called system:masters (or similar) which would then grant anyone in that github team cluster administrator privileges. There's a lot of groups that would grant sensitive access to a Kubernetes cluster like that.

Proposed solution would be to prefix the values returned in groups inside the token with connector name and possibly with connector specific value (ie. in case of GitHub it would be the org name).

For example:

"groups": ["github:myorg:myteam"]

vs what is returned currently:

"groups": ["myteam"]

[rithujohn191]

@foobarto Thank you for your suggestion. This would be a great value add for all the connectors we have. Here are some initial thoughts about implementing this feature:

• This feature is something we wont be able to turn on my default since we have no way of making it backward compatible.
• We can probably add a flag that the user can specify which can enable this.

[candlerb]

Note for the given use case: kubernetes now supports a flag --oidc-groups-prefix which applies a prefix to the group names from the IDP.

However I still think it would be good to qualify the group names with the IDP name. Upstreams may have overlapping group names which you might not want to handle in the same way - e.g. you might treat upstream1:admins differently to upstream2:admins.

[candlerb]

BTW, isn't it also possible that two different IDPs both return "sub": "123456" ? If so, being able to prefix the sub (as well as groups) could be worthwhile.

EDIT: ignore that. I can see that the "sub" claim consists of the original sub concatenated with the connector name, e.g. "local", "mock", "google", and then base64-encoded, so it's unique. But I'm not sure why it's base64-encoded: #1719

[puja108]

We just realized that we might run into this issue as we have an upstream on which we have a group that has very high privileges (within K8s) and other upstreams should never be able to get those. However, if they guess our high privilege group claim they could theoretically create a group in their IDP to escalate their privileges to that.

I could also imagine in multi-tenant environments where tenants are using different upstreams, the case of overlapping group names like @candlerb mentioned above being a big issue.

I think a solution would need to be per upstream as it needs to set different prefixes otherwise the problem persists (like when setting prefix in k8s itself), maybe could be included in configs, but I fear that means touching every connector? Or maybe simpler, just have a flag where upstream name gets added as prefix to all groups.

Anyway, as we are keen to get this fixed, we were planing of contributing here. Any tips/guidance on where to start @rithujohn191 @candlerb?

[candlerb]

Mangling group names could be a job for middleware (#1635).

Aside: regarding groups and group memberships in general, I have been looking at Vault recently, and I really like the way they deal with this. Vault stores entities and groups as local objects in their own database, which are linked using "aliases".

• An "entity" represents a user, and is linked via an alias to one or more authentication identities (such as an OIDC sub on a given upstream)
  • This means that you can allow the same user to authenticate using multiple OIDC identities: see this blog post
  • If someone authenticates and there is no linked entity, a new one is created dynamically
• Vault groups can be linked to aliases ("external groups"), such as OIDC groups
• Vault groups can also be linked directly to vault entities ("internal groups", i.e. locally defined membership)

This means that you get to choose how to name each group, and which upstream OIDC group(s) to link each one to, and/or to define your own ad-hoc groups (#1080).

I blogged here how this can be assembled to make a self-service SSH certificate authority.

The one part Vault is missing, though, is an OIDC front-end. That is: it can consume OIDC providers as a relying party, but it can't act as on OIDC provider. However, it seems to me that very little would need to be added to support this, since Vault can already generate JWTs via its own API. EDIT: Vault can now act as a fully-fledged OIDC provider in its own right.

Perhaps Vault could be added as a backend for Dex, at least once the plugin mechanism (#1907) is present?

[marians]

Has any progress been made on connector middlewares?

Back in March last year my colleague has provided https://github.com/dexidp/dex/pull/2051 as a suggestion how to achieve group name prefixing in the groups claim.

We have extended that solution since and we've been running it in production for some time now. Ideally we'd like to contribute it here so we don't have to run Dex off a fork.

Should we provide our solution as a new PR? CC @sagikazarmark

[nabokihms]

We think this issue can be resolved by adding a connector middleware layer. I will link the issue https://github.com/dexidp/dex/issues/1635

[jonaz]

@nabokihms @sagikazarmark any progress on the middleware to be able to prefix groups?

We are currently looking into throwing out rancher and moving to dex for k8s access. But we have multiple user directories and its a real security issue that they cannot be differentiated in the groups with a prefix.

Without this feature someone hacking directory1 will be able to create a fake group for example "admin" and it will allow priviledge escalation on directory2. If the groups coming from dex to k8s would have been prefixed "directory1:admin" and "directory2:admin" this would not have been a security issue.