Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get JupyterHub groups from Keycloak, support oauthenticator 16.3+ #2361

Merged

Conversation

krassowski
Copy link
Member

Reference Issues or PRs

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

To test:

  1. Set the image to use Upgrade oauthenticator to 16.3.0 nebari-docker-images#131:
default_images:
  jupyterhub: quay.io/nebari/nebari-jupyterhub:oauthenticator-16.3.0
  1. Deploy
  2. Open https://your-nebari-deployment/hub/token
  3. Open Dev Tools in your browser, switch to Network tab
  4. Press "Request new API token"
  5. Right click on the request in Network tab → copy as fetch
  6. Paste the request, change method to "GET", remove "body", change URL to /hub/api/users
  7. Re-send the request
  8. Click on the new request, preview the response
  9. It should contain groups from keycloak e.g.:
[
    {
        "kind": "user",
        "auth_state": null,
        "groups": [
            "grafana_developer",
            "query-users",
            "manage-identity-providers",
            "manage-clients",
            "manage-account",
            "manage-realm",
            "view-profile",
            "argo-admin",
            "dask_gateway_developer",
            "grafana_admin",
            "view-identity-providers",
            "jupyterhub_admin",
            "view-realm",
            "view-authorization",
            "jupyterhub_developer",
            "view-clients",
            "query-groups",
            "conda_store_developer",
            "view-events",
            "query-realms",
            "impersonation",
            "realm-admin",
            "create-client",
            "conda_store_superadmin",
            "argo-viewer",
            "argo-developer",
            "manage-events",
            "grafana_viewer",
            "manage-users",
            "dask_gateway_admin",
            "manage-account-links",
            "manage-authorization",
            "query-clients",
            "view-users",
            "conda_store_admin"
        ],
        "admin": true,
        "roles": [
            "user",
            "admin"
        ],
        "name": "USER",
        "pending": null,
        "server": "/user/USER/",
        "servers": {
            "": {
                "name": "",
                "pending": null,
                "ready": true,
                "stopped": false,
                "url": "/user/USER/",
                "user_options": {
                    "profile": "small-instance"
                },
                "progress_url": "/hub/api/users/USER/server/progress",
                "state": {
                    "pod_name": "jupyter-USER"
                }
            }
        }
    }
]

Any other comments?

@krassowski
Copy link
Member Author

The cypress failure indicates that permissions did change. It looks like we need to adjust test permissions for the test to pass:

First Test -- Check Nebari login and start JupyterLab (failed)

@krassowski krassowski force-pushed the keycoak-groups-and-oauthenticator-16.3 branch from 6a4691a to 36ff361 Compare March 27, 2024 17:03
@krassowski
Copy link
Member Author

krassowski commented Mar 27, 2024

I can reproduce the failure when using the old oauthenticator version; the intent of this PR was to make it backward compatible, so I will see if we can change something to make it such.

Edit: I actually see this with the new version too.

@krassowski
Copy link
Member Author

krassowski commented Mar 27, 2024

Previously the keycloak roles where mapped to JupyterHub groups:

role_mapping = {
"admin" = ["jupyterhub_admin", "dask_gateway_admin"]
"developer" = ["jupyterhub_developer", "dask_gateway_developer"]
"analyst" = ["jupyterhub_developer"]
}

claim_groups_key = "roles"
allowed_groups = ["jupyterhub_admin", "jupyterhub_developer"]

Of note previously extracting the group data (for some reason from roles) was working, but these groups were not persisted in JupyterHub (which is why we are moving to oauthenticator 16.3+).

The logs:

│ [I 2024-03-27 18:09:45.199 JupyterHub generic:185] Validating if user claim groups match any of ['jupyterhub_admin', 'jupyterhub_developer']                                                                                                                                                        │
│ [W 2024-03-27 18:09:45.199 JupyterHub base:843] Failed login for unknown user                                                                                                                                                                                                                       │

highlight that this is caused by the misalignment between allowed_groups and groups from keycloak. These are (from keycloak, allowed):

['/analyst', '/developer', '/users'], ['jupyterhub_admin', 'jupyterhub_developer'] # 15.x
{'/developer', '/users', '/analyst'}, {'jupyterhub_admin', 'jupyterhub_developer'} # 16.3

@krassowski
Copy link
Member Author

krassowski commented Mar 29, 2024

Looking more through the codebase, it appears that the idea of mapping from keycloak group to a service-specific group roles is prevalent in nebari.

I think that the solution is to align the JupyterHub and keycloak roles as follows:

  • change the claim_groups_key to "groups" (already done)
  • change allowed_groups in JupyterHub to ['/analyst', '/developer', '/admin'] (to be done)
  • fetch the roles from keycloak in addition to "groups"

@aktech any thoughts?

@krassowski krassowski marked this pull request as ready for review March 29, 2024 17:18
@aktech
Copy link
Member

aktech commented Apr 1, 2024

change the claim_groups_key to "groups" (already done)

I assume this will look for the key "groups" in the keycloak's userdata url to populate groups in JupyterHub? If yes, then it makes sense.

change allowed_groups in JupyterHub to ['/analyst', '/developer', '/admin'] (to be done)

Will that prevent users in any other group to login to JupyterHub (Nebari)?

Context on this: We would want people to be able to create any number of groups with any roles (pre-existing) attached to them. The arbitrary groups creation is already possible. The arbitrary roles creation is not possible yet as they are static in nebari at the moment (explicitly mapped to specific service roles), but after the permissions overhaul, we should be able to create arbitrary roles, like say "I want to create a role to have read access to conda environments in the xyz namespace", hence it's important to be able to fetch all groups/roles from keycloak, irrespective of the fact they are created at the deployment time or after deployment via keycloak's GUI.

fetch the roles from keycloak in addition to "groups"

Yes.

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one comment regarding how allowed_groups and admin_groups is set.

Comment on lines +160 to +161
username_claim = "preferred_username"
claim_groups_key = "groups"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note:
It's quite weird on oauthenticator side, that one of them is called: username_claim and other one is called: claim_groups_key instead of consistent names.

@krassowski
Copy link
Member Author

Will that prevent users in any other group to login to JupyterHub (Nebari)?

Yes. But! We don't need to use allowed_groups for allow-listing users. There is a few choices here:

  • allowed_users/admin_users - well, not very flexible
  • check_allowed()/is_admin() - we can implement arbitrary logic
  • do nothing and just rely on roles

Ultimately I think we want the last one, but at this time the roles syncing is work in progress.

I think I can already make it work by overriding check_allowed()/is_admin() in the Authenticator because the auth_state already does contain the roles from Keycloak.

Do we want to do that in this PR?

@aktech
Copy link
Member

aktech commented Apr 1, 2024

I am curious, do we even need this allowed_*? As in, is it necessary to have them? I think it's fine to allow all groups and roles from keycloak.

do nothing and just rely on roles

Will this fetch all the groups from keycloak? If yes, then this is what we need.

Ultimately I think we want the last one, but at this time the roles syncing is work in progress.
I think I can already make it work by overriding check_allowed()/is_admin() in the Authenticator because the auth_state already does contain the roles from Keycloak.

Since we're fetching all the groups as per third option, this is not required.

Do we want to do that in this PR?

Certainly not, separate PR is better infact.

@marcelovilla marcelovilla added this to the Next release milestone Apr 2, 2024
@krassowski
Copy link
Member Author

I am curious, do we even need this allowed_*? As in, is it necessary to have them? I think it's fine to allow all groups and roles from keycloak.

By default OAuthenticator will not allow any user unless they meet one of the allowed_* rules, docs:

Default behavior: nobody is allowed!

The default behavior of OAuthenticator (starting with version 16) is to block all users unless explicitly authorized via some allow configuration. If you want anyone to be able to use your hub, you must specify at least one allow configuration.

Changed in version 16: Prior to OAuthenticator 16, allow_all was implied if no other allow configuration was specified. Starting from 16, allow_all can only be enabled explicitly.

What you are proposing is essentially setting allow_all = True. We can do that. I am not quite if there are any downsides, but here is what the docs has to say about it:

This is appropriate when you use an authentication provider (e.g. an institutional single-sign-on provider), where everyone who has an account in the provider should have access to your Hub. It may also be appropriate for unadvertised short-lived hubs, e.g. dedicated hubs for workshops that will be shutdown after a day, where you may decide it is acceptable to allow anyone who finds your hub to login.

Is there a scenario when a nebari user would be allowed access to say conda-store but not to JupyterHub? I guess it does not make sense because conda-store is a resource for JupyterHub users, and any user who is trusted with conda-store would usually also trusted to access JupyterHub... unless in a high confidentiality deployment when someone wanted to get an external eye on conda-store config, but without exposing the shared file system? But we can also have fine-grained file system permissions so I guess it does not matter if configured properly?

@krassowski
Copy link
Member Author

FYI while I was referencing OAuthenticator above, this behaviour is also going to be the same in JupyterHub 5.0 in general, see jupyterhub/jupyterhub#4701.

@aktech
Copy link
Member

aktech commented Apr 4, 2024

By default OAuthenticator will not allow any user unless they meet one of the allowed_* rules, docs:

It's fine to have allowed_* as long as it can be synced with keycloak, ideally without restarting the hub. Imagine, we add a new group with x, y, z (roles) permissions and we would like to make sure it takes effect without having to restart the hub. If the user needs to logout/login, that's a reasonable scenario.

Is there a scenario when a nebari user would be allowed access to say conda-store but not to JupyterHub?

I don't think so.

I guess it does not make sense because conda-store is a resource for JupyterHub users, and any user who is trusted with conda-store would usually also trusted to access JupyterHub... unless in a high confidentiality deployment when someone wanted to get an external eye on conda-store config, but without exposing the shared file system? But we can also have fine-grained file system permissions so I guess it does not matter if configured properly?

Yes, that makes sense. Regarding fine-grained permissions here is a proposal: nebari-dev/governance#47

@aktech
Copy link
Member

aktech commented Apr 4, 2024

It's fine to have allowed_* as long as it can be synced with keycloak, ideally without restarting the hub. Imagine, we add a new group with x, y, z (roles) permissions and we would like to make sure it takes effect without having to restart the hub. If the user needs to logout/login, that's a reasonable scenario.

Also, If this is true, this PR is good to merge.

@krassowski
Copy link
Member Author

It's fine to have allowed_* as long as it can be synced with keycloak, ideally without restarting the hub. Imagine, we add a new group with x, y, z (roles) permissions and we would like to make sure it takes effect without having to restart the hub. If the user needs to logout/login, that's a reasonable scenario.

The groups are synced from keycloak, but allowed_groups are hard-coded. Is that what you meant?

@aktech
Copy link
Member

aktech commented Apr 4, 2024

The groups are synced from keycloak, but allowed_groups are hard-coded. Is that what you meant?

I meant all groups should be allowed, but I see what's happening. I understand now that we need to make sure every user who's allowed to login to Nebari (JupyterHub), needs to be in one of the allowed_groups (which we do already now), that's fine, we can have minimal permissions in one of the allowed_groups and make sure every user is there by default so that they can login.

So, no change required now, this PR looks good to me.

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into it, this looks good to me.

@aktech aktech merged commit 6a83ada into nebari-dev:develop Apr 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants