Skip to content

Conversation

@samvaity
Copy link
Member

Update redirect policy to clear auth header when creating the redirect request.

@ghost ghost added the Azure.Core azure-core label Mar 23, 2022
@samvaity samvaity requested a review from pallavit March 23, 2022 20:21

// Clear the authorization header to avoid the client to be redirected to an untrusted third party server
// causing it to leak your authorization token to.
httpResponse.getHeaders().remove("Authorization");
Copy link
Member

Choose a reason for hiding this comment

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

If we remove the "Authorization" header, how does the redirected request get authorized?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is expected behavior to remove the authorization header before we redirect for security reasons.
So that we don't forward the authorization token to an untrusted/unwanted site.

It could be the service's responsibility to set the auth tokens accordingly in the redirect request so that it is forwarded correctly I would think.

Ref: Azure/azure-sdk-for-net#22876 (comment)

@pallavit
Copy link
Contributor

One other thing that came up as part of ACR investigation was that any time a service call returned a redirect return code - we lost the headers of the original request(which is intentional) but ACR needs to intercept one of those headers. As of now I am handling this via an internal ACR policy. I am not sure if this is a common enough scenario to be in the core? I am curious what others think?

@samvaity samvaity requested a review from srnagar March 29, 2022 03:00
Copy link
Contributor

@pallavit pallavit left a comment

Choose a reason for hiding this comment

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

:shipit:

@samvaity samvaity requested a review from pallavit March 30, 2022 17:38
@samvaity samvaity merged commit 83ec004 into Azure:main Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core azure-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants