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

The REGION_NAME env var should be prefixed with MSAL_ #382

Closed
crenshaw-dev opened this issue Jul 20, 2021 · 5 comments · Fixed by #394
Closed

The REGION_NAME env var should be prefixed with MSAL_ #382

crenshaw-dev opened this issue Jul 20, 2021 · 5 comments · Fixed by #394

Comments

@crenshaw-dev
Copy link

REGION_NAME 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
Copy link
Collaborator

rayluo commented Jul 23, 2021

Thanks Michael for bringing this to our attention.

@bgavrilMS , what is your thought on these topics?

  • Our code base is using REGION_NAME to detect the region in Azure Function. Do we have a link to a public doc to "justify" the use of that name?
  • Does Azure Function also have other env var to indicate the existence of Azure Function, so that we could use a combination of both to detect region without false positive?

@rayluo rayluo added this to Todo P1 (Investigate, Bugs or Questions) in MSAL Python Board Jul 23, 2021
@bgavrilMS
Copy link
Member

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 - AzureAD/microsoft-authentication-library-for-dotnet#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
Copy link
Author

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

rayluo commented Jul 23, 2021

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

  • MSAL does not own that env var REGION_NAME. So, adding an "MSAL_" prefix is not an option.
  • MSAL means to piggyback on an existing env var defined by a given platform. Therefore, in order to avoid false positive, ideally that platform should ideally also provide a way to indicate MSAL is running on that platform in the first place.
  • REGION_NAME is believed to be used by Azure Functions to store the short name of its region (TODO: we should find a link to its document here).
    Here comes another env var that might be used to confirm MSAL is running on Azure Functions.
  • For what it's worth, REGION_NAME is defined in Azure App Service, but there is no another obvious choice to detect App Service.

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

bgavrilMS commented Jul 26, 2021

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

  • Today REGION_NAME is set by default by Azure Functions to the long region name, which cannot be used by MSAL / ESTS-R. I think this is a regression, because my team manually tested this a few months ago. Hence this bug.

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/

MSAL Python Board automation moved this from Todo P1 (Investigate, Bugs or Questions) to Done Aug 10, 2021
@rayluo rayluo mentioned this issue Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants