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

adding option to specify client_id for MSI #748

Merged
merged 9 commits into from
May 10, 2022
Merged

adding option to specify client_id for MSI #748

merged 9 commits into from
May 10, 2022

Conversation

aj9411
Copy link
Contributor

@aj9411 aj9411 commented May 6, 2022

My team recently ran into an error when using an MSI to get a token from an AKS cluster. The agent pool for this particular cluster had two MSIs, the one we created and another one that came from AzSecPack. Other clusters with our same application were fine, but only had the single MSI we were expecting to be there.

We spun up an ubuntu container and ran some curl commands

curl 'http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource={ourResource}' -H Metadata:true

resulted in

{ "error":"invalid_request", "error_description":"Multiple user assigned identities exist, please specify the clientId / resourceId of the identity in the token request" }

and

curl 'http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource={ourResource}&client_id={ourClientId}' -H Metadata:true

resulted in a good response.

Looking at https://docs.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Cdotnet#rest-protocol-examples I saw that one can specify the client_id as a query parameter on the URL to fetch the token, so that is what I am trying to do in this pull request.

Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. I see that there are several optional query params:
https://docs.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Cdotnet#rest-endpoint-reference

Could you please model the optional query params with Option<String>?

@aj9411
Copy link
Contributor Author

aj9411 commented May 7, 2022

Thanks, I have added the other query parameters and switched to Option<String>.

@cataggar cataggar requested review from bmc-msft and heaths May 9, 2022 14:00
@bmc-msft
Copy link
Contributor

bmc-msft commented May 9, 2022

The API spec for IMDS and the managed identity reference has the optional params object_id, client_id, and ms_res_id. application_id does not show up here.

This aligns with the managed identity documentation here: https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token?msclkid=1dd0c2a2cfa211ecac474657926cfda6

@aj9411
Copy link
Contributor Author

aj9411 commented May 9, 2022

The API spec for IMDS and the managed identity reference has the optional params object_id, client_id, and ms_res_id. application_id does not show up here.

This aligns with the managed identity documentation here: https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token?msclkid=1dd0c2a2cfa211ecac474657926cfda6

Yes, I think the link form AppService/AzureFunctions I posted on the description has principal_id as the main term over 'object_id', which is referred to as an alias. Your link to the documentation from the identity team has object_id only, so I dropped principal_id. The API on the docs also has mi_res_id instead of msi_res_id (as in the imds data plane), I suppose the 's' is dropped somewhere along the line.

@bmc-msft
Copy link
Contributor

bmc-msft commented May 9, 2022

I dug into this more, as the mi_res_id vs msi_res_id was bothering me. I think the documentation is wrong. The CLI, specs, and multiple language SDK implementations all refer to msi_res_id.

Note, I opened a work item to address the document typo here, with references to multiple SDKs and the REST API spec: https://github.com/MicrosoftDocs/azure-docs/issues/92662

Copy link
Contributor

@bmc-msft bmc-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the changes regarding msi_res_id are checked in, I'm fine with this PR.

@aj9411
Copy link
Contributor Author

aj9411 commented May 9, 2022

Thank you for the investigation, I've been convinced and replaced mi_res_id for msi_res_id. I also opened another issue for the other document I originally read. https://github.com/MicrosoftDocs/azure-docs/issues/92664

Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking much better.

@cataggar cataggar requested a review from bmc-msft May 10, 2022 17:16
@cataggar cataggar merged commit dfb2430 into Azure:main May 10, 2022
@aj9411 aj9411 deleted the anjimene/clientIdForMsi branch May 10, 2022 19:45
@cataggar cataggar added this to the azure_identity 0.3.0 milestone May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants