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
788 stars 194 forks source link

The REGION_NAME env var should be prefixed with MSAL_ #382

Closed crenshaw-dev closed 3 years ago

crenshaw-dev commented 3 years ago

REGIONNAME is a fairly common phrase. In the next major version, it (and probably any other env vars) should be prefixed with `MSAL`.

Bringing it up because my team just encountered a difficult-to-diagnose bug due to env var name collision. :-)

rayluo commented 3 years ago

Thanks Michael for bringing this to our attention.

@bgavrilMS , what is your thought on these topics?

bgavrilMS commented 3 years ago

REGION_NAME isn't an MSAL specific artefact. It is supposed to be set in Azure workloads / environments to the short region name. MSAL is just trying to read that to auto-detect the region.

A similar issue has been reported on MSAL.NET repository - https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/2772 - there we decided to ignore the region name if it has spaces in it etc. This is a tactical fix.

The long term fix is going to be to follow up with Azure Functions and, as Ray mentions, to maybe detect running on Azure Functions and fixing it.

crenshaw-dev commented 3 years ago

In our case, the value of the REGION env var would have passed this validation: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/2784/files#diff-f7b01fd271675670aa28e19bf77efd89ebb8a4401e7f332c8719b4df9592418eR231

But I do think that's a good step. I also like the idea of falling back to the global authority DN if the regional authority doesn't resolve, but that would obviously be more work.

rayluo commented 3 years ago

Thanks for the input. To summarize the info so far:

Under the current circumstance, there is no obvious change we can make. Let's maintain the status quo, for now. If more future reports would suggest REGION_NAME brings more trouble, we will revisit the decision of using REGION_NAME.

bgavrilMS commented 3 years ago

Yes, this is a good summary @rayluo, however:

Also:

PS: I found this env variable that contains the region short name, but it's not meant for this.

APPLICATIONINSIGHTS_CONNECTION_STRING = InstrumentationKey=f1e2565e-240d-4f51-8c96-ae2186448849;IngestionEndpoint=https://**uksouth-1**.in.applicationinsights.azure.com/