Add option to enable groups for oidc connectors #1434
Conversation
|
cc @ericchiang (as you seem to be the one working with PRs :) ) |
|
dup of #1180 |
|
@ericchiang I looked over #1180 I don't see it adding the groups claim for oidc connectors? From my reading that PR adds better support for refresh no changes in the oidc connector for groups; which seems to indicate that this PR is still needed. |
|
ping? thanks :) |
|
cc @ericchiang ^^ |
|
Currently dex clients can refresh their IDToken with dex. When using an upstream OpenID Connect provider, the groups will be populated when the user logs in. But if the client refreshes, the groups aren't updated. I don't want to add groups until we can refresh them. |
|
Why don't we want to add them? Not all claims are guaranteed to be refreshed as-is, and this specifically calls it out in the docs (not to mention its off by default). Without this it is impossible to do groups with an oidc upstream regardless of your dex configuration. |
|
@ericchiang bump ^^ |
1 similar comment
|
@ericchiang bump ^^ |
|
I'd rather have groups not refreshing than not having them at all. |
|
cc @JoelSpeed and @srenatus (2 other active maintainers) |
|
Not caught up on the state of the OIDC connector, sorry. I'll try to get up to speed... |
|
@JoelSpeed AFAIk this is not blocked on either of those. Those PRs are for implementing refresh; this PR is to add groups to the oidc connector. (Those PRs are the google connector, this is the oidc one) |
|
I'm still with the standpoint that we shouldn't be providing group information until we can properly refresh them (as mentioned #1434 (comment)), so in my view the refresh work blocks them. @srenatus Do you agree that refresh should block groups so that we don't allow people to continue to have groups they may have been removed from, seems like a major security risk to me. Also, if you have a moment, could you look at #1180 and see what you think needs doing before we merge, I don't think I have much time at the moment but may be able to squeeze a little in if that PR will unblock other stuff |
Unfortunately we've been waiting for refresh groups for >1year (longer than this PR has been open :( ) -- which is why this PR specifically adds it as an optional config which is disabled by default. This way users can decide they are willing to take the risk and/or mitigate it some other way (setting short id token lifetimes, etc.) I'd be okay to implement refresh (presumably just hit the userinfo endpoint and update from there) -- but we'd need #1180 merged to do so -- and I would need to make a new PR since this one was closed (unfortunately it was closed erroneously as a dupe of #1180 initially :/ ). |
|
I could potentially see us merging this before #1180 if we renamed the option to make it more obvious that it is insecure. Though this decision is not just up to me. My suggestion would be to call the option The will however, be creating deprecation work once the refreshing is actually merged. In the mean time, I've reopened the PR, so if @srenatus and @bonifaido agree with my suggestion, would you be ok to fix up the conflicts and do the rename? |
Definitely -- and if not I'll just update this PR with the refresh path once ready. |
|
I had a look at #1180 which looks good to me (I would be happy to see that merged as well since we started to rely on the refresh mechanism in other providers, and OIDC would be needed as well), great work! I agree with the |
|
👍 Let's get this moving! Thanks to everyone involved, and those waiting for bearing with the delays 😉 |
|
Do we have an idea of when we'd get #1180 in or what is left to get that in? If that is soon I'd rather go that approach, but seeing as its been a long time I'm not sure if/when it'll get done :/ |
|
PR updated with requested rename; hopefully we can get this merged in soon :) |
jacksontj
force-pushed
the
groups
branch
3 times, most recently
from
2408de7 to
e1723dd
Compare
There's been some discussion in dexidp#1065 regarding what to do about refreshing groups. As it stands today dex doesn't update any of the claims on refresh (groups would just be another one). The main concern with enabling it is that group claims may change more frequently. While we continue to wait on the upstream refresh flows, this adds an option to enable the group claim. This is disabled by default (so no behavioral change) but enables those that are willing to have the delay in group claim change to use oidc IDPs. Workaround to dexidp#1065
|
bump @JoelSpeed @srenatus |
1 similar comment
|
bump @JoelSpeed @srenatus |
|
Hey @jacksontj, I fixed up #1180, I think that long term it may be a better route to go down if we can get that reviewed and merged, would you be up for doing a sanity check on it? |
|
I do agree that #1180 is the correct long-term solution, but my understanding of some previous comments (e.g. #1434 (comment) ) was that we'd merge this first. I'm okay to wait-- but its already been a month, do we have any idea how much longer it'll take to get refresh in? |
Yeah I'm ok with merging this first tbh, but I think #1180 is close to landing
I'm sorry it's taken so long. I don't know what other maintainers schedules are like, I'm unfortunately waiting for them on 1180. But I will make sure to address any feedback on that PR promptly if there is anything further needed |
|
@bonifaido @jacksontj Any chance of this PR getting merged soon since #1180 is now merged? Would love to benefit from this without needing to fork dex! |
|
Would something need to be done here to see #1185 land in mainline? Curious, as it sounds like some overlap in the areas of concern. |
|
I'd like to see OIDC Groups claims independently of the google connector modifications in #1185. |
There's been some discussion in #1065 regarding what to do about
refreshing groups. As it stands today dex doesn't update any of the
claims on refresh (groups would just be another one). The main concern
with enabling it is that group claims may change more frequently. While
we continue to wait on the upstream refresh flows, this adds an option
to enable the group claim. This is disabled by default (so no behavioral
change) but enables those that are willing to have the delay in group
claim change to use oidc IDPs.
Workaround to #1065