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

Correct IMDS resource ID query parameter #4911

Open
christothes opened this issue Aug 30, 2024 · 13 comments
Open

Correct IMDS resource ID query parameter #4911

christothes opened this issue Aug 30, 2024 · 13 comments

Comments

@christothes
Copy link

christothes commented Aug 30, 2024

Specifying an identity by resource ID doesn't work on Azure Container Instances (and maybe some other platforms) because we specify the resource ID with query parameter "mi_res_id". ACI observes only "msi_res_id", which we should prefer anyway because although IMDS observes both parameters, it documents only msi_res_id.

@rayluo
Copy link
Contributor

rayluo commented Aug 30, 2024

Interesting.

We (MSAL teams) were mostly referencing public docs from https://learn.microsoft.com, but indeed they seem inconsistent, such as this App Service doc mentions mi_res_id and this VM doc mentions msi_res_id (and IIRC we manually tested the mi_res_id before and it also worked).

Question:

  1. What is the single source of truth of this topic? That repo (https://github.com/Azure/azure-rest-api-specs) sounds promising but is it also used by all the Managed Identity services?
  2. MSALs currently support these flavors of managed identities. Is Azure Container Instance considered one of them, or a new one?

@christothes
Copy link
Author

Interesting.

We (MSAL teams) were mostly referencing public docs from https://learn.microsoft.com, but indeed they seem inconsistent, such as this App Service doc mentions mi_res_id and this VM doc mentions msi_res_id (and I thought we manually tested the mi_res_id before).

Question:

  1. What is the single source of truth of this topic? That repo (https://github.com/Azure/azure-rest-api-specs) sounds promising but is it also used by all the Managed Identity services?

Good question, and I don't know the right answer. We would need to validate with the resource owners.

  1. MSALs currently support these flavors of managed identities. Is Azure Container Instance considered one of them, or a new one?

ACI is an IMDS-like endpoint, but I'm not sure if it is an actual IDMS implementation, or similar to it like with other hosting platforms. In such cases, we typically rely on the IMDS fallback as long as they adhere to the same protocol.

@bgavrilMS
Copy link
Member

Hi @christothes - do I understand correctly that Azure SDK has the same issue today? Is this blocking Azure SDK adoption of MSAL for MSI ?

@christothes
Copy link
Author

Hi @christothes - do I understand correctly that Azure SDK has the same issue today? Is this blocking Azure SDK adoption of MSAL for MSI ?

Correct - we could fix it but as we've already migrated to MSAL in preview, it wouldn't take effect until MSAL addresses it.

@chlowell
Copy link

chlowell commented Sep 4, 2024

What is the single source of truth of this topic? That repo (https://github.com/Azure/azure-rest-api-specs) sounds promising but is it also used by all the Managed Identity services?

All our flavors of managed identity are owned by different teams. They have sometimes imitated each other but their implementations are separate. mi_res_id is correct for App Service. It happens to work for IMDS as well, however the source of truth for IMDS is its REST spec, which documents only msi_res_id. We should follow the IMDS spec if only because at least one service (ACI) apparently did the same and observes only msi_res_id.

@rayluo
Copy link
Contributor

rayluo commented Sep 4, 2024

What is the single source of truth of this topic? That repo (https://github.com/Azure/azure-rest-api-specs) sounds promising but is it also used by all the Managed Identity services?

All our flavors of managed identity are owned by different teams. They have sometimes imitated each other but their implementations are separate. mi_res_id is correct for App Service. It happens to work for IMDS as well, however the source of truth for IMDS is its REST spec, which documents only msi_res_id. We should follow the IMDS spec if only because at least one service (ACI) apparently did the same and observes only msi_res_id.

  1. Agreed that there are many flavors of Managed Identity (MI). I guess that means we the SDKs can no longer operate with an assumption that "we expect a parameter foo to work across all MIs". We need to refactor our implementation to allow each flavor having its own parameters and fall back into the IMDS as a default.
  2. The Azure Rest Api Specs repo is meant to be an archive of truth: "The REST API descriptions for all Azure services should be published in the Azure REST API specs GitHub repositories". We will use it as a reference from now on. FWIW, it seems that it does not contain all the API versions of the 5 or 6 MI flavors MSAL currently support.
  3. Back to the Azure Container Instance (ACI) topic. Do we have further docs to define the ACI environment detection and its protocol? Or shall MSAL simply serve it by going with the default code path for Azure VM?

@chlowell
Copy link

chlowell commented Sep 4, 2024

  1. Agreed that there are many flavors of Managed Identity (MI). I guess that means we the SDKs can no longer operate with an assumption that "we expect a parameter foo to work across all MIs". We need to refactor our implementation to allow each flavor having its own parameters and fall back into the IMDS as a default.

👍 it's never been safe to assume uniformity across these APIs. They're all snowflakes.

  1. The Azure Rest Api Specs repo is meant to be an archive of truth: "The REST API descriptions for all Azure services should be published in the Azure REST API specs GitHub repositories". We will use it as a reference from now on. FWIW, it seems that it does not contain all the API versions of the 5 or 6 MI flavors MSAL currently support.

So far as I know, only IMDS has a formal spec. Other endpoints are described less formally in their public docs, if at all, and those docs are sometimes wrong. The Azure SDK may be the best example of what works.

  1. Back to the Azure Container Instance (ACI) topic. Do we have further docs to define the ACI environment detection and its protocol? Or shall MSAL simply serve it by going with the default code path for Azure VM?

It's indistinguishable from IMDS at runtime. See e.g. the ACI docs. The same code can support both IMDs and ACI. I don't know any practical difference other than the resource ID parameter.

@bgavrilMS
Copy link
Member

bgavrilMS commented Sep 9, 2024

How about we test this out in MSAL Golang, which is being worked on, and then make the changes in the other MSALs too? It will help with testing. But it will take longer to fix.

CC @4gust @AndyOHart

@christothes
Copy link
Author

christothes commented Sep 25, 2024

It appears there is another scenario that we haven't accounted for on ACI. As described in this article, for Windows containers, the normal IMDS endpoint (169.254.169.254) isn't available. In that case, the IDENTITY_ENDPOINT environment variable will contain the endpoint URI and the IDENTITY_HEADER env var will contain a header value that must also be sent.

Here are some details from an issue where a customer hit this. Azure/azure-sdk-for-net#45547 (comment)

@rayluo
Copy link
Contributor

rayluo commented Sep 26, 2024

It appears there is another scenario that we haven't accounted for on ACI. As described in this article, for Windows containers, the normal IMDS endpoint (169.254.169.254) isn't available. In that case, the IDENTITY_ENDPOINT environment variable will contain the endpoint URI and the IDENTITY_HEADER env var will contain a header value that must also be sent.

Here are some details from an issue where a customer hit this. Azure/azure-sdk-for-net#45547 (comment)

Then we might have reached the limitation of the current Managed Identity v1 design. With the current heuristic algorithm, the existence of both IDENTITY_ENDPOINT and IDENTITY_HEADER will trigger the App Services code path, however App Services uses a different version of api-version which also does not mention anything about principalId. How can we differentiate App Services from ACI? Or is that even possible?

@jaysondin
Copy link

It appears there is another scenario that we haven't accounted for on ACI. As described in this article, for Windows containers, the normal IMDS endpoint (169.254.169.254) isn't available. In that case, the IDENTITY_ENDPOINT environment variable will contain the endpoint URI and the IDENTITY_HEADER env var will contain a header value that must also be sent.

Here are some details from an issue where a customer hit this. Azure/azure-sdk-for-net#45547 (comment)

Hi all, one of my customers is running into similar issues as they try to get managed identity in their ACI implementation. Do we have any updates / guidance please?

@bgavrilMS
Copy link
Member

Just an update on this - @4gust is testing with msi_res_id in MSAL Golang. So far this works well for: IMDS, Arc and App Service. Once we confirm the other sources, we will update all MSALs.

@rayluo
Copy link
Contributor

rayluo commented Jan 6, 2025

Just an update on this - @4gust is testing with msi_res_id in MSAL Golang. So far this works well for: IMDS, Arc and App Service. Once we confirm the other sources, we will update all MSALs.

IMDS uses msi_res_id. Many other IMDS-like services may follow.

App Service may support msi_res_id as their implementation detail, but App Serice's canonical param is documented as mi_res_id.

Our previous understanding is to use mi_res_id for App Service only, and use msi_res_id for everything else. If we will choose to use the msi_res_id across all managed identities including App Service, please let me know so that I will need to adjust the previous implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Committed
Development

No branches or pull requests

6 participants
@rayluo @christothes @chlowell @bgavrilMS @jaysondin and others