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

Fix System.Net.Http.HttpClientHandler for net46 build #15036

Merged
merged 14 commits into from Jan 11, 2017

Conversation

@davidsh
Copy link
Member

davidsh commented Jan 10, 2017

This PR addresses the problem seen in #11100. Here is a summary:

  1. The original .NET Framework (Desktop) code for the HttpClientHandler implementation has been ported for this net46 build. It replaces the WinHttpHandler based code that is used for the other builds of .NET Core (Windows). This means that it uses the original HttpWebRequest (HWR) code in .NET Framework System.dll which is based on managed sockets. This ensures we go back to 100% app-compat behavior against .NET Framework. This also means that existing code that uses ServicePointManager to control behavior will also work 100% app-compat. One downside of this PR is that HTTP/2 protocol support is no longer available since we are reverting back to original .NET HTTP stack for the net46 build. If HTTP/2 protocol support is required on .NET Framework (Desktop), then a developer can use the separate WinHttpHandler package to get that.

  2. The System.Net.Http.dll assembly is now marked as APTCA (Allow Partially Trusted Callers) similar to the original annotation of the .NET Framework version of the assembly. As part of this, a few security annotations were added to source code that is being compiled from the CoreFx repo. The SecAnnotate tool was used to verify the annotations and the tool now runs clean against this assembly.

  3. This PR also addresses the problem introduced where .NET Framework WebRequestHandler wasn't compatible with the OOB System.Net.Http.dll on Desktop. I manually tested packages with this change and verified sending an HTTP request. There is no longer any "Derived types must either match the security accessibility of the base type or be less accessible" exceptions.

  4. The tracing mechanism of this implementation implicitly re-activates the System.Net tracing (TraceListener based) since we are now going back to the original .NET Framework HTTP stack. However, part of the source code for the rest of the assembly compiled in from .NET Core repo also includes the new DiagnosticSource tracing as well. There didn't appear to be any negative consequences of leaving this in as it simply adds extra tracing on top of the original System.Net tracing.

  5. There are 8 new properties that were added to the HttpClientHandler class compared to the original HttpClientHandler defined in .NET Framework. These new properties are part of the NETStandardard2.0 definitions. Due to switching back to the original HTTP stack implementation in Desktop, some of these new properties can not be implemented fully since there are incompatibilities between defining them as per handler instance compared to the actual underlying ServicePointManager implementation. So, some of these properties will throw PlatformNotSupportedException for now. Since these were newly added properties, the expectation is that there will be minimal app-compat affect.

  6. Even with this PR, any project using an OOB (Out of Band) assembly such as System.Net.Http.dll might be REQUIRED to have binding redirects in their app.config/web.config. This is because there needs to be a way to reconcile onto ONE assembly version of the same name. In particular implicit and explicit references to something like the .NET Framework System.Net.Http.WebRequest.dll (which contains WebRequestHandler) is bound to the System.Net.Http.dll version 4.0.0.0 of the .NET Framework. Once an OOB System.Net.Http.dll is introduced, there is now another version of System.Net.Http.dll with a different version number. For the current master branch, the System.Net.Http.dll assembly version is 4.2.0.0. Note that the assembly version is disjoint from the NuGet package version (currently at 4.4* in the master branch). So, this requires a binding redirect in the app.config/web.config similar to:

    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="System.Net.Http" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-4.2.0.0" newVersion="4.2.0.0" />
      </dependentAssembly>
    </assemblyBinding>

Tooling such as Visual Studio will normally create this binding for you. But we have seen cases where it doesn't do it automatically. Please keep this in mind.

If you don't have any binding redirects for System.Net.Http.dll, you will potentially get errors when sending an HTTP request such as this:

System.InvalidCastException: [A]System.Net.Http.Headers.MediaTypeHeaderValue cannot be cast to [B]System.Net.Http.Headers.MediaTypeHeaderValue. Type A originates from 'System.Net.Http, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' in the context 'Default' at location 'C:\WINDOWS\Microsoft.Net\assembly\GAC_MSIL\System.Net.Http\v4.0_4.0.0.0__b03f5f7f11d50a3a\System.Net.Http.dll'. Type B originates from 'System.Net.Http, Version=4.2.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

@davidsh davidsh added this to the 2.0.0 milestone Jan 10, 2017

@davidsh davidsh self-assigned this Jan 10, 2017

@davidsh davidsh requested review from stephentoub , karelz , CIPop , ericstj and morganbr Jan 10, 2017

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 10, 2017

@stephentoub @CIPop @morganbr @ericstj @karelz PTAL

Note to reviewers: I left in the series of commits to illustrate the process I went through in porting the original .NET Framework HttpClientHandler. As you can see from the commits, some code was removed that no longer applies. I also did minimal re-formating (such as renaming fields to use underscores) to comply with CoreFx coding guidelines. I did not do any other optimizations since this is basically a port of Desktop code and we don't want to change much at this time.

@morganbr
Copy link
Contributor

morganbr left a comment

I've reviewed the security portions, but can't comment on the actual networking code.

@@ -13,6 +13,9 @@
using System.Diagnostics.Tracing;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
#if NET46

This comment has been minimized.

@morganbr

morganbr Jan 10, 2017

Contributor

There shouldn't be any reason to ifdef this. .NET Core will just ignore these.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I thought it was useful to highlight where changes are NET46 specific. Otherwise, someone might try to remove that 'using' statement thinking it is not needed.


// Security: We need an assert for a call into WindowsIdentity.GetCurrent
[SecuritySafeCritical]
[SecurityPermission(SecurityAction.Assert, Flags = SecurityPermissionFlag.ControlPrincipal)]

This comment has been minimized.

@morganbr

morganbr Jan 10, 2017

Contributor

Is this something you added or did desktop already have this? If we have to deal with permissions, that could get more complicated.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I did not add this. It was already in the .NET Framework (Desktop) code.

@CIPop
Copy link
Member

CIPop left a comment

Left a few comments / questions.


public bool CheckCertificateRevocationList
{
// New property not in .NET Framework HttpClientHandler.

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

Nit: the comment here will quickly go out of sync with reality.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I've removed this comment.

// New property not in .NET Framework HttpClientHandler.
get
{
throw new PlatformNotSupportedException();

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

We've discussed potential ServicePointManager implementations for these. Is there a tracking item for the remaining work?

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I've reviewed possible designs using existing ServicePointManager implementation. There is no perfect solution. Any implementation of these properties would end up modifying a ServicePointManager property which is basically global. That would introduce race conditions on the actual value used during an http request.

If we're comfortable with that uncertainty in the design, then we could do changes for that. and I could create a tracking issue for it. However, the current thinking is that it is better to just throw.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

For now, we're using only one issue, #11100 as we continue to iterate on this even beyond this PR. So, I'll reference that in the TODO.

This comment has been minimized.

@karelz

karelz Jan 10, 2017

Member

We discussed it briefly at standup -- the need to make final decision is already tracked in #11100 execution plan [4] -- we plan to discuss the final decision in our Thu Networking meeting.

There is no value in creating more tracking issues at this point (@davidsh also agreed with that).

SslPolicyErrors sslPolicyErrors)
{
return _serverCertificateCustomValidationCallback(
null, // TODO: How to map sender (which is really an HttpWebRequest) to an HttpRequestMessage?

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

TODO is missing issue#.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

For now, we're using only one issue, #11100 as we continue to iterate on this even beyond this PR. So, I'll reference that in the TODO.

@@ -311,6 +314,14 @@ public Task CopyToAsync(Stream stream)
return CopyToAsync(stream, null);
}

#if NET46
// Workaround for HttpWebRequest synchronous resubmit

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

Please add more information here. What is the sync resubmit issue? (link or code ref)

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

This comment was copied as-is from the original .NET Framework source code. The code is required because the underlying HttpWebRequest implementation cannot use CopyToAsync and only uses sync based CopyTo.

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

Ah, saw it now. Since you know the cause, if you have some time, please update the comment.

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I'll update that comment with more information.

@@ -706,6 +717,9 @@ private void CheckSize(int countToAdd)
}
}

#if NET46
[SecuritySafeCritical]

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

These don't exist in .Net Framework. Why are they present in the NET46 compilation? (Are these fixes or enhancements that we would like to preserve in .Net Framework?)

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

This was required to satisfy SecAnnotate. The reason it differs from original .NET Framework source code is that HttpContent on .NET Core code has diverged a little bit from the original HttpContent source code in .NET Framework.

This comment has been minimized.

@CIPop

CIPop Jan 10, 2017

Member

.NET Core code has diverged a little bit from the original HttpContent source code in .NET Framework.

Understood. My question is: do we want this in .Net Framework or should it be compiled out using #if or getting the .Core separated in a different file? When @karelz and I were discussing the plan, we wanted to ensure 100% app-compat and behavior parity first and add risky enhancements only after proper testing. Do we have CoreFX tests to cover this? What about .Net Framework using NuGet?

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

I see your point. The LimitArrayPoolWriteStream class is only defined in the .NET Core version of HttpContent and is used only in the .NET Core version of HttpClient.cs.

So, I think I will change this so that the LimitArrayPoolWriteStream implementation is not in the NET46 build (then I won't need the security annotation for that) and I'll update the HttpContent.cs file so it is has a NET46 version for those methods that is 100% same as Desktop.

@CIPop

CIPop approved these changes Jan 10, 2017

Copy link
Member

CIPop left a comment

All my concerns have been addressed by the answers (some code changes are required).

Thanks @davidsh!

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 10, 2017

@chadwackerman ... I'm hoping this solves a compatibility issue ... On Windows it works. On Mac or Linux it returns ...

The goal of this change is to unblock the OOB package for .NET 4.6. The PR should not change anything in the .NET Core package - i.e. it will not affect any Linux/Mac code nor Windows .NET Core code. Doing anything more in this PR would just increase the risk of the change and delay the final fix. Please file a separate bug for the issue you mentioned (unless it already exists).

@karelz

karelz approved these changes Jan 10, 2017

Copy link
Member

karelz left a comment

2 minor non-blocking comments (they may even prove to be wrong).

@@ -0,0 +1,5 @@
using System.Security;

This comment has been minimized.

@karelz

karelz Jan 10, 2017

Member

Should we name this file AssemblyInfo.net46.cs?

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

Yes, good idea. I will rename this file since it is NET46 specific.

@@ -276,6 +276,9 @@
<data name="net_http_invalid_proxy" xml:space="preserve">
<value>When using WindowsProxyUsePolicy.UseCustomProxy, the Proxy property must not be null.</value>
</data>
<data name="net_http_handler_nocontentlength" xml:space="preserve">
<value>The content length of the request content can't be determined. Either set TransferEncodingChunked to true, load content into buffer, or set AllowRequestContentBuffering to true.</value>
</data>

This comment has been minimized.

@karelz

karelz Jan 10, 2017

Member

Are the resources 4.6 specific? Or can we add #if NET46 in resx?

This comment has been minimized.

@davidsh

davidsh Jan 10, 2017

Member

For this added string, yes. But we don't usually have IFDEF stuff (don't think it even works) in the *.resx file. For example, unix only strings are in here as well. The cost is pretty small so we just live with all the strings in the one *.resx file.

This comment has been minimized.

@karelz

karelz Jan 10, 2017

Member

Got it, sounds good as is then.

CIPop and others added some commits Dec 17, 2016

Remove Rtc support
Rtc only works for Windows 8 Store apps.  Windows 10 UWP dropped support
for Rtc.
Reformat
Use CoreFx naming convention for fields (use underscore, etc.)
Put properties in alphabetical ordering
Update new property implementation
Fix up security annotations
Fix up security annotations
Add required annotations.  SecAnnotate tool now runs clean against
System.Net.Http.dll.
@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@dotnet-bot test Outerloop Windows_NT Debug please
@dotnet-bot test Outerloop Ubuntu14.04 Debug please

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 11, 2017

Will the binding redirects described in the PR be possible when building net46x code using the Sdk based/new-csproj system? This kind of project doesn't typically have an app.config, particularly if multitargetting.

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

Will the binding redirects described in the PR be possible when building net46x code using the Sdk based/new-csproj system? This kind of project doesn't typically have an app.config, particularly if multitargetting.

What "new csproj system" are you referring to? If you're talking about the change in .NET Core projects from using project.json files to using proj files, those projects in general are not net46 .NET Framework projects. They are .NET Core projects. This PR only changes the net46 .NET Framework version of the System.Net.Http.dll assembly. And running anything against .NET Framework (console app, ASP.NET app) still uses app.config/web.config and thus binding redirects are possible.

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@dotnet-bot Test OuterLoop CentOS7.1 Debug Build and Test

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@dotnet-bot test Outerloop Windows_NT Debug please

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@dotnet-bot test Outerloop Ubuntu14.04 Debug please

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 11, 2017

I'm referring to projects based on microsoft.net.sdk - this includes e.g. asp.net core applications running on the desktop framework. The term "core" is rather overloaded and in this case the .net core project system is not specific to the core clr or corefx.

Will ASP.NET Core, when using a TFM like net461 instead of netcoreapp1.0, need binding redirects to use the updated System.Net.Http?

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@dotnet-bot Test OuterLoop CentOS7.1 Debug Build and Test

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@gulbanana

Will ASP.NET Core, when using a TFM like net461 instead of netcoreapp1.0, need binding redirects to use the updated System.Net.Http?

That is a good question. I don't know how ASP.NET Core works on top of .NET Framework w.r.t. this topic. I would suggest asking this question in the ASP.NET repo. The use of binding redirects is not specific to System.Net.Http.dll and they must have some mechanism to support it.

cc: @danroth27

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 11, 2017

OK, thanks. I'll ask over there.

@davidsh

This comment has been minimized.

Copy link
Member

davidsh commented Jan 11, 2017

@davidsh davidsh merged commit d24dded into dotnet:master Jan 11, 2017

10 of 11 checks passed

OuterLoop CentOS7.1 Debug Build finished.
Details
Innerloop CentOS7.1 Debug Build and Test Build finished.
Details
Innerloop CentOS7.1 Release Build and Test Build finished.
Details
Innerloop OSX Debug Build and Test Build finished.
Details
Innerloop OSX Release Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release Build and Test Build finished.
Details
Innerloop Windows_NT Debug Build and Test Build finished.
Details
Innerloop Windows_NT Release Build and Test Build finished.
Details
OuterLoop Ubuntu14.04 Debug Build finished.
Details
OuterLoop Windows_NT Debug Build finished.
Details

@davidsh davidsh deleted the davidsh:net46http branch Jan 11, 2017

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 11, 2017

Great, thanks @davidsh!
Can you please put together instructions to consume the myget feed Nuget package, so that we can try it on the simplified repro and then ask community to help us validate all remaining end-to-end scenarios?

BTW: I saw changes from @terrajobst on dogfooding CoreFX yesterday. I think they might be heplful.

@baal2000

This comment has been minimized.

Copy link

baal2000 commented Jan 11, 2017

@davidsh feedback

The use of binding redirects is not specific to System.Net.Http.dll

Understand, but that's unfortunate the redirects came into the picture. Another concrete example where it hurts is VS test runner, or RE# test runner that while being processes of their own unexpectedly start failing unit tests in the projects that are forced to use the binding redirects. All these, strictly speaking, unrelated processes now require manual configuration fixes. The time spent on resolving these small issues adds up.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 11, 2017

@ericstj @weshaggard can you help us assess the impact of binding redirects?
How easy will it be for anyone to consume the fixed NuGet package? Will we really need binding redirects? Will they be really that painful as hinted above? Can we somehow address it proactively in upstack packages? Or do we need tooling updates to deal with binding redirects better?

@ericstj

This comment has been minimized.

Copy link
Member

ericstj commented Jan 11, 2017

TL;DR: bindingRedirects suck but are required for desktop.

bindingRedirects are required for any assembly mismatches on desktop. Without this you get ref-def mismatches.
bindingRedirects are required to load an assembly that is in the unification table on desktop. Without this you get "first to load wins" paired with ref-def mismatches.

This assembly must have a different version since it exposes more API than the inbox assembly and also would clash with the inbox assembly if someone installed to the GAC.

Typically places where it matters (test runners, plugin hosts) the test runner defines a convention for a sandboxed assembly to provide those bindingRedirects. I believe in the case of a VS test runner that convention is .dll.config.

An alternate approach to bindingRedirects is to hook assemblyResolve such as https://github.com/dotnet/buildtools/blob/master/src/common/AssemblyResolver.cs. This works well if the assemblyResolve implementer can know up front all possible directories where it will load assemblies from in a given domain, then it can compute the best assembly up front. Even so, this is very hard to get right. /cc @gkhanna79

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 12, 2017

From that point of view even today's NuGet package has to have binding redirects. Will we make it any worse with the new patch we are about to release?
It seems to me that the pain is the same in both cases as soon as you stop using the inbox 4.0.0.0 version.

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 12, 2017

Yeah, it certainly won't be worse. But when consuming from e.g. the project.json system it is very nonobvious how to handle this stuff - partly because the initial design of that system assumed that binding redirects were now gone forever.

For example, I suspect that dotnet/cli#3415 will be triggered, keeping it infeasible to use this package in preview2. The new msbuild based version doesn't actually generate redirects properly yet either (see for example dotnet/sdk#514), but at least workarounds are possible in that case.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 12, 2017

@gulbanana while it is relevant to the end-to-end, I would like to keep the discussion separate from the this particular 'hot' bug. We have customers on the floor, blocked and for quite a long time :( - let's address that primarily.
So let's track separately anything that is not regressed by the fix of the 'hot' issue, to stay focused on the biggest problem at hand. We can explore improvements of the end-to-end experience in parallel.
Is that ok?

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 12, 2017

As one of those customers, it's fine by me :) Just wanted to make sure people were aware that part of the pain of this issue is that the existing workaround (binding redirects) isn't always readily available, and so a fix that relies on redirects is only partial.
The port in this PR will definitely help out many people.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Jan 12, 2017

@gulbanana if you want to make sure it doesn't get lost, please file a new issue to track it. It may be closed eventually as by design or moved elsewhere, but it is just easier to make sure the feedback doesn't get lost by accident.

@robertmclaws

This comment has been minimized.

Copy link

robertmclaws commented Jan 13, 2017

I'm really glad that we got this problem fixed. I want to take a moment and point that out.

However, the original fix to the problem was binding redirects. We'll still need to have binding redirects, with the added "benefit" of the assembly version being out of sync with the package number (super annoying). And we lose HTTP/2. So, I'm glad it's fixed, but it feels like a "lost the battle, lost the war" situation, too. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment