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.

Sign up for GitHub

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

Scope to resource #785

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Scope to resource #785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5420c59
Relax the upper bound after testing the latest one
rayluo Nov 28, 2024
f27f4b2
Supports GUID/scope and https://contoso.com//scope
rayluo Jan 20, 2025
388b86d
Pin ubuntu 22.04 so that we can still test on Py37
rayluo Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/python-package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ jobs:
LAB_OBO_PUBLIC_CLIENT_ID: ${{ secrets.LAB_OBO_PUBLIC_CLIENT_ID }}

# Derived from https://docs.github.com/en/actions/guides/building-and-testing-python#starting-with-the-python-workflow-template
runs-on: ubuntu-latest # It switched to 22.04 shortly after 2022-Nov-8
runs-on: ubuntu-22.04
strategy:
matrix:
python-version: [3.7, 3.8, 3.9, "3.10", "3.11", "3.12"]
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', '3.13']

steps:
- uses: actions/checkout@v4
Expand Down
6 changes: 5 additions & 1 deletion msal/cloudshell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,12 @@ def _scope_to_resource(scope): # This is an experimental reasonable-effort appr
if scope.startswith(a):
return a
u = urlparse(scope)
if not u.scheme and not u.netloc: # Typically the "GUID/scope" case
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this logic. It can potentially strip important scope information and be seen as a security issue.

Since this is managed identity, we only deal with app scopes and there is 1:1 mapping between app scope and resource. i.e. scope = resource + "./default"

As such this logic should take it into account - i.e. strip "./default" if it exists. It should not attempt to remove anything else.

Copy link
Member

Choose a reason for hiding this comment

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

This is the logic that MSAL.NET uses https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main//src/client/Microsoft.Identity.Client/Utils/ScopeHelper.cs#L106

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We discussed offline. The code path is for Azure CLI running inside Cloud Shell to get a token for the signed-in user. It is never about a managed machine. (That is also why it is misleading to expose Cloud Shell token acquisition as part of the managed identity.)

Currently, Cloud Shell's token endpoint accepts a resource parameter which shall not contain any scope; Cloud Shell also has special case that accepts "https://pas.windows.net/CheckMyAccess/Linux/.default" as-is. That .Net logic will not work here.

return u.path.split("/")[0]
if u.scheme:
return "{}://{}".format(u.scheme, u.netloc)
trailer = ( # https://learn.microsoft.com/en-us/entra/identity-platform/scopes-oidc#trailing-slash-and-default
"/" if u.path.startswith("//") else "")
return "{}://{}{}".format(u.scheme, u.netloc, trailer)
Comment on lines +38 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

If resource is provided to Azure CLI, Azure CLI just adds a trailing /.default to the resource:

https://github.com/Azure/azure-cli/blob/967983c749c70e5e7dec5f1f84dae9032dc1008d/src/azure-cli-core/azure/cli/core/_profile.py#L451

            scopes = resource_to_scopes(resource)

https://github.com/Azure/azure-cli/blob/464a79cb0f9c474da1f9d426b9aaf56afcffc47a/src/azure-cli-core/azure/cli/core/auth/util.py#L83

    scope = resource + '/.default'

So this scope-to-resource conversion is still not exactly the reverse of Azure CLI's process.

Copy link
Contributor

@jiasli jiasli Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is an edge case where the resource contains more than 2 trailing slashes.

Imagine resource is https://myresource.com//, CLI will convert it to scope as https://myresource.com///.default. The converted-back resource will be https://myresource.com/, losing the last slash.

Copy link
Contributor

Choose a reason for hiding this comment

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

This conversion logic will remove the path part.

As mentioned in Azure/azure-cli#18312, valid identifier URI can also be

  • https://{verifiedCustomerDomain}/{string}
  • https://{string}.{verifiedCustomerDomain}/{string}

The trailing /{string} will be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...
So this scope-to-resource conversion is still not exactly the reverse of Azure CLI's process.

Not that the current msal.cloudshell._scope_to_resource() conversion algorithm is perfect, but logically speaking, the goal is not reversing Azure CLI's process, but to match Cloud Shell's quirk.

Imagine resource is https://myresource.com// ...

Theoretically, yes, but we never saw that before. And, most importantly, the "Cloud Shell's quirk" above contains several resources with one trailing slash, but never a double (or more) trailing slash.

https://myresource.com/mypath will also be converted to https://myresource.com.

That is arguably an expected behavior. Again, we have never encountered a Cloud Shell resource containing path, except the two known outliners which have already been handled properly.

I will suggest we focus on this test case and verify its solving the "GUID/scope" use case.

Copy link
Member

Choose a reason for hiding this comment

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

There seem to be a lot of special cases here. Will there be other customers for this code path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No other customer uses this code path. It is for Cloud Shell only, and not for any other managed identity.

return scope # There is no much else we can do here


Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ install_requires =
# And we will use the cryptography (X+3).0.0 as the upper bound,
# based on their latest deprecation policy
# https://cryptography.io/en/latest/api-stability/#deprecation
cryptography>=2.5,<46
cryptography>=2.5,<47


[options.extras_require]
Expand Down
23 changes: 23 additions & 0 deletions tests/test_cloudshell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import unittest
from msal.cloudshell import _scope_to_resource

class TestScopeToResource(unittest.TestCase):

def test_expected_behaviors(self):
for scope, expected_resource in {
"https://analysis.windows.net/powerbi/api/foo":
"https://analysis.windows.net/powerbi/api", # A special case
"https://pas.windows.net/CheckMyAccess/Linux/.default":
"https://pas.windows.net/CheckMyAccess/Linux/.default", # Special case
"https://double-slash.com//scope": "https://double-slash.com/",
"https://single-slash.com/scope": "https://single-slash.com",
"guid/some/scope": "guid",
Copy link
Member

Choose a reason for hiding this comment

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

Is guid really the answer here? I think you should only strip away .\default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Semantically, since the Cloud Shell's token endpoint is expecting a resource, so we consider the resource of a potential "GUID/path/subpath" input to be only the leading part i.e. the "GUID".

Ideally, Cloud Shell will support scope directly in the future and then we will be able to remove all these workarounds.

"6dae42f8-4368-4678-94ff-3960e28e3630/.default":
# The real guid of AKS resource
# https://learn.microsoft.com/en-us/azure/aks/kubelogin-authentication#how-to-use-kubelogin-with-aks
"6dae42f8-4368-4678-94ff-3960e28e3630",
}.items():
self.assertEqual(_scope_to_resource(scope), expected_resource)

if __name__ == '__main__':
unittest.main()
Loading