AzureAD / microsoft-authentication-library-for-python

Microsoft Authentication Library (MSAL) for Python makes it easy to authenticate to Microsoft Entra ID. General docs are available here https://learn.microsoft.com/entra/msal/python/ Stable APIs are documented here https://msal-python.readthedocs.io. Questions can be asked on www.stackoverflow.com with tag "msal" + "python".
https://stackoverflow.com/questions/tagged/azure-ad-msal+python
Other
754 stars 191 forks source link

[Bug] Do not auto-detect region if app developer does not opt-in to region #629

Closed pamelafox closed 7 months ago

pamelafox commented 7 months ago

Describe the bug

We're using code like this on App Service:

        self.confidential_client = ConfidentialClientApplication(
            server_app_id,
            authority=self.authority,
            client_credential=server_app_secret,
            token_cache=PersistedTokenCache(persistence),
        )

To Reproduce Steps to reproduce the behavior:

Open https://github.com/Azure-Samples/azure-search-openai-demo/pull/891 in a Codespace and follow README steps to deploy.

  1. Run azd env set AZURE_USE_AUTHENTICATION true to enable the login UI and App Service authentication.
  2. Run azd env set AZURE_AUTH_TENANT_ID <YOUR-TENANT-ID> to set the tenant ID associated with authentication.
  3. Run azd up to deploy the app.

Expected behavior

I don't expect to see warnings in the logs.

What you see instead

I see this warning:

WARNING:msal.application:Region configured (None) != region detected ('eastus2')

I read through https://msal-python.readthedocs.io/en/latest/index.html#msal.ClientApplication.params.azure_region but am still not sure what we should be doing on App Service, since it only explicitly mentions VMs and Functions. For App Service, should we keep it as None and avoid the warning? Or set it to our region? Or set it to True?

The MSAL Python version you are using

1.24.1

rayluo commented 7 months ago

Thanks for reading our docs and speaking up for app developers.

An app running inside Azure Functions and Azure VM can use a special keyword ClientApplication.ATTEMPT_REGION_DISCOVERY to auto-detect region.

I believe the suggestion above still applies. Basically, as long as auto detection can find a region, then the ATTEMPT_REGION_DISCOVERY would use that region. (P.S. We did not mention App Service probably because we weren't sure at that time, but services may also pick up new behaviors without notifying us the SDK team.)

Please try that ATTEMPT_REGION_DISCOVERY and let us know how that goes.

bgavrilMS commented 7 months ago

Hi @pamelafox . Azure Region (ESTS-R) is a mechanism only available to 1st party applications (registered via the 1p app portal). Apps need to use Subject/Name issuer mechanism for certificates (sendX5C API). This is only available in Microsoft production tenants.

If your app is 1p and you are using SN/I, you are requested to use region, we recommend that you configure the region yourself. Auto-discovery is not working very well.

If, instead, you ask customers to setup their own apps, then these are 3p apps and you cannot use regional auth. Is this your case?

@rayluo - from the code snippet above, it does not look like region was set at all. MSAL Py should not run auto-discovery. Marking this as a bug.

rayluo commented 7 months ago

Azure Region (ESTS-R) is a mechanism only available to 1st party applications (registered via the 1p app portal). Apps need to use Subject/Name issuer mechanism for certificates (sendX5C API). This is only available in Microsoft production tenants.

Back then, these were the region feature criteria that was documented. The 4 criteria were believed to be with OR condition, not AND. In particular, the criteria No. 2 was Managed Identity, which can also be used by 3rd-party. @bgavrilMS , please let me know if that doc would need to be updated. You may even directly modify the doc source send out a PR as usual.

from the code snippet above, it does not look like region was set at all. MSAL Py should not run auto-discovery. Marking this as a bug.

@bgavrilMS , thanks for pointing that out. I went back to memory lane and now remember what happened in the history.

Design Motivation
1. Initial Design Even when the app developer did not opt in thus MSAL would not use a region, MSAL still always perform region auto-discovery. Under the hood, there are two discovery mechanisms: env var-based detection is fast and cheap, IMDS endpoint call could potentially hang. Catch potential configuration error or missed opportunity. And then somehow inform us and/or app developer "hey you could use this region here but you did not".
2. Updated Design Skip auto-discovery when did not opt in for region "This design allows apps to skip the potential long latency of Region Detection, while running outside of Azure VMs."
3. MSAL Python Implementation Follow the spirit of both designs, by skipping the IMDS-based auto-discovery when did not opt in for region, but still performing the env var-based detection. In hoping to get the best of both worlds.

So, this customer's code snippet did not opt in to use region, currently MSAL Python performs an env var-based auto-detection and emits that warning, but would not use region in this case. Those are all the current known behaviors.

If we now want to have MSAL Python strictly follow design No.2, let me know. I can work out a PR for this.

bgavrilMS commented 7 months ago

Will discuss offline more details. The correct behavior is:

pamelafox commented 7 months ago

I'm a bit confused as to what I should be doing for our app on App Service. I'm not positive what 1p vs 3p means in this situation, our app is a sample that many customers deploy to their tenants, and we're working on automating its auth setup in this PR: https://github.com/Azure-Samples/azure-search-openai-demo/pull/891 I did try what Ray suggested (ATTEMPT_REGION_DISCOVERY ) and it seemed to work. Should I stick with that? I do have access to the location if I should just pass in the exact location.

bgavrilMS commented 7 months ago

@pamelafox - it looks like you expect your end-users to create the applications in their own tenants, so the apps are 3p. 1p apps means application created in the 1st party application portal.

Also, it looks like you configure the back-end as a web-api, and expect to use OBO (on-behalf-of) flow. Regional Auth is not available to OBO anyway, so this is a moot point. It's better to not use "azure_region" in your case.

Aside: you are asking users to configure a secret in their app registration. While this works, we recommend folks to use certificates in their production apps. Perhaps you can add a note about this.

Aside2: @rayluo does MSAL.py automatically ignore region for OBO ?

pamelafox commented 7 months ago

Okay, if I remove azure_region, I will go back to seeing the warning, but perhaps that's an issue on your side?

Yeah, I know certificates are recommended, and I've been trying to come up with an automation flow using KeyVault to create certificates, but it's been a bit tricky. Hoping to have that worked out by Build. I'll look for a place in code or README to suggest certs. Thanks!

bgavrilMS commented 7 months ago

Okay, if I remove azure_region, I will go back to seeing the warning, but perhaps that's an issue on your side?

Yeah, I know certificates are recommended, and I've been trying to come up with an automation flow using KeyVault to create certificates, but it's been a bit tricky. Hoping to have that worked out by Build. I'll look for a place in code or README to suggest certs. Thanks!

Yeah the warning is an issue in MSAL Py. I proposed a PR for this, waiting for @rayluo to review.