App service support for MI #537
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -46,9 +46,10 @@ const ( | |||||
| wwwAuthenticateHeaderName = "www-authenticate" | ||||||
|
|
||||||
| // UAMI query parameter name | ||||||
| miQueryParameterClientId = "client_id" | ||||||
| miQueryParameterObjectId = "object_id" | ||||||
| miQueryParameterResourceIdIMDS = "msi_res_id" | ||||||
| miQueryParameterResourceId = "mi_res_id" | ||||||
|
|
||||||
| // IMDS | ||||||
| imdsDefaultEndpoint = "http://169.254.169.254/metadata/identity/oauth2/token" | ||||||
|
|
@@ -66,6 +67,9 @@ const ( | |||||
| himdsExecutableName = "himds.exe" | ||||||
| tokenName = "Tokens" | ||||||
|
|
||||||
| // App Service | ||||||
| appServiceAPIVersion = "2019-08-01" | ||||||
|
|
||||||
| // Environment Variables | ||||||
| identityEndpointEnvVar = "IDENTITY_ENDPOINT" | ||||||
| identityHeaderEnvVar = "IDENTITY_HEADER" | ||||||
|
|
@@ -213,6 +217,7 @@ func New(id ID, options ...ClientOption) (Client, error) { | |||||
| for _, option := range options { | ||||||
| option(&opts) | ||||||
| } | ||||||
|
|
||||||
| switch t := id.(type) { | ||||||
| case UserAssignedClientID: | ||||||
| if len(string(t)) == 0 { | ||||||
|
|
@@ -290,36 +295,49 @@ func (c Client) AcquireToken(ctx context.Context, resource string, options ...Ac | |||||
| return ar, err | ||||||
| } | ||||||
| } | ||||||
| switch c.source { | ||||||
| case AzureArc: | ||||||
| return c.acquireTokenForAzureArc(ctx, resource) | ||||||
| case DefaultToIMDS: | ||||||
| return c.acquireTokenForIMDS(ctx, resource) | ||||||
| case AppService: | ||||||
| return c.acquireTokenForAppService(ctx, resource) | ||||||
| default: | ||||||
| return base.AuthResult{}, fmt.Errorf("unsupported source %q", c.source) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func (c Client) acquireTokenForAppService(ctx context.Context, resource string) (base.AuthResult, error) { | ||||||
| req, err := createAppServiceAuthRequest(ctx, c.miType, resource) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
| tokenResponse, err := c.getTokenForRequest(req) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
| return authResultFromToken(c.authParams, tokenResponse) | ||||||
| } | ||||||
|
|
||||||
| func (c Client) acquireTokenForIMDS(ctx context.Context, resource string) (base.AuthResult, error) { | ||||||
| req, err := createIMDSAuthRequest(ctx, c.miType, resource) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
| tokenResponse, err := c.getTokenForRequest(req) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
| return authResultFromToken(c.authParams, tokenResponse) | ||||||
| } | ||||||
|
|
||||||
| func (c Client) acquireTokenForAzureArc(ctx context.Context, resource string) (base.AuthResult, error) { | ||||||
| req, err := createAzureArcAuthRequest(ctx, resource, "") | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
|
|
||||||
| response, err := c.httpClient.Do(req) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
|
|
@@ -329,7 +347,7 @@ func acquireTokenForAzureArc(ctx context.Context, client Client, resource string | |||||
| return base.AuthResult{}, fmt.Errorf("expected a 401 response, received %d", response.StatusCode) | ||||||
| } | ||||||
|
|
||||||
| secret, err := c.getAzureArcSecretKey(response, runtime.GOOS) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
|
|
@@ -339,11 +357,11 @@ func acquireTokenForAzureArc(ctx context.Context, client Client, resource string | |||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
|
|
||||||
| tokenResponse, err := c.getTokenForRequest(secondRequest) | ||||||
| if err != nil { | ||||||
| return base.AuthResult{}, err | ||||||
| } | ||||||
| return authResultFromToken(c.authParams, tokenResponse) | ||||||
| } | ||||||
|
|
||||||
| func authResultFromToken(authParams authority.AuthParams, token accesstokens.TokenResponse) (base.AuthResult, error) { | ||||||
|
|
@@ -377,7 +395,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.Minute) | ||||||
| defer tryCancel() | ||||||
|
There was a problem hiding this comment. What's the problem with deferring this? |
||||||
| if resp != nil && resp.Body != nil { | ||||||
| _, _ = io.Copy(io.Discard, resp.Body) | ||||||
|
|
@@ -446,6 +464,31 @@ func (c Client) getTokenForRequest(req *http.Request) (accesstokens.TokenRespons | |||||
| return r, err | ||||||
| } | ||||||
|
|
||||||
| func createAppServiceAuthRequest(ctx context.Context, id ID, resource string) (*http.Request, error) { | ||||||
| identityEndpoint := os.Getenv(identityEndpointEnvVar) | ||||||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, identityEndpoint, nil) | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
| req.Header.Set("X-IDENTITY-HEADER", os.Getenv(identityHeaderEnvVar)) | ||||||
| q := req.URL.Query() | ||||||
| q.Set("api-version", appServiceAPIVersion) | ||||||
| q.Set("resource", resource) | ||||||
| switch t := id.(type) { | ||||||
| case UserAssignedClientID: | ||||||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. |
||||||
| q.Set(miQueryParameterClientId, string(t)) | ||||||
| case UserAssignedResourceID: | ||||||
| q.Set(miQueryParameterResourceId, string(t)) | ||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. Is this change only for App service ? There was a problem hiding this comment. 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 There was a problem hiding this comment. We were hoping to use There was a problem hiding this comment. If they support it, they should document it; we shouldn't depend on undocumented behavior. There was a problem hiding this comment. I will test this for on Azure Function with all the different user assigned ways and update this comment on the findings. There was a problem hiding this comment. I did some testing,
Outcome - There was a problem hiding this comment. 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 It'd be safer to use that and use There was a problem hiding this comment. I will update with this in mind |
||||||
| case UserAssignedObjectID: | ||||||
| q.Set(miQueryParameterObjectId, string(t)) | ||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. I don't see this in .net or other msal's There was a problem hiding this comment. Yes, but it appears I'm out of date on this point, as the docs now state the API will accept There was a problem hiding this comment. I tested with There was a problem hiding this comment. You should run live tests before merging in any case |
||||||
| case systemAssignedValue: | ||||||
| default: | ||||||
| return nil, fmt.Errorf("unsupported type %T", id) | ||||||
| } | ||||||
| req.URL.RawQuery = q.Encode() | ||||||
| return req, nil | ||||||
| } | ||||||
|
|
||||||
| func createIMDSAuthRequest(ctx context.Context, id ID, resource string) (*http.Request, error) { | ||||||
| msiEndpoint, err := url.Parse(imdsDefaultEndpoint) | ||||||
| if err != nil { | ||||||
|
|
@@ -459,7 +502,7 @@ func createIMDSAuthRequest(ctx context.Context, id ID, resource string) (*http.R | |||||
| case UserAssignedClientID: | ||||||
| msiParameters.Set(miQueryParameterClientId, string(t)) | ||||||
| case UserAssignedResourceID: | ||||||
| msiParameters.Set(miQueryParameterResourceIdIMDS, string(t)) | ||||||
| case UserAssignedObjectID: | ||||||
| msiParameters.Set(miQueryParameterObjectId, string(t)) | ||||||
| case systemAssignedValue: // not adding anything | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DurationTime exists to calculate ExpiresOn from
expires_in. That doesn't help now that you want to consider a possible value ofexpires_onas well, and you're disabling unmarshaling for this field anyway. So, why not make the type here simplytime.Time?