Latest Microsoft.AspNetCore.App 2.2 version is not used automatically

[omajid]

This is a regression. It's #586 but for 2.2.

In other words, consumers of source-build wont get the 2.2.1 ASP.NET Core security release!

This was supposed to have been fixed by https://github.com/dotnet/source-build/blob/release/2.2/patches/cli/0001-Persist-ASP.NET-runtime-patch-targeting-default.patch, but it turns out it's not sufficient for 2.2.

In 2.2, the final dotnet/sdk/$VERSION/Microsoft.NETCoreSdk.BundledVersions.props file contains this line:

    <!-- If true, always target the latest ASP.NET Core runtime by default -->
    <TargetLatestAspNetCoreRuntimePatch Condition="'' == ''">true</TargetLatestAspNetCoreRuntimePatch>

The settings in this file are consumed by dotnet/sdk/2.2.102/Sdks/Microsoft.NET.Sdk.Web/Sdk/Sdk.DefaultItems.targets. And this targets file is not loaded out of the box: dotnet/sdk/2.2.102/Sdks/Microsoft.NET.Sdk.Web/Sdk/Sdk.targets says:

<Import Project="$(MSBuildThisFileDirectory)Sdk.DefaultItems.targets" Condition="'$(EnableWebSdkImplicitPackageVersions)' == 'true'"/>

EnableWebSdkImplicitPackageVersions is false, as set by dotnet/websdk#414.
So our custom overrides to TargetLatestAspNetCoreRuntimePatch are ineffective.

This was an intentional change done by dotnet/websdk#414. The TargetLatestRuntimePatch flag added by dotnet/sdk#2533 lets me get the old behaviour via dotnet restore -p:TargetLatestRuntimePatch=true:

  Installing Microsoft.NETCore.App 2.2.1.                        
  Installing Microsoft.NETCore.Targets 2.0.0.                        
  Installing Microsoft.AspNetCore.App 2.2.1. 

However, this also forces Microsoft.NETCore.App to 2.2.1, which is against the goals of the roll-forward design: dotnet/designs#36

We need to make TargetLatestRuntimePatch or a Microsoft.AspNetCore.App/All-specific variant the default for source-build to get secure builds.

cc @tmds @RheaAyase

[crummel]

Does #801 need to be ported to 2.2? Otherwise if that's not an actual fix anymore we might need to do something in 2.1 as well.

[omajid]

I am not sure. It could be, but it shouldn't make a difference in this case, assuming we keep the current SDK version resolution logic.

As I mentioned in #801, the original 2.1 file looks like this without the fix:

<TargetLatestAspNetCoreRuntimePatch Condition="'true' == ''">true</TargetLatestAspNetCoreRuntimePatch>"

And TargetLatestAspNetCoreRuntimePatch is never set to true. In 2.2, the line looks like this:

<TargetLatestAspNetCoreRuntimePatch Condition="'' == ''">true</TargetLatestAspNetCoreRuntimePatch>

And TargetLatestAspNetCoreRuntimePatch is always set to true (this is what we want). It can't be overridden, but the default behaviour would have been correct if dotnet/websdk#414 hadn't changed the entire version resolution logic.

Edit: to elaborate a little more. The 2.1.xxx SDKs used the logic implemented in msbuild files to pick a version of Microsoft.AspNetCore.App/.All to use. #801 fixes a problem there. The 2.2.yyy SDKs use an entirely different set of logic and the TargetLatestAspNetCoreRuntimePatch property is entirely ignored.

[tmds]

Does #801 need to be ported to 2.2?

Yes, we should port this fix to the condition.

EnableWebSdkImplicitPackageVersions is false, as set by dotnet/websdk#414.

We count on the logic that is in the websdk, which is not in the .NET SDK.

The .NET SDK disables the websdk logic with a line in Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.DefaultItems.props:

<EnableWebSdkImplicitPackageVersions>false</EnableWebSdkImplicitPackageVersions>

Maybe removing that line in source-build is enough to make things work?

CC @dsplaisted @natemcmaster

[tmds]

Maybe removing that line in source-build is enough to make things work?

I'm giving this a try, by patching the Microsoft 2.1.202 SDK (I don't have a source-build version available atm). This works, but gives me a warning:

/home/tmds/Downloads/dotnet-2.2.102/sdk/2.2.102/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.DefaultItems.targets(153,5): warning NETSDK1071: A PackageReference to 'Microsoft.AspNetCore.App' specified a Version of `2.2.1`. Specifying the version of this package is not recommended. For more information, see https://aka.ms/sdkimplicitrefs [/tmp/web/web.csproj]

To get rid of the warning, I remove the ImplicitPackageReferenceVersion for Microsoft.AspNetCore.App/Microsoft.AspNetCore.All in Microsoft.NETCoreSdk.BundledVersions.props.

Unfortunately, this also gets rid of the warning when explicitly specifying a version in the csproj file as the websdk doesn't emit such a warning. (This is the same behavior as the 2.1 sdks.)

[tmds]

We count on the logic that is in the websdk

Note that this is also the ability to override the default version using DefaultPatchVersionForAspNetCoreAll2_2, DefaultPatchVersionForAspNetCoreApp2_2.

[dsplaisted]

The way the implicit versions work changed in 2.2. See dotnet/cli#10018 and dotnet/sdk#2533. Properties such as DefaultPatchVersionForAspNetCoreApp2_2 are no longer used.

It looks like the problem with source build may be that TargetLatestAspNetCoreRuntimePatch is no longer used. Instead, use the TargetLatestRuntimePatch property, which will apply to both .NET Core and ASP.NET Core. (If you weren't setting that previously, then perhaps you would not have been getting security fixes in the base .NET Core runtime).

[dseefeld]

@dsplaisted Did you see the comment from @omajid at the end of the original issue description?

The TargetLatestRuntimePatch flag added by dotnet/sdk#2533 lets me get the old behaviour via dotnet restore -p:TargetLatestRuntimePatch=true:

  Installing Microsoft.NETCore.App 2.2.1.                        
  Installing Microsoft.NETCore.Targets 2.0.0.                        
  Installing Microsoft.AspNetCore.App 2.2.1. 

However, this also forces Microsoft.NETCore.App to 2.2.1, which is against the goals of the roll-forward design: dotnet/designs#36

We need to make TargetLatestRuntimePatch or a Microsoft.AspNetCore.App/All-specific variant the default for source-build to get secure builds.

[tmds]

Properties such as DefaultPatchVersionForAspNetCoreApp2_2 are no longer used.

We are using that property to inform older SDKs of the latest ASP.NET Core version.
We care about that, because source-build SDKs bundle ASP.NET Core with the app (there is no shared framework).
Without this, older SDKs should no longer be used, because they use an out-dated version of ASP.NET Core, which cannot be overridden.

[dsplaisted]

I think perhaps this is what you want:

  <ItemGroup>
    <ImplicitPackageReferenceVersion Update="Microsoft.AspNetCore.App">
      <DefaultVersion>%(LatestVersion)</DefaultVersion>
    </ImplicitPackageReferenceVersion>
    <ImplicitPackageReferenceVersion Update="Microsoft.AspNetCore.All">
      <DefaultVersion>%(LatestVersion)</DefaultVersion>
    </ImplicitPackageReferenceVersion>
  </ItemGroup>

This will set the default version of ASP.NET to reference to the latest version.

Without this, older SDKs should no longer be used, because they use an out-dated version of ASP.NET Core, which cannot be overridden.

I think you do need to update to a newer SDK to get the updated version of ASP.NET Core by default. The SDK is where the latest version is recorded, so the normal way for self-contained projects to get updated patches is to be built with an updated SDK. For source-build, it sounds like ASP.NET Core is basically always treated as "self-contained".

Separately, are you aware of the plans for .NET Core 3.0? ASP.NET won't be referencable as packages in the same way, so you'll probably need to change how it works with source build. Here is an overview of the design: https://github.com/dotnet/designs/pull/50/files?short_path=ebf265d#diff-ebf265d0476041dd8cab82fa24ab5b0e

[tmds]

I think perhaps this is what you want:

If we do that, we want our users to only use the latest version of the 2.2 SDK. Using an older version is a security risk.

it sounds like ASP.NET Core is basically always treated as "self-contained".

This is because ASP.NET Core isn't in a source-buildable state for 2.x.
We (Red Hat, community, and also Microsoft) don't like this type of differences: it makes a source-build version of .NET Core less feature rich than a Microsoft build version of .NET.

Separately, are you aware of the plans for .NET Core 3.0? ASP.NET won't be referencable as packages in the same way, so you'll probably need to change how it works with source build.

For 3.0, ASP.NET Core is expected to be source-buildable.

[dagood]

  <ItemGroup>
    <ImplicitPackageReferenceVersion Update="Microsoft.AspNetCore.App">
      <DefaultVersion>%(LatestVersion)</DefaultVersion>
    </ImplicitPackageReferenceVersion>
    <ImplicitPackageReferenceVersion Update="Microsoft.AspNetCore.All">
      <DefaultVersion>%(LatestVersion)</DefaultVersion>
    </ImplicitPackageReferenceVersion>
  </ItemGroup>

In source-build, LatestVersion isn't up to date, and opening up the Microsoft 2.2.102 build (specifically https://dotnet.microsoft.com/download/thank-you/dotnet-sdk-2.2.102-windows-x64-binaries), it's not up to date either. Microsoft.NETCoreSdk.BundledVersions.props from that url:

...
    <!-- Latest patch versions for each minor version of .NET Core -->
    <LatestPatchVersionForNetCore1_0 Condition="'$(LatestPatchVersionForNetCore1_0)' == ''">1.0.13</LatestPatchVersionForNetCore1_0>
    <LatestPatchVersionForNetCore1_1 Condition="'$(LatestPatchVersionForNetCore1_1)' == ''">1.1.10</LatestPatchVersionForNetCore1_1>
    <LatestPatchVersionForNetCore2_0 Condition="'$(LatestPatchVersionForNetCore2_0)' == ''">2.0.9</LatestPatchVersionForNetCore2_0>
    <LatestPatchVersionForNetCore2_1 Condition="'$(LatestPatchVersionForNetCore2_1)' == ''">2.1.2</LatestPatchVersionForNetCore2_1>
  </PropertyGroup>
  <ItemGroup>
    <ImplicitPackageReferenceVersion Include="Microsoft.NETCore.App" TargetFrameworkVersion="1.0" DefaultVersion="1.0.5" LatestVersion="1.0.13"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.NETCore.App" TargetFrameworkVersion="1.1" DefaultVersion="1.1.2" LatestVersion="1.1.10"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.NETCore.App" TargetFrameworkVersion="2.0" DefaultVersion="2.0.0" LatestVersion="2.0.9"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.NETCore.App" TargetFrameworkVersion="2.1" DefaultVersion="2.1.0" LatestVersion="2.1.6"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.NETCore.App" TargetFrameworkVersion="2.2" DefaultVersion="2.2.0" LatestVersion="2.2.1"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.AspNetCore.App" TargetFrameworkVersion="2.1" DefaultVersion="2.1.1" LatestVersion="2.1.6"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.AspNetCore.All" TargetFrameworkVersion="2.1" DefaultVersion="2.1.1" LatestVersion="2.1.6"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.AspNetCore.App" TargetFrameworkVersion="2.2" DefaultVersion="2.2.0" LatestVersion="2.2.1"/>
    <ImplicitPackageReferenceVersion Include="Microsoft.AspNetCore.All" TargetFrameworkVersion="2.2" DefaultVersion="2.2.0" LatestVersion="2.2.1"/>
  </ItemGroup>
</Project>

It seems like the old versions would make this not work, is that right? Is it a broader concern that these aren't up to date in official sources/binaries?

[crummel]

Adding TargetLatestRuntimePatch got me the same results. I'm trying the ImplicitPackageReferenceVersion approach now.

[dagood]

I think perhaps this is what you want:

If we do that, we want our users to only use the latest version of the 2.2 SDK. Using an older version is a security risk.

The goal here is to (continue to) be able to maintain an old version of the SDK without rebuilding it by telling it to bring down newer Microsoft.NETCore.App/Microsoft.AspNetCore.App by changing env variables, correct?

Maybe we could change how the ImplicitPackageReferenceVersions are generated to use properties if specified. Combined with the DefaultVersion update, might be something like this for each one:

<ImplicitPackageReferenceVersion
  Include="Microsoft.AspNetCore.App"
  TargetFrameworkVersion="2.1"
  DefaultVersion="2.1.1"
  LatestVersion="2.1.6"/>
<ImplicitPackageReferenceVersion
  Update="Microsoft.AspNetCore.App"
  DefaultVersion="$(LatestPatchVersionForNetCore2_1)"
  Condition="
    '$(LatestPatchVersionForNetCore2_1)' != '' and
    '%(TargetFrameworkVersion)' == '2.1'"/>
<ImplicitPackageReferenceVersion
  Update="Microsoft.AspNetCore.App"
  LatestVersion="%(DefaultVersion)"/>

[tmds]

The goal here is to (continue to) be able to maintain an old version of the SDK without rebuilding it by telling it to bring down newer Microsoft.NETCore.App/Microsoft.AspNetCore.App by changing env variables, correct?

Yes, for Microsoft.AspNetCore.{App,All}.

Maybe we could change how the ImplicitPackageReferenceVersions are generated to use properties if specified. Combined with the DefaultVersion update, might be something like this for each one:

Yes, I think that would work.
We may opt for the simpler version proposed earlier, and only provide the latest sdk for 2.2 to avoid people using out-dated ASP.NET Core versions.

[omajid]

We have a workaround for 2.2. Is that good enough for now? Should we close this?

[crummel]

With simultaneous 2.1 and 2.2 releases we have to keep the workaround available anyway, so I think this is okay to close.