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

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

Closed
pamelafox opened this issue Nov 22, 2023 · 8 comments · Fixed by #630
Closed

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

pamelafox opened this issue Nov 22, 2023 · 8 comments · Fixed by #630

Comments

@pamelafox
Copy link
Contributor

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 Azure-Samples/azure-search-openai-demo#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
Copy link
Collaborator

rayluo commented Nov 22, 2023

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
Copy link
Member

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.

@bgavrilMS bgavrilMS added bug regression Behavior that worked in a previous release that no longer works in a newer release p2 confidential-client and removed question regression Behavior that worked in a previous release that no longer works in a newer release labels Nov 23, 2023
@rayluo
Copy link
Collaborator

rayluo commented Nov 23, 2023

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
Copy link
Member

Will discuss offline more details. The correct behavior is:

  • if region is set by app developer "AutoDiscover", then MSAL has to auto-discover. If auto-discover fails, use ESTS.
  • if region is set by app developer to specific region, e.g. "eastus1", then MSAL will use that region. In the background, MSAL will also perform an auto-discovery to check if region discovered == region configued. This is for telemetry purposes only.

@bgavrilMS bgavrilMS changed the title [Bug] Unclear how to address warning about azure_region (None != eastus2) on App Service [Bug] Do not auto-detect region if app developer does not opt-in to region Nov 24, 2023
@pamelafox
Copy link
Contributor Author

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: Azure-Samples/azure-search-openai-demo#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
Copy link
Member

bgavrilMS commented Nov 24, 2023

@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
Copy link
Contributor Author

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 bgavrilMS linked a pull request Nov 24, 2023 that will close this issue
@bgavrilMS
Copy link
Member

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.

rayluo added a commit that referenced this issue Dec 1, 2023
* #629 - skip region discory when region=None

* Tidy up

---------

Co-authored-by: Ray Luo <[email protected]>
@rayluo rayluo mentioned this issue Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants