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

Add support to use "TryAutoDetect" to enable auto detect #526

Closed
yingxumsft opened this issue Jan 4, 2023 · 6 comments
Closed

Add support to use "TryAutoDetect" to enable auto detect #526

yingxumsft opened this issue Jan 4, 2023 · 6 comments

Comments

@yingxumsft
Copy link

yingxumsft commented Jan 4, 2023

Describe the bug
Looks like we can only use True (boolean) to enable auto detect regional endpoint, rather than using "TryAutoDetect":
ATTEMPT_REGION_DISCOVERY = True # "TryAutoDetect"

However, Azure.Identity library is using environment variable AZURE_REGIONAL_AUTHORITY_NAME ("TryAutoDetect"), which can not be set to true.
https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/identity/azure-identity/azure/identity/_internal/msal_credentials.py

Thus we cannot use auto detect with Azure.Identity library.

To Reproduce
Steps to reproduce the behavior - run following code in local machine, which trying to auto detect but it will fail as local machine is not AzureVM.

region = os.getenv("AZURE_REGIONAL_AUTHORITY_NAME")
if region is None or len(region) == 0:
print("set region to auto detect")
os.environ["AZURE_REGIONAL_AUTHORITY_NAME"] = "TryAutoDetect"

certificate_path = "your_local_cert.pfx"
certificate_credential = CertificateCredential(tenant_id='your tenant id',
client_id='your client id',
certificate_path=certificate_path,
password="your password",
send_certificate_chain=True)
token = certificate_credential.get_token('https://vault.azure.net/.default')
print(token)

Expected behavior
token should be acquired successfully.

What you see instead
CertificateCredential.get_token failed: Authentication failed: <urllib3.connection.HTTPSConnection object at 0x000001CAF0AE0D88>: Failed to establish a new connection: [Errno 11001] getaddrinfo failed

The MSAL Python version you are using
1.20.0

Additional context
Add any other context about the problem here.

@yingxumsft yingxumsft changed the title Failed to fallback to non-regional ESTS when TryAutoDetect is set but failed to discover azure region CertificateCredential in Azure.Identity module is using environment variable to enable auto detect, but it cannot pass True to ConfidentialClientApplication Jan 4, 2023
@yingxumsft yingxumsft changed the title CertificateCredential in Azure.Identity module is using environment variable to enable auto detect, but it cannot pass True to ConfidentialClientApplication CertificateCredential in Azure.Identity module is using environment variable to enable auto detect, and it cannot pass True to ConfidentialClientApplication Jan 4, 2023
@yingxumsft yingxumsft changed the title CertificateCredential in Azure.Identity module is using environment variable to enable auto detect, and it cannot pass True to ConfidentialClientApplication Add support to use "TryAutoDetect" to enable auto detect Jan 4, 2023
@rayluo
Copy link
Collaborator

rayluo commented Jan 5, 2023

os.environ["AZURE_REGIONAL_AUTHORITY_NAME"] = "TryAutoDetect"
...
certificate_credential = CertificateCredential(...)

So, you were using the Azure Identity library (which is part of the Azure SDK for Python). The Azure Identity is built on top of MSAL Python, but their calling patterns are different.

  • Opt-in
    1. Azure Identity library may be using an environment variable AZURE_REGIONAL_AUTHORITY_NAME=TryAutoDetect to opt in (looping in @xiangyan99 to confirm)
    2. But here inside MSAL Python, the region behavior is controlled by an input parameter ConfidentialClientApplication(..., azure_region=msal.ClientApplication.ATTEMPT_REGION_DISCOVERY).
    3. As long as Azure Identity maps its "TryAutoDetect" into MSAL Python's msal.ClientApplication.ATTEMPT_REGION_DISCOVERY (does it, @xiangyan99?), the downstream app developer does not have to know that MSAL's ATTEMPT_REGION_DISCOVERY is defined as a boolean True.
  • No fallback.
    Unfortunately, once opted-in, the auto-detection behavior does not imply a graceful fallback. See more details in the "note" section in MSAL Python's documentation.

@xiangyan99
Copy link

Ray is right. You can use RegionalAuthority.AUTO_DISCOVER_REGION
https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/identity/azure-identity/azure/identity/_enums.py#L15
in AZURE_REGIONAL_AUTHORITY_NAME to opt in.

@yingxumsft
Copy link
Author

yingxumsft commented Jan 5, 2023

hi @xiangyan99 , thanks for the info. I tried to set that with following code:
os.environ[EnvironmentVariables.AZURE_REGIONAL_AUTHORITY_NAME] = RegionalAuthority.AUTO_DISCOVER_REGION.value

On my local machine, the auto discover will return None region as expected, but the problem is MSAL think the configured region is 'True', thus it will use the 'True.r.login.microsoftonline.com' as the regional host in this code:
https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/dev/msal/application.py#:~:text=region_to_use%20%3D%20(,e.%20opted%20out

The reason I think is because Identiy library is passing 'True' as string to for azure_region in MSAL application, while MSAL application expect azure_region is boolean True to opt in auto discovery. For string 'True', MSAL application think it is same as any other configured region like 'westus2'.

image

@xiangyan99
Copy link

Looks like it is by design that ATTEMPT_REGION_DISCOVERY = True # "TryAutoDetect"?

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/dev/msal/application.py#L164

@rayluo ?

@rayluo
Copy link
Collaborator

rayluo commented Jan 6, 2023

Looks like it is by design that ATTEMPT_REGION_DISCOVERY = True # "TryAutoDetect"?

There is some history behind that line. MSALs accept either a string region name, or a special flag to represent auto-detect. MSAL .Net chose to use a magic string "TryAutoDetect" for the latter. MSAL Python tried that during prototyping, but eventually switched to a non-string value approach (so that all strings are considered a valid region name).

But if we are talking about the design, then, by design, none of implementation details above really matters in MSAL Python, because MSAL Python only documents its auto-detect API as "use a special keyword ClientApplication.ATTEMPT_REGION_DISCOVERY", even without disclosing what value that constant actually is. And MSAL Python may even change that constant's value in the future, and that shall not be considered as a breaking change. (Now that the fact that we are having this conversation, makes me seriously considering to change MSAL Python to use "sentinel object pattern" ClientApplication.ATTEMPT_REGION_DISCOVERY = object() in upcoming version 1.21, so that there will be zero chance for other downstream apps to run into this same issue again. Are you OK with that, @xiangyan99 ?)

@rayluo
Copy link
Collaborator

rayluo commented Feb 2, 2023

Conclusion: The issue reported here is actually for MSAL's downstream library, the Azure Identity package, and the fix is provided there.

MSAL itself does not have to change the magic value, as long as the caller will stick with MSAL's documented constant's name ConfidentialClientApplication.ATTEMPT_REGION_DISCOVERY.

@rayluo rayluo closed this as completed Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants