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

[AzureAD] Add config to map Azure's concept of AppRoles to conclude if a user is allowed and/or if the user is an admin #446

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

weisdd
Copy link

@weisdd weisdd commented Jul 15, 2021

This PR is a suggested solution for #445 (opened by me).
Since it's the first time I try to do something with Oauth2 in Python, I'd be grateful if you bear with me for all potential defects my implementation might have.

So, I suggest to rely on App roles to give admin privileges to a user and to decide whether he/she is even allowed to enter JupyterHub.

It requires the following configuration to be done on the AzureAD side:

Azure Active Directory -> App registrations -> [Name] -> Manifest:

	"appRoles": [
		{
			"allowedMemberTypes": [
				"User"
			],
			"description": "JupyterHub regular users",
			"displayName": "user",
			"id": "[RANDOM-UUID]",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"value": "user"
		},
		{
			"allowedMemberTypes": [
				"User"
			],
			"description": "JupyterHub admin users",
			"displayName": "admin",
			"id": "[RANDOM-UUID]",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"value": "admin"
		}
	],

Note: UUIDs can be generated with the help of uuidgen.

The second step would be:
Enterprise applications -> [NAME] -> Users and Groups -> [assign some of the above-mentioned roles to user groups]

Then, you'll need to deploy jupyterhub (zero-to-jupyterhub) with the following values (requires to install the modified version of azuread.py):

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    Authenticator:
      admin_azure_app_roles:
        - admin
      allowed_azure_app_roles:
        - admin
        - user

After that, any user with the admin role assigned would get admin permissions and only those who have either user or admin role would be allowed to enter JupyterHub.

Additional notes:

  • I'm not aware of how the whole test setup is supposed to be built, so I just replaced this code in a live container and manually started the jupyterhub process;
  • I'm pretty sure I don't have exactly the same settings set for Black, so your tests are likely to fail (it'd be great if you can point me at some docs on how to make it all conformant to what you expect to see).
  • It's just a draft implementation (that works in my setup), so you're more than welcomed to point at any changes in naming or main logic that need to be made.

@welcome
Copy link

welcome bot commented Jul 15, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@weisdd weisdd changed the title [WIP] App roles for AzureAD App roles for AzureAD Aug 18, 2021
Signed-off-by: Igor Beliakov <[email protected]>
@weisdd
Copy link
Author

weisdd commented Aug 18, 2021

@consideRatio Could you, please, let me know who can review the PR? :) Thanks!

@@ -94,6 +111,16 @@ async def authenticate(self, handler, data=None):
# results in a decoded JWT for the user data
auth_state['user'] = decoded

roles = decoded.get("roles", [])
Copy link
Member

Choose a reason for hiding this comment

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

Just for readability.

Suggested change
roles = decoded.get("roles", [])
roles = auth_state['user'].get("roles", [])

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in 6dc41a6

@consideRatio consideRatio changed the title App roles for AzureAD [AzureAD] Add config to map Azure's concept of AppRoles to conclude if a user is allowed and/or if the user is an admin Aug 18, 2021
@consideRatio
Copy link
Member

consideRatio commented Aug 18, 2021

I updated the z2jh Helm chart config example with some notes

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    # NOTE: This must be a class that has these traitlets defined, which isn't the base Authenticator class
    AzureAdOAuthenticator:
      # NOTE: I think we should name this similar to `admin_users`, suggestion below
      #       Hmm... Not sure on the name but I think we should include "admin_users" in it
      app_roles_with_admin_users:
        - admin
      # NOTE: I think we should name this similar to `allowed_users`, suggestion below
      # NOTE: I think we should make the admin app roles be assumed to be valid users as well
      #       Hmm... Not sure on the name but I think we should include "allowed_users" in it
      app_roles_with_allowed_users:
        - user

What is an app role? Is it a list of users? Is it a property a user can have? Not sure on my naming suggestions because I don't know anything about app roles.

Comment on lines 46 to 57
admin_azure_app_roles = List(
Unicode(),
config=True,
help="App roles that should give Jupyterhub admin privileges to a user",
)

allowed_azure_app_roles = List(
Unicode(),
config=True,
help="Automatically allow users with selected app roles",
)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should make these to be Set instead of List. Practically, if anyone passes a List, for example via a YAML configuration that is read and passed to the configuration - then it will be automatically casted to a Set so thats fine.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also like to have it clarified how to list the app roles. Is it their display name rather than id? That seems a bit odd to me but that is what matches your example configuration, but perhaps it is required to be unique as well or assumed to not risk causing problems.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a link or something as a reference to learn more about app roles this is relevant as well? I'm not sure, but in general I'm thinking the help text seem a bit too short and could be extended with another sentence or two.

Copy link
Author

Choose a reason for hiding this comment

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

@consideRatio

I think we should make these to be Set instead of List. Practically, if anyone passes a List, for example via a YAML configuration that is read and passed to the configuration - then it will be automatically casted to a Set so thats fine.

In other implementations for IDPs, I saw both Set and List being used, so wasn't sure which of those was the best fit. I'll change that to Set as per your recommendation.

I'd also like to have it clarified how to list the app roles. Is it their display name rather than id? That seems a bit odd to me but that is what matches your example configuration, but perhaps it is required to be unique as well or assumed to not risk causing problems.

It's rather value that gets sent in the token claim. It's allowed to have the same displayName, but not the value. For the latter, Azure AD will throw an error like this:
Failed to update jupyterhub application. Error detail: It contains duplicate value. Please Provide unique value.
IDs are used by Azure AD internally and they're not exposed in the respective token claim.

Perhaps a link or something as a reference to learn more about app roles this is relevant as well? I'm not sure, but in general I'm thinking the help text seem a bit too short and could be extended with another sentence or two.

Sure, I'll think about what I can add here.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to Set and improved help messages in 6dc41a6

@consideRatio
Copy link
Member

@weisdd thanks for the ping, sorry for the stale feedback you have received. I think this PR makes sense and isn't breaking in any way and is also an opt-in mechanism that is safe to use without breaking something else.

I consider this PR to be quite merge-ready with my comments are addressed. Oh btw, I'd also like a consideration if a test could be added to verify some logic of this or if its too complicated or similar.

Btw @weisdd and @thomafred, you have both created PRs (this, and #448) to AzureAD - perhaps you can review each others PR as well to help with the maintenance burden?

@thomafred
Copy link
Contributor

This is quite neat, and I like how simple it is. It would certainly help our usecase.

A few questions:

  • Will this work with B2C?
  • Does the tenant require an active subscription?

@weisdd
Copy link
Author

weisdd commented Aug 19, 2021

What is an app role? Is it a list of users? Is it a property a user can have? Not sure on my naming suggestions because I don't know anything about app roles.

@consideRatio Thanks for the review :)
App roles are described here. It's very similar to client roles in keycloak. And, btw, Grafana relies on them as well (docs).

When a user authenticates via an identity provider (IDP), the IDP may add a claim (=attribute) to identity token and/or access token sent to the client. In Azure AD, identity token is used for that.
The claim contains an array with role names (not IDs). Then, it's up to the application to decide whether it wants to do something with those roles. An app is free to ignore the claim if it doesn't care.
Every client (=application) can have its own isolated set of roles. So, even if you're reusing the same names across multiple apps, it's absolutely fine as long as those apps have their own client (and, thus, credentials).
Roles can be assigned either directly to a user or to a group the user belongs to.
The beauty of this approach is the fact that you don't have to give any permissions to the client that interacts with IDP.

1. Let admins login without explicitly having an allowed app role;
2. Style change for getting a list of roles;
3. Moved to Set traitlets.

Signed-off-by: Igor Beliakov <[email protected]>
* (Optionally) Revoke stale admin privileges;
* Improve help messages.

Signed-off-by: Igor Beliakov <[email protected]>
Signed-off-by: Igor Beliakov <[email protected]>
@weisdd
Copy link
Author

weisdd commented Aug 19, 2021

I updated the z2jh Helm chart config example with some notes

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    # NOTE: This must be a class that has these traitlets defined, which isn't the base Authenticator class
    AzureAdOAuthenticator:
      # NOTE: I think we should name this similar to `admin_users`, suggestion below
      #       Hmm... Not sure on the name but I think we should include "admin_users" in it
      app_roles_with_admin_users:
        - admin
      # NOTE: I think we should name this similar to `allowed_users`, suggestion below
      # NOTE: I think we should make the admin app roles be assumed to be valid users as well
      #       Hmm... Not sure on the name but I think we should include "allowed_users" in it
      app_roles_with_allowed_users:
        - user

@consideRatio What do you think about this naming?

hub:
  config:
    JupyterHub:
      admin_access: true
      authenticator_class: azuread
    AzureAdOAuthenticator:
      admin_users_app_roles:
        - admin
      allowed_users_app_roles:
        - user
      revoke_stale_admin_privileges: true

I decided to add revoke_stale_admin_privileges (it's set to False by default) along the way (code). It should cover the following case:
JupyterHub remembers that a user is admin meaning even if userdict["admin"] is absent during login, the admin privileges are not revoked. That means that if we change user's app role in future (e.g. admin -> user), the user will still remain admin unless someone manually revokes privileges.
The flag is aimed to address that case.

        # Admin privileges will be revoked if a user is not listed in admin_users
        # and then added back if the user has one of admin_users_app_roles.
        if self.revoke_stale_admin_privileges:
            if userdict["name"].lower() not in self.admin_users:
                userdict["admin"] = False

        if self.admin_users_app_roles:
            if check_user_has_role(roles, self.admin_users_app_roles):
                userdict["admin"] = True
                return userdict

A couple of notes:

  • As I understand, JupyterHub always converts names to lowercase, that's why I used userdict["name"].lower() not in self.admin_users: an alternative would probably be:
        if self.revoke_stale_admin_privileges:
            admin_users = [admin.lower() for admin in self.admin_users]
            if userdict["name"].lower() not in admin_users:
                userdict["admin"] = False
  • I thought that making the flag optional can give users a chance to assign privileges manually if they want to.

I consider this PR to be quite merge-ready with my comments are addressed. Oh btw, I'd also like a consideration if a test could be added to verify some logic of this or if its too complicated or similar.

Maybe we can only check the logic for assigning admin privileges, though the test is likely to be over-complicated unless the code is moved to a separate function. The latter will make the code look differently from other providers. So, not sure about that.

@weisdd
Copy link
Author

weisdd commented Aug 19, 2021

This is quite neat, and I like how simple it is. It would certainly help our usecase.

A few questions:

  • Will this work with B2C?
  • Does the tenant require an active subscription?

@thomafred Unfortunately, I'm don't know Azure deep enough to answer any of those questions (I mostly used Keycloak in the past). I can only say that as long as you're using Azure AD and can create app roles and assign users/usergroups to them, it all should work just fine. :)

@manics
Copy link
Member

manics commented Aug 19, 2021

As I understand, JupyterHub always converts names to lowercase

That's the default but it's not mandatory, it's handled by an overridable function:
https://github.com/jupyterhub/jupyterhub/blob/59b25813705ef884e54661daf9c10bb48fa1fb54/jupyterhub/auth.py#L397-L407

@consideRatio
Copy link
Member

Awwww haha I was so happy that this was potentially ready for merge but now there is another feature to consider and such that may not fit under the title of this PR.

@weisdd would you mind waiting with adding the revoke admin status logic into a separate PR? I think it makes a lot of sense but I think it would be hard to help readers of a changelog or similar to manage understand the changes of the next release if it bundles into this PR for example.

@weisdd
Copy link
Author

weisdd commented Aug 20, 2021

Awwww haha I was so happy that this was potentially ready for merge but now there is another feature to consider and such that may not fit under the title of this PR.

@weisdd would you mind waiting with adding the revoke admin status logic into a separate PR? I think it makes a lot of sense but I think it would be hard to help readers of a changelog or similar to manage understand the changes of the next release if it bundles into this PR for example.

@consideRatio Ah, I had thought it would be a good idea to have it included in this PR, though you're right, better to move discussion on it to a separate PR then. I've just removed the feature :)
Please, let me know if anything else should be adjusted here :)

@weisdd
Copy link
Author

weisdd commented Aug 20, 2021

That's the default but it's not mandatory, it's handled by an overridable function:
https://github.com/jupyterhub/jupyterhub/blob/59b25813705ef884e54661daf9c10bb48fa1fb54/jupyterhub/auth.py#L397-L407

@manics thanks for the info!

Signed-off-by: Igor Beliakov <[email protected]>
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience with the review iterations @weisdd!

@weisdd
Copy link
Author

weisdd commented Sep 7, 2021

@mafloh Could you, please, take a look at the PR? :) Thanks!

@devhell
Copy link

devhell commented Sep 29, 2021

mafloh

Is it possible you meant @manics? :)

@weisdd
Copy link
Author

weisdd commented Sep 29, 2021

@devhell you're right, thanks :)

@manics could you, plz, take a look at the PR?
Thanks!

@weisdd
Copy link
Author

weisdd commented Oct 18, 2021

@consideRatio I'm leaving my current company in two weeks, might not have a proper test bed later on. Any chance to either merge the PR or to request additional changes soon? :)

@Santhin
Copy link

Santhin commented Apr 5, 2023

Any updates?

@christian-sapconet
Copy link

@manics Would it possible to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants