App service support for MI #537

4gust · 2024-11-28T14:34:12Z

Added support for App service
Added support for logging in Managed Identity

…AD/microsoft-authentication-library-for-go into 4gust/appservice

bgavrilMS · 2024-11-28T14:49:03Z

apps/logger/logger_118.go

@@ -0,0 +1,20 @@
+//go:build go1.18 && !go1.20
+
+package logger


Can you please split the logger in a separate commit? It can even go directly to main.

bgavrilMS · 2024-11-28T14:50:18Z

apps/logger/logger_118.go

+
+// Logger struct for Go versions <= 1.20.
+type Logger struct {
+	logCallback CallbackFunc


I was under the impression that Azure SDK will not be using the callback for now so there is no need for it. When they upgrade to 1.20, they will start using slog

bgavrilMS · 2024-11-28T14:54:52Z

apps/managedidentity/managedidentity.go

@@ -378,7 +424,6 @@ func (c Client) retry(maxRetries int, req *http.Request) (*http.Response, error)
 	var err error
 	for attempt := 0; attempt < maxRetries; attempt++ {
 		tryCtx, tryCancel := context.WithTimeout(req.Context(), time.Second*15)


Why 15s? I recommend 60s to use the same default as .NET

Azure SDK has it at 15 seconds, I will update it to 60

bgavrilMS · 2024-11-28T14:57:24Z

apps/managedidentity/managedidentity.go

+	q.Set("api-version", appServiceAPIVersion)
+	q.Set("resource", resource)
+	switch t := id.(type) {
+	case UserAssignedClientID:


These are general purpose params, not related to App Service. So instead of logging them here, can we instead have logging statements at the start of the MSI "AcquireToken" request, which display all config values?

And similarly, after the HTTP request is done ... log failures and in case of success log things like "got an access token", expires in etc. Don't log the actual access token.

bgavrilMS · 2024-11-28T15:01:17Z

apps/logger/logger_120.go

+	if a.logging == nil {
+		return
+	}
+	var slogLevel slog.Level


It's unpleaseant to have to do this on every logging call :(.

bgavrilMS

Approved with comments.

chlowell

Blockers are:

query parameter names (docs)
returned token expiration time is always 0
fake token responses in tests don't resemble actual responses

Expiration time is always 0 because App Service responses include only expires_on (docs) and MSAL unmarshals only expires_in, using a custom UnmarshalJSON to convert that duration to an instant. Unit tests pass despite this because their fake responses always include expires_in

apps/managedidentity/managedidentity.go

chlowell · 2024-12-02T22:13:09Z

apps/managedidentity/managedidentity.go

@@ -377,8 +394,7 @@ func (c Client) retry(maxRetries int, req *http.Request) (*http.Response, error)
 	var resp *http.Response
 	var err error
 	for attempt := 0; attempt < maxRetries; attempt++ {
-		tryCtx, tryCancel := context.WithTimeout(req.Context(), time.Second*15)
-		defer tryCancel()


What's the problem with deferring this?

chlowell · 2024-12-02T22:15:02Z

apps/managedidentity/managedidentity.go

+	case UserAssignedResourceID:
+		q.Set(miQueryParameterResourceId, string(t))
+	case UserAssignedObjectID:
+		q.Set(miQueryParameterObjectId, string(t))


Suggested change

q.Set(miQueryParameterObjectId, string(t))

q.Set("principal_id", string(t))

I don't see this in .net or other msal's
Is this changed for App service only?

Yes, but it appears I'm out of date on this point, as the docs now state the API will accept object_id as an alias for principal_id. 🤷 I'd still make this change because Azure SDK uses, and tests, principal_id, and these docs have been incorrect before (i.e. if you want to keep using object_id, make sure you test it)

I tested with object_id and it worked, would you recommend that I also test with principal_id?

You should run live tests before merging in any case

chlowell · 2024-12-02T22:15:37Z

apps/managedidentity/managedidentity.go

+	case UserAssignedClientID:
+		q.Set(miQueryParameterClientId, string(t))
+	case UserAssignedResourceID:
+		q.Set(miQueryParameterResourceId, string(t))


Suggested change

q.Set(miQueryParameterResourceId, string(t))

q.Set("mi_res_id", string(t))

Is this change only for App service ?
AzureAD/microsoft-authentication-library-for-dotnet#4911
This bug was created to change this to "msi_res_id"

Yes. That issue is about supporting ACI, which imitates IMDS and requires msi_res_id, in accordance with IMDS docs. App Service requires mi_res_id. Every platform has its own managed identity implementation, so subtle differences like this are normal

We were hoping to use msi_res_id everywhere. Seems like AppService supports it. Thoughts?

If they support it, they should document it; we shouldn't depend on undocumented behavior.

I will test this for on Azure Function with all the different user assigned ways and update this comment on the findings.

I did some testing,
I Created a App service function as assigned a UserAssigned managed Identity and a SystemAssigned managed identity.
Then I tried to access a KeyVault using the token we acquired.
Created 4 different client one for each type

SystemAssgiened() -- No Querry parameter value in the request.
UserAssignedClientID("") -- client_id client id was assigned to this parameter
UserAssignedObjectID("") -- object_id Object id was assigned to this parameter
UserAssignedResourceID("") -- msi_res_id Resource id was assigned to this parameter

Outcome -
I was able to get AccessToken (verified the token on the jwt.ms) from all the types of the managed identities, using that token I was able to get the secret that was stored in the KeyVault

I tend to agree with @chlowell on this one though. The documented parameter for App Service (and for all other sources except IMDS I believe) is mi_res_id - see https://learn.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Chttp#rest-endpoint-reference

It'd be safer to use that and use msi_res_id only for IMDS.

I will update with this in mind

apps/managedidentity/managedidentity.go

apps/managedidentity/managedidentity_test.go

Updated the resource Id parameter for every source except IMDS

apps/managedidentity/managedidentity.go

chlowell · 2025-01-02T17:00:11Z

apps/managedidentity/managedidentity_test.go

-		TokenType:   "Bearer",
+func getSuccessfulResponse(resource string, doesHaveExpireIn bool) ([]byte, error) {
+	var response SuccessfulResponse
+	if doesHaveExpireIn {


expires_on and expires_in aren't mutually exclusive. IMDS and Cloud Shell responses have both. You want to test that, so this may be a good time to migrate to platform-specific responses.

Also, as JSON, SuccessfulResponse always includes expires_in because the JSON tag doesn't specify omitempty:
https://play.golang.com/p/kK9VdA_l4Wd

chlowell · 2025-01-02T17:36:07Z

apps/internal/oauth/ops/accesstokens/tokens.go

+
+	// Try to parse ExpiresIn first, then fallback to ExpiresOn
+	if duration, err := parseDuration(aux.ExpiresIn); err != nil {
+		println("122121@@")


chlowell · 2025-01-02T22:41:16Z

apps/internal/oauth/ops/accesstokens/tokens.go

+		return num.Int64()
+	}
+
+	// Try to parse ExpiresIn first, then fallback to ExpiresOn


Shouldn't this be the other way around--prefer expires_on if the response includes it, otherwise calculate from expires_in?

chlowell · 2025-01-03T00:56:42Z

apps/internal/oauth/ops/accesstokens/tokens.go

+		ExpiresIn json.Number `json:"expires_in"`
+		ExpiresOn json.Number `json:"expires_on"`


You could simplify this method by continuing to use internalTime.DurationTime to calculate ExpiresOn from expires_in:

Suggested change

ExpiresIn json.Number `json:"expires_in"`

ExpiresOn json.Number `json:"expires_on"`

ExpiresOn json.Number `json:"expires_on,omitempty"`

ExpiresOnCalculated internalTime.DurationTime `json:"expires_in,omitempty"`

then after unmarshaling, if aux.ExpiresOn is nonzero, you can convert it to a time.Time for the TokenResponse. If that field is zero, ExpiresOnCalculated.T is the time.Time you want

sonarqubecloud · 2025-01-03T10:01:56Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

4gust added 6 commits November 25, 2024 12:17

Added app service support

f8bd970

updated comment

d210244

Update managedidentity.go

fedc34e

Added logging

509a187

Removed a potential leak

0c4f0d2

Merge branch 'andyohart/managed-identity' of https://github.com/Azure…

cc4ca5a

…AD/microsoft-authentication-library-for-go into 4gust/appservice

4gust marked this pull request as ready for review November 28, 2024 14:38

4gust requested review from bgavrilMS and rayluo as code owners November 28, 2024 14:38

4gust requested review from chlowell and bgavrilMS and removed request for rayluo and bgavrilMS November 28, 2024 14:38

bgavrilMS reviewed Nov 28, 2024

View reviewed changes

bgavrilMS approved these changes Nov 28, 2024

View reviewed changes

4gust added 2 commits November 29, 2024 11:03

Removed logger to have it own PR

d52e165

Updated request timeout to 60 seconds

8c5b978

chlowell requested changes Dec 2, 2024

View reviewed changes

4gust added 3 commits December 30, 2024 11:30

Updated for comments

5aed02f

Updated Resource id parameter

549b2b0

Updated the resource Id parameter for every source except IMDS

Removed some unused parameters

7ce51d1

chlowell reviewed Dec 31, 2024

View reviewed changes

apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved

added support for the expires_on

e6bf2b0

chlowell reviewed Jan 3, 2025

View reviewed changes

removed comments

818cdc9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App service support for MI #537

App service support for MI #537

4gust commented Nov 28, 2024

bgavrilMS Nov 28, 2024

bgavrilMS Nov 28, 2024

bgavrilMS Nov 28, 2024

4gust Nov 29, 2024

bgavrilMS Nov 28, 2024

bgavrilMS Nov 28, 2024

bgavrilMS Nov 28, 2024

bgavrilMS left a comment

chlowell left a comment

chlowell Dec 2, 2024

chlowell Dec 2, 2024

4gust Dec 30, 2024

chlowell Dec 30, 2024

4gust Dec 31, 2024

chlowell Dec 31, 2024

chlowell Dec 2, 2024

4gust Dec 30, 2024 •

edited

Loading

chlowell Dec 30, 2024

bgavrilMS Dec 30, 2024

chlowell Dec 31, 2024

4gust Dec 31, 2024

4gust Dec 31, 2024

bgavrilMS Dec 31, 2024 •

edited

Loading

4gust Dec 31, 2024

chlowell Jan 2, 2025

chlowell Jan 2, 2025

chlowell Jan 2, 2025

chlowell Jan 3, 2025

sonarqubecloud bot commented Jan 3, 2025

	q.Set(miQueryParameterObjectId, string(t))
	q.Set("principal_id", string(t))

	q.Set(miQueryParameterResourceId, string(t))
	q.Set("mi_res_id", string(t))

		ExpiresIn json.Number `json:"expires_in"`
		ExpiresOn json.Number `json:"expires_on"`

App service support for MI #537

Are you sure you want to change the base?

App service support for MI #537

Conversation

4gust commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

chlowell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4gust Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 3, 2025

Quality Gate passed

4gust Dec 30, 2024 •

edited

Loading

bgavrilMS Dec 31, 2024 •

edited

Loading