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

Option to allow HttpClient to follow HTTPS -> HTTP redirects #28039

Closed
martin-heralecky opened this issue Dec 1, 2018 · 19 comments
Closed

Option to allow HttpClient to follow HTTPS -> HTTP redirects #28039

martin-heralecky opened this issue Dec 1, 2018 · 19 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@martin-heralecky
Copy link

API Proposal:

class SocketsHttpHandler
{
    bool DangerousAllowHttpsToHttpRedirection { get; set; }
}

Currently HttpClient does not follow HTTPS -> HTTP redirects. However, even though this increases security, sometimes it is necessary to follow these redirects.

I suggest to add an option which would make HttpClient follow HTTPS -> HTTP redirects.

@davidsh
Copy link
Contributor

davidsh commented Dec 2, 2018

If you need to follow HTTPS -> HTTP redirects, the currently solution is to disable automatic redirection, HttpClientHandler.AllowAutoRedirect.

Then do the redirection manually by reading the Location: response header on the 3xx response.

@martin-heralecky
Copy link
Author

Yes, I know. But it'd be easier just to construct HttpClient with some optional argument (or some other way) to have it follow these redirects automatically.

@davidsh
Copy link
Contributor

davidsh commented Dec 2, 2018

You can also create a delegating handler (based on HttpMessageHandler or DelegatingHandler). In that handler you can do the manual redirection there.

See examples:
https://www.joelverhagen.com/blog/2014/11/more-control-in-httpclient-redirects/

https://stackoverflow.com/questions/11970313/delegatinghandler-for-response-in-webapi

@rmkerr
Copy link
Contributor

rmkerr commented Dec 3, 2018

Given that (1) this behavior is disabled for security reasons, and (2) there are well supported methods for working around its limitations, I don't think that we should change anything here.

I think that by leaving the process as slightly more involved, we increase the likelihood that anyone who enables insecure redirects actually understands the risk.

@davidsh
Copy link
Contributor

davidsh commented Dec 3, 2018

@karelz

@karelz
Copy link
Member

karelz commented Dec 3, 2018

Related to #23813

I agree with @rmkerr and @davidsh - IMO we should not add such API. I am fine with keeping it open to collect more feedback / upvotes.
I wonder if having standalone community-driven NuGet package with delegating handler would be a reasonable middle ground solution.

@GSPP
Copy link

GSPP commented Dec 4, 2018

What is the security risk that this mitigates? It seems that if the server (deliberately) redirects to an HTTP url then so be it.

@rmkerr
Copy link
Contributor

rmkerr commented Dec 4, 2018

My primary concern is around the potential lack of transparency when we perform an https -> http redirect. For example, when a developer writes this code:

var result = await client.GetAsync("https://contoso.com/account");

They reasonably expect to be getting the content over a secure connection, and may make assumptions about the integrity of the response. If we silently redirect from https to http those assumptions will not necessarily hold true.

The current implementation does allow servers to break that assumption, but only if the client also explicitly opts in to that behavior by manually processing redirects or creating a delegating handler. We could make it easier for clients to opt in, but as I said above:

I think that by leaving the process as slightly more involved, we increase the likelihood that anyone who enables insecure redirects actually understands the risk.

@narciero
Copy link

narciero commented Feb 14, 2019

why make everyone have to go through the trouble of manually handling redirects when a property called AllowInsecureRedirects could easily be added ? I think its pretty clear that the property enables insecure behavior - it would obviously default to false.

many websites in the wild perform 301/302 redirects from HTTPS -> HTTP (which then gets redirected back from HTTP -> HTTPS) - so it would be nice to be able to control that behavior from a client's perspective where you have no control over the server - left with the only option of manually implementing yourself which seems less than ideal since its such a simple API to add.

@karelz
Copy link
Member

karelz commented Oct 9, 2019

Triage:
We don't want to encourage insecure behavior in APIs, but given the larger impact, we are willing to take this one if our security folks agree.
We will expose it only on SocketsHttpHandler, because that is for more advanced scenarios.
Suggested name: DangerousAllowHttpsToHttpRedirection.

@terrajobst
Copy link
Member

terrajobst commented Dec 10, 2019

Video

This needs work:

  • It's a dangerous API; the name seems OK, although we normally use the Unsafe prefix.
  • We wonder whether instead of simple bool this should be callback that takes at least the URL or even the request message and return true whether it should be redirected or not.
namespace System.Net.Http
{
    public partial class SocketsHttpHandler
    {
        public bool DangerousAllowHttpsToHttpRedirection { get; set; }
    }
}

@davidsh
Copy link
Contributor

davidsh commented Dec 10, 2019

It's a dangerous API; the name seems OK, although we normally use the Unsafe prefix.

We already use the prefix 'Dangerous' here in 'HttpClientHandler'

public static System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get { throw null; } }

    public partial class HttpClientHandler : System.Net.Http.HttpMessageHandler
    {
        public static System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get { throw null; } }
        // ...
    }

@davidsh
Copy link
Contributor

davidsh commented Dec 10, 2019

We wonder whether instead of simple bool this should be callback that takes at least the URL or even the request message and return true whether it should be redirected or not.

Why make this API so complicated? Is this API really more dangerous than being able to easily bypass TLS certificate verification with the DangerousAcceptAnyServerCertificateValidator API?

As long as this API default is 'safe' (i.e. don't do redirect from HTTPS to HTTP), then I don't see why making this API difficult to use will help developers. If we make it so complicated to use, they might as well turn off auto-direct and parse the 'Location' header themselves from the 3xx response.

@danmoseley
Copy link
Member

@bartonjs do the guidelines suggest when to use Dangerous vs Unsafe? In my mind Dangerous means “difficult to use at all in a safe way” whereas Unsafe means more like “removes some safety checks” but that’s just what I’d assumed.

@bartonjs
Copy link
Member

do the guidelines suggest when to use Dangerous vs Unsafe?

Nope. I also don't recall a Dangerous v Unsafe discussion in the meeting; only that maybe a context-providing delegate was a good balance between "do a bunch of work to do manual redirects" and "anything's fair game".

According to apisof.net, we only have 4 Dangerouses: SafeHandle.DangerousAddRef, SafeHandle.DangerousRelease, SafeHandle.DangerousGetHandle, HttpClient.DangerousAcceptAnyServerCertificateValidator (then ASP.Net has a handful). But generally "Unsafe" means "pointer-type operations that may be lacking in bounds checking" and (for ThreadPool) "doesn't capture context".

I'd call this one Dangerous if it's a boolean property, but that's because I like keeping Unsafe as being related to the unsafe code keyword.

@scalablecory
Copy link
Contributor

Why make this API so complicated?

@davidsh there's a suggestion that you could do eg. handler.RedirectValidator = e => IsTrustedIntranet(e.Hostname) for when you're okay with redirects to specific trusted sources (maybe intranet) but want to guard against anything else.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@karelz karelz modified the milestones: 5.0, Future May 29, 2020
@TonyValenti
Copy link

Is there any ETA on this? We could really use this as well.

@karelz
Copy link
Member

karelz commented Jan 15, 2021

@TonyValenti no ETA.
The demand is not that strong (please upvote top post to help us keep track of that) and there are some workarounds.
Given that the last +1 took 2 years, I am tempted to close it as Won't Fix at this moment.

@karelz
Copy link
Member

karelz commented Feb 4, 2021

Closing as Won't Fix for now due to complexity, security concerns and low demand.
If you stumble upon it and need it, please upvote top post and reply here. Thanks!

@karelz karelz closed this as completed Feb 4, 2021
@karelz karelz modified the milestones: Future, 6.0.0 Feb 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests