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
756 stars 191 forks source link

Function _get_instance_metadata is hardcoded to Azure Public URL #578

Closed lm-sig closed 11 months ago

lm-sig commented 11 months ago

Describe the bug The function in application.py, '_get_instance_metadata', is hard coded to point to the Azure Public URL of 'login.microsoftonline.com' and this is a problem for those that use other clouds (ex. AzureUSGovernment) and access to the Public URL is not permitted or allowed. The function should work similar to others were the URL is passed in as an argument.

To Reproduce Steps to reproduce the behavior:

  1. Use any Ansible azcollection module.
  2. Set "cloud_environment" or AZURE_CLOUD_ENVIRONMENT to any cloud besides Azure Public.
  3. Set AZURE_AUTHORITY_HOST=https://login.microsoftonline.us
  4. Block DNS resolution and/or network connectivity to any login.microsoftonline.com address. (Set to 127.0.0.1 in your hosts file if you need to test)
  5. Run a playbook that calls a module in the azcollection.
  6. The azcollection module task will fail with an error of "name or service not known" like there is a DNS problem or "connection refused" if a firewall is blocking.

Expected behavior Ansible modules are able to authenticate with a non-Azure Public cloud.

What you see instead "[Errno -2] Name or service not known"

The MSAL Python version you are using 1.20.0

Additional context This function was introduced in MSAL 1.19.0. Previous 'azcollection' packages pulled in 1.18.0 and worked as expected.

If I update my copy of 1.20.0 to replace ".com" with ".us" the Ansible modules work.

rayluo commented 11 months ago

Thanks for reporting your concern.

We are trying to figure out why this is the case.

Additional context This function was introduced in MSAL 1.19.0. Previous 'azcollection' packages pulled in 1.18.0 and worked as expected.

While that internal function was indeed the result of a refactoring which was shipped in MSAL Python 1.19, the same behavior has long existed, probably since day 1. But then, your statement of "Previous 'azcollection' packages pulled in 1.18.0 and worked as expected" contradicted with our understanding here.

We do not have ansible or azcollection environments in hand. But we believe the following one-liner shall be an equivalent and simpler way to reproduce the issue using MSAL Python only. It calls MSAL's get_accounts() which will in turn trigger that metadata behavior.

python -c "import logging, msal; logging.basicConfig(level=logging.DEBUG); print(msal.__version__); msal.PublicClientApplication('client_id', authority='https://login.microsoftonline.us/common').get_accounts()"

Running it in a virtual environment with MSAL Python 1.18 or 1.19+ installed, you shall always see it printing some logs, and the last line of logs will indeed contains the public cloud login.microsoftonline .com:

DEBUG:urllib3.connectionpool:https://login.microsoftonline .com:443 "GET /common/discovery/instance?api-version=1.1&authorization_endpoint=https://login.microsoftonline.com/common/oauth2/authorize HTTP/1.1" 200 945

MSAL 1.19 introduced a new parameter to turn off this behavior. But then again, your error description contradicted with our understanding, so, we do not know whether you were actually running into a different issue.

lm-sig commented 11 months ago

I am not directly using MSAL. Ansible modules call the Azure Python SDK and the Azure Python SDK calls MSAL. I'll look into seeing if either the Ansible modules or the Azure Python SDK can be updated to pass the parameter offered in 1.19+ and report the results. It may take up to a week. Thanks for the pointer.

rayluo commented 11 months ago

Closing because it is not actionable from MSAL side. Feel free to reopen if there will be any follow-up that needs our attention.

lm-sig commented 11 months ago

Setting "instance_discovery=False" in the function call helps. Thanks for the pointer.

Resolution:

Thanks again for your time.

rayluo commented 11 months ago

We just did our job. It is you, @lm-sig , that started your quest and saw it through. The community will thank you for your contribution.