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.

Sign up for GitHub

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 #22650

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Correct IMDS resource ID query parameter #22650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e40846b
Correct IMDS resource ID query parameter
chlowell Mar 26, 2024
c6de43a
changelog
chlowell Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/azidentity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Breaking Changes

### Bugs Fixed
* `ManagedIdentityCredential` now specifies resource IDs correctly for Azure Container Instances

### Other Changes

Expand Down
2 changes: 1 addition & 1 deletion sdk/azidentity/assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"AssetsRepo": "Azure/azure-sdk-assets",
"AssetsRepoPrefixPath": "go",
"TagPrefix": "go/azidentity",
"Tag": "go/azidentity_03176ee180"
"Tag": "go/azidentity_4d7934c64a"
}
18 changes: 9 additions & 9 deletions sdk/azidentity/managed_identity_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ const (
identityServerThumbprint = "IDENTITY_SERVER_THUMBPRINT"
headerMetadata = "Metadata"
imdsEndpoint = "http://169.254.169.254/metadata/identity/oauth2/token"
miResID = "mi_res_id"
Copy link
Member

Choose a reason for hiding this comment

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

How come this is still needed (in what scenarios)?

I can't easily tell from the logic below where we'd use this one vs the MSI one.
Both seem to be wrapped around if id.idKind() == miResourceID

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need this for App Service. Each platform we support has a different API, so the query parameter we need to set depends on the platform. id in the line you pasted is the value of the parameter i.e. the resource ID.

msiEndpoint = "MSI_ENDPOINT"
msiResID = "msi_res_id"
msiSecret = "MSI_SECRET"
imdsAPIVersion = "2018-02-01"
azureArcAPIVersion = "2019-08-15"
qpClientID = "client_id"
serviceFabricAPIVersion = "2019-07-01-preview"

qpClientID = "client_id"
qpResID = "mi_res_id"
)

type msiType int
Expand Down Expand Up @@ -286,7 +286,7 @@ func (c *managedIdentityClient) createIMDSAuthRequest(ctx context.Context, id Ma
q.Add("resource", strings.Join(scopes, " "))
if id != nil {
if id.idKind() == miResourceID {
q.Add(qpResID, id.String())
q.Add(msiResID, id.String())
} else {
q.Add(qpClientID, id.String())
}
Expand All @@ -306,7 +306,7 @@ func (c *managedIdentityClient) createAppServiceAuthRequest(ctx context.Context,
q.Add("resource", scopes[0])
if id != nil {
if id.idKind() == miResourceID {
q.Add(qpResID, id.String())
q.Add(miResID, id.String())
} else {
q.Add(qpClientID, id.String())
}
Expand All @@ -329,7 +329,7 @@ func (c *managedIdentityClient) createAzureMLAuthRequest(ctx context.Context, id
if id.idKind() == miResourceID {
log.Write(EventAuthentication, "WARNING: Azure ML doesn't support specifying a managed identity by resource ID")
q.Set("clientid", "")
q.Set(qpResID, id.String())
q.Set(miResID, id.String())
} else {
q.Set("clientid", id.String())
}
Expand All @@ -351,7 +351,7 @@ func (c *managedIdentityClient) createServiceFabricAuthRequest(ctx context.Conte
if id != nil {
log.Write(EventAuthentication, "WARNING: Service Fabric doesn't support selecting a user-assigned identity at runtime")
if id.idKind() == miResourceID {
q.Add(qpResID, id.String())
q.Add(miResID, id.String())
} else {
q.Add(qpClientID, id.String())
}
Expand Down Expand Up @@ -411,7 +411,7 @@ func (c *managedIdentityClient) createAzureArcAuthRequest(ctx context.Context, i
if id != nil {
log.Write(EventAuthentication, "WARNING: Azure Arc doesn't support user-assigned managed identities")
if id.idKind() == miResourceID {
q.Add(qpResID, id.String())
q.Add(miResID, id.String())
} else {
q.Add(qpClientID, id.String())
}
Expand All @@ -437,7 +437,7 @@ func (c *managedIdentityClient) createCloudShellAuthRequest(ctx context.Context,
log.Write(EventAuthentication, "WARNING: Cloud Shell doesn't support user-assigned managed identities")
q := request.Raw().URL.Query()
if id.idKind() == miResourceID {
q.Add(qpResID, id.String())
q.Add(miResID, id.String())
Copy link
Member

Choose a reason for hiding this comment

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

Most places seem to keep using the mi string, instead of msi, but the PR description is implying we should convege and only use msi.

Copy link
Member Author

@chlowell chlowell Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We need both. We should use "msi_res_id" for IMDS (and implicitly, anything resembling IMDS). For other platforms such as App Service, "mi_res_id" is correct. Edit: whoops, not according to the docs. I'll investigate App Service next 🕵️

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this was interesting. The App Service doc was changed after we implemented this feature because a reader saw that other APIs use msi_res_id and concluded mi_res_id was a typo. It isn't: mi_res_id is correct and App Service ignores msi_res_id. I opened MicrosoftDocs/azure-docs#121176 to fix the doc.

} else {
q.Add(qpClientID, id.String())
}
Expand Down
8 changes: 4 additions & 4 deletions sdk/azidentity/managed_identity_credential_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,18 @@ func TestManagedIdentityCredential_AppService(t *testing.T) {
t.Fatalf(`unexpected resource "%s"`, v)
}
if id == nil {
if q.Get(qpClientID) != "" || q.Get(qpResID) != "" {
if q.Get(qpClientID) != "" || q.Get(miResID) != "" {
t.Fatal("request shouldn't include a user-assigned ID")
}
} else {
if q.Get(qpClientID) != "" && q.Get(qpResID) != "" {
if q.Get(qpClientID) != "" && q.Get(miResID) != "" {
t.Fatal("request includes two IDs")
}
var v string
if _, ok := id.(ClientID); ok {
v = q.Get(qpClientID)
} else if _, ok := id.(ResourceID); ok {
v = q.Get(qpResID)
v = q.Get(miResID)
}
if v != id.String() {
t.Fatalf(`unexpected id "%s"`, v)
Expand Down Expand Up @@ -450,7 +450,7 @@ func TestManagedIdentityCredential_ResourceID_IMDS(t *testing.T) {
if reqQueryParams["resource"][0] != liveTestScope {
t.Fatalf("Unexpected resource in resource query param")
}
if reqQueryParams[qpResID][0] != resID {
if reqQueryParams[msiResID][0] != resID {
t.Fatalf("Unexpected resource ID in resource query param")
}
}
Expand Down
1 change: 1 addition & 0 deletions sdk/azidentity/test-resources-post.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ FROM mcr.microsoft.com/oss/go/microsoft/golang:latest as builder
ENV GOARCH=amd64 GOWORK=off
COPY . /azidentity
WORKDIR /azidentity/testdata/managed-id-test
RUN go mod tidy
RUN go build -o /build/managed-id-test .
RUN GOOS=windows go build -o /build/managed-id-test.exe .

Expand Down