Add property for opting out of apphost signing in macOS #18216

mateoatr · 2021-06-10T23:29:59Z

Add support for the new CreateAppHost argument, enableMacOSCodeSign. This PR adds property _EnableMacOSCodeSign to allow users to opt-out of apphost signing in macOS.

Dependent upon dotnet/runtime#53913.

ghost · 2021-06-10T23:30:02Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

vitek-karas · 2021-06-21T08:41:12Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

                           ('$(SelfContained)' == 'true' or
                            ('$(RuntimeIdentifier)' != '' and '$(_TargetFrameworkVersionWithoutV)' >= '2.1') or
-                            ('$(_TargetFrameworkVersionWithoutV)' >= '3.0' and $(_OnOsx) != 'true'))">true</UseAppHost>
+                            '$(_TargetFrameworkVersionWithoutV)' >= '3.0')">true</UseAppHost>


While I support this change I think we should make the change more visible. I don't know what's the process for this - technically it could be seen as a mild breaking change. @agocke or @marcpopMSFT might know how we track these. Short version:

.NET 6 SDK will generate apphost executable for macOS apps by default for all TFMs that support apphost

It will now codesign it with the default cert when running on macOS

In .NET 5 we disabled apphosts on macOS because we didn't sign them and default settings OS would not run such executables.

(This will help a lot with making things like dotnet run work on Apple M1 when targetting downlevel TFMs)

Minor changes can go into release notes but full breaking changes get filed as issues in the doc's repo to go into the breaking change documentation. Since it's a change in default behavior, I'd probably lean towards breaking changes so filing an issue with the details using this link: https://github.com/dotnet/docs/issues/new?assignees=&labels=&template=dotnet-breaking-change.md&title=

I agree, this is probably not a breaking change for most people, but it seems good to have it documented as such, just in case.

Opened the breaking change issue: dotnet/docs#24766

There is another issue with this change unfortunately:
On Apple M1 when running with native SDK (so arm64), if I try to build a 3.1 app, it would:

Try to generate apphost (as per the above change)

It would imply implicit RID which is the SDK's RID, which would be osx-arm64

This will FAIL - there's no osx-arm64. It's possible to try this today:

> dotnet new console -f netcoreapp3.1 > dotnet publish -r osx-arm64 --self-contained false error NETSDK1084: There is no application host available for the specified RuntimeIdentifier 'osx-arm64'.

Related to the discussion here: https://gist.github.com/richlander/8cd7b4570775d40f9024561883d79516#overriding-the-implict-rid

@richlander for context

To clarify per Teams chat, the plan is for the implicit RID when targeting downlevel runtimes when using the arm64 SDK is to cross-target to x64 for emulation.

vitek-karas

I think we will have to somehow address the implicit RID on Apple M1 problem before merging this, as it would make what is a very mild breaking change a very hard break (dotnet build doesn't work by default on 3.1 app on osx-arm64 SDK).

vitek-karas · 2021-06-25T12:25:43Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

+    <RuntimeIdentifier Condition="$(NETCoreSdkRuntimeIdentifier.EndsWith('arm64')) and
+                                  $(NETCoreSdkRuntimeIdentifier.StartsWith('osx')) and
+                                  '$(_TargetFrameworkVersionWithoutV)' &lt; '6.0'">$([System.String]::Copy('$(NETCoreSdkRuntimeIdentifier)').Replace("arm64", "x64"))</RuntimeIdentifier>
+    <RuntimeIdentifier Condition="'$(RuntimeIdentifier)' == ''">$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier>


@wli3 or @dsplaisted could you please review this piece? The goal is when running on osx-arm64 and working with downlevel app (TFM < 6.0) to switch to osx-x64 since the first version which supports osx-arm64 is .NET 6 (downlevel versions don't have support and only run on x64, also macOS arm64 can emulate x64, so it's the right thing to do to run the app as x64).

The main concern we had was how to construct the new RID - specifically if it should maintain the potential OS version and such in the RID or not.

This will only apply for projects that opt in to UseCurrentRuntimeIdentifier, or when --use-current-runtime is passed on the command line. Is that the intention?

I thought that we were going to use this type of logic for the the DefaultAppHostRuntimeIdentifier.

It's not great that we have to do string manipulation to check the RuntimeIdentifier, but I don't think there's a better option.

I do think when setting the value you don't need to use String.Copy. You can just call replace directly, String.Copy is a workaround to call replace on item metadata, and shouldn't be needed for a property value.

I don't know what's the difference between UseCurrentRuntimeIdentifier and DefaultAppHostRuntimeIdentifier. The intent of this change is that typing dotnet run on a 3.1 project when running on macOS arm64 SDK will produce macOS x64 app output (apphost) and thus will be able to execute the app. This then also means that dotnet build should behave that way.

In short - SDK should not default to the RID of the SDK when targeting downlevel TFMs and such TFMs are not supported on the RID. It should default to whatever RID is actually supported for the TFM on the current machine.

DefaultAppHostRuntimeIdentifier is the primary property you should be concerned with. That's what determines which apphost gets used if a RuntimeIdentifier (or AppHostRuntimeIdentifier) isn't otherwise specified.

UseCurrentRuntimeIdentifier is a new-ish option to allow building a self-contained app without explicitly specifying the RuntimeIdentifier. See #14093. It is probably a good idea for UseCurrentRuntimeIdentifier to switch from arm64 to x64 when targeting .NET 5.0 or earlier, but that's not the mainline scenario you're trying to address.

That makes sense. I've changed things so we modify the DefaultAppHostRuntimeIdentifier.

If UseCurrentRuntimeIdentifier isn't set to DefaultAppHostRuntimeIdentifier I think we should set that too. Otherwise I think that scenario will be broken for Mac.

vitek-karas

Looks good to me, but this should also be reviewed by somebody from SDK to validate mainly the implicit RID calculation.

agocke · 2021-06-28T17:55:55Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

    <DefaultAppHostRuntimeIdentifier Condition="$(DefaultAppHostRuntimeIdentifier.StartsWith('win')) and '$(PlatformTarget)' == 'x86'">win-x86</DefaultAppHostRuntimeIdentifier>
    <DefaultAppHostRuntimeIdentifier Condition="$(DefaultAppHostRuntimeIdentifier.StartsWith('win')) and '$(PlatformTarget)' == 'ARM'">win-arm</DefaultAppHostRuntimeIdentifier>
    <DefaultAppHostRuntimeIdentifier Condition="$(DefaultAppHostRuntimeIdentifier.StartsWith('win')) and '$(PlatformTarget)' == 'ARM64'">win-arm64</DefaultAppHostRuntimeIdentifier>
+    <DefaultAppHostRuntimeIdentifier Condition="$(NETCoreSdkRuntimeIdentifier.EndsWith('arm64')) and


Do we need EndsWith('arm64')? Isn't it good enough to say osx+<6.0?

And why use NETCoreSdkRuntimeIdentifier instead of DefaultAppHostRuntimeIdentifier?

Changed to use DefaultAppHostRuntimeIdentifier instead of NETCoreSdkRuntimeIdentifier. I don't think we need the Ends... here, it's enough to check for osx & < 6.0, but left it here to make things clearer (since we are replacing that same string later).

Do not attempt to codesign when xplat building from macOS

Remove unnecessary file

dsplaisted

There are tests that verify that we don't sign the apphost, but it doesn't look like there are any that verify that the apphost is signed correctly. It also looks like it's not currently being signed in the framework-dependent case.

dsplaisted · 2021-06-30T15:14:12Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

+    <DefaultAppHostRuntimeIdentifier Condition="$(DefaultAppHostRuntimeIdentifier.EndsWith('arm64')) and
+                                  $(DefaultAppHostRuntimeIdentifier.StartsWith('osx')) and
+                                  '$(_TargetFrameworkVersionWithoutV)' &lt; '6.0'">$(DefaultAppHostRuntimeIdentifier.Replace("arm64", "x64"))</DefaultAppHostRuntimeIdentifier>


I think it would be good to have a comment explaining the reasoning behind this logic.

dsplaisted · 2021-06-30T15:15:02Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

                     Exists('$(AppHostSourcePath)')">
    <PropertyGroup>
      <_UseWindowsGraphicalUserInterface Condition="($(RuntimeIdentifier.StartsWith('win')) or $(DefaultAppHostRuntimeIdentifier.StartsWith('win'))) and '$(OutputType)'=='WinExe'">true</_UseWindowsGraphicalUserInterface>
+      <_EnableMacOSCodeSign Condition="$([MSBuild]::IsOSPlatform(`OSX`)) and Exists('/usr/bin/codesign') and $(RuntimeIdentifier.StartsWith('osx'))">true</_EnableMacOSCodeSign>


For framework-dependent apps, RuntimeIdentifier won't be set. Don't you still want to sign the apphost in that case?

We definitely want to sign the apphost in all cases

dsplaisted · 2021-06-30T15:18:18Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs

+            var tfm = testAsset.TargetFrameworkMoniker;
+            if (!string.IsNullOrEmpty(tfm) && (tfm.Contains("netcoreapp") || tfm.Contains("net5") || tfm.Contains("net6")))
+            {
+                var netCoreTFM = new Version(new string(tfm.Where(c => !char.IsLetter(c)).ToArray()));


I think we should use NuGet TargetFramework parsing APIs (NuGetFramework.Parse I believe) here instead of ad-hoc manual parsing.

dsplaisted · 2021-06-30T15:20:39Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs

+            var psi = new ProcessStartInfo()
+            {
+                Arguments = $"-d {appHostFullPath}",
+                FileName = @"/usr/bin/codesign",
+                RedirectStandardError = true
+            };
+
+            using (var codesign = Process.Start(psi))
+            {
+                codesign.Start();
+                codesign.StandardError.ReadToEnd()


I'd suggest using RunExeCommand here, which handles running the process and capturing the output.

dsplaisted · 2021-06-30T15:21:13Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs

+            var appHostFullPath = Path.Combine(outputDirectory.FullName, hostExecutable);
+
+            // Check that the apphost was not signed
+            var psi = new ProcessStartInfo()


Also here for using RunExeCommand

dsplaisted · 2021-06-30T15:25:30Z

src/Tests/Microsoft.NET.TestFramework/TestAsset.cs


        public TestAsset WithTargetFramework(string targetFramework, string projectName = null)
        {
+            TargetFrameworkMoniker = targetFramework;


Most of our test assets don't set the TargetFramework via WithTargetFramework. So this new property would only be set for a small portion of our tests, while it looks like it should work generally. Can you try to find another way to solve the problem? For example, you could add a targetFramewokr parameter to the GetExpectedFilesFromBuild method.

Remove TargetFrameworkMoniker from TestAsset

lewing · 2021-07-11T03:44:51Z

This is approved and green, is there something that needs additional review?

vitek-karas · 2021-07-11T13:29:18Z

We need sign-off from somebody from SDK (for example Daniel).

dsplaisted

A few comments, but looks good.

dsplaisted · 2021-07-12T15:35:28Z

src/Tasks/Microsoft.NET.Build.Tasks/CreateAppHost.cs

            }
+            catch (AppHostSigningException ex)
+            {
+                throw new BuildErrorException(Strings.AppHostSigningFailed, ex.Message, ex.ExitCode.ToString());


This is passing the exit code as a parameter, but it isn't being used in the AppHostSigningFailed string. Probably either the string should be changed to include the exit code, or we don't need to pass it here.

dsplaisted · 2021-07-12T16:08:52Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

+    <!-- If we are running on an M1 with a native SDK and the TFM is < 6.0, we have to use a x64 apphost since there are no osx-arm64 apphosts previous to .NET 6.0. -->
+    <DefaultAppHostRuntimeIdentifier Condition="$(DefaultAppHostRuntimeIdentifier.EndsWith('arm64')) and
+                                  $(DefaultAppHostRuntimeIdentifier.StartsWith('osx')) and
+                                  '$(_TargetFrameworkVersionWithoutV)' &lt; '6.0'">$(DefaultAppHostRuntimeIdentifier.Replace("arm64", "x64"))</DefaultAppHostRuntimeIdentifier>


Use [MSBuild]::VersionLessThan here instead of < for version comparison.

dsplaisted · 2021-07-12T16:46:09Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs

+            if (!string.IsNullOrEmpty(targetFramework))
+            {
+                var parsedTargetFramework = NuGetFramework.Parse(targetFramework);
+                if (parsedTargetFramework.Version >= new Version(5, 0, 0, 0))


The Version type has somewhat unexpected comparison rules (5.0 is not the same as 5.0.0.0), so I would suggest just comparing against the Major version.

Suggested change

if (parsedTargetFramework.Version >= new Version(5, 0, 0, 0))

if (parsedTargetFramework.Version.Major >= 5)

dsplaisted · 2021-07-12T16:46:29Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs

+                if (parsedTargetFramework.Version >= new Version(5, 0, 0, 0))
+                    expectedFiles.Add($"ref/{testProjectName}.dll");
+
+                if (parsedTargetFramework.Version < new Version(6, 0, 0, 0))


Suggested change

if (parsedTargetFramework.Version < new Version(6, 0, 0, 0))

if (parsedTargetFramework.Version.Major < 6)

dsplaisted · 2021-07-12T16:47:21Z

src/Tests/Microsoft.NET.Build.Tests/AppHostTests.cs


-        [PlatformSpecificTheory(TestPlatforms.Windows | TestPlatforms.Linux | TestPlatforms.FreeBSD)]
-        [InlineData("netcoreapp3.0")]
+        [PlatformSpecificTheory(TestPlatforms.Windows | TestPlatforms.Linux | TestPlatforms.FreeBSD | TestPlatforms.OSX)]


Now that this runs on macOS, can this just be a Theory instead of a PlatformSpecificTheory?

Log error code

wli3 requested a review from vitek-karas June 14, 2021 22:48

wli3 added the Team:Runtime label Jun 14, 2021

vitek-karas reviewed Jun 15, 2021

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets Outdated Show resolved Hide resolved

mateoatr force-pushed the codesign branch from 6b9c582 to 4b25370 Compare June 18, 2021 21:57

vitek-karas reviewed Jun 21, 2021

View reviewed changes

mateoatr mentioned this pull request Jun 21, 2021

Generate an apphost for macOS by default dotnet/docs#24766

Closed

23 tasks

mateoatr marked this pull request as ready for review June 22, 2021 23:56

vitek-karas requested changes Jun 23, 2021

View reviewed changes

vitek-karas reviewed Jun 25, 2021

View reviewed changes

vitek-karas approved these changes Jun 25, 2021

View reviewed changes

agocke reviewed Jun 28, 2021

View reviewed changes

Mateo Torres Ruiz added 13 commits June 28, 2021 14:42

Add EnableMacOSCodeSign

faa01fb

Generate apphost by default on macOS

9dd0ee1

Do not attempt to codesign when xplat building from macOS

Update resources

1e7f52b

Adjust the expected files depending on the sdk version used

40fcbb4

Rename to TFM

98f7d0d

Remove unnecessary file

Store TFM as string

a573228

Update tests

e4022e3

Use a x64 rid if TFM < 6.0 and we are on a native SDK on a macOS.

b0c3ea1

Change targets so that the OS version is not changed

6f74069

Fix condition

dbbc6cf

Fix binary suffix

4796a68

Set DefaultAppHostRuntimeIdentifier instead of RuntimeIdentifier

38f4d52

Modify condition

d3d9021

mateoatr force-pushed the codesign branch from 35e2b12 to d3d9021 Compare June 28, 2021 21:44

Mateo Torres Ruiz added 2 commits June 29, 2021 14:06

Update xlf files

b29dc0a

Remove directory

55b6de4

dsplaisted reviewed Jun 30, 2021

View reviewed changes

Update tests

9a664a6

Remove TargetFrameworkMoniker from TestAsset

Mateo Torres Ruiz added 2 commits July 7, 2021 13:56

PR feedback

f738d4f

Allow test asset to be copied if present

442cf38

mateoatr requested a review from dsplaisted July 8, 2021 03:49

dsplaisted approved these changes Jul 12, 2021

View reviewed changes

Mateo Torres Ruiz added 2 commits July 12, 2021 10:52

Fix version comparison

4c20bf5

Log error code

Use /langversion:preview

b39fd7d

mateoatr merged commit 714082d into dotnet:main Jul 14, 2021

sfoslund mentioned this pull request Jul 19, 2021

Architecture and OS targeting improvements for .NET 6 #18832

Closed

7 tasks

dsplaisted mentioned this pull request Aug 16, 2021

Unable to sign console apps in macOS with .net 6 preview 7 dotnet/core#6591

Closed

	if (parsedTargetFramework.Version >= new Version(5, 0, 0, 0))
	if (parsedTargetFramework.Version.Major >= 5)

	if (parsedTargetFramework.Version < new Version(6, 0, 0, 0))
	if (parsedTargetFramework.Version.Major < 6)

Add property for opting out of apphost signing in macOS #18216

Add property for opting out of apphost signing in macOS #18216

Uh oh!

Conversation

mateoatr commented Jun 10, 2021

Uh oh!

ghost commented Jun 10, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lewing commented Jul 11, 2021

Uh oh!

vitek-karas commented Jul 11, 2021

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels