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

Use managed Mach-O signer on non-Mac hosts #45019

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Condition="'$(UseAppHost)' == 'true' and '$(_IsExecutable)' == 'true'">
<PropertyGroup>
<_UseWindowsGraphicalUserInterface Condition="($(RuntimeIdentifier.StartsWith('win')) or $(DefaultAppHostRuntimeIdentifier.StartsWith('win'))) and '$(OutputType)'=='WinExe'">true</_UseWindowsGraphicalUserInterface>
<_EnableMacOSCodeSign Condition="'$(_EnableMacOSCodeSign)' == '' and $([MSBuild]::IsOSPlatform(`OSX`)) and Exists('/usr/bin/codesign') and
<_EnableMacOSCodeSign Condition="'$(_EnableMacOSCodeSign)' == '' and
($(RuntimeIdentifier.StartsWith('osx')) or $(AppHostRuntimeIdentifier.StartsWith('osx')))">true</_EnableMacOSCodeSign>
<_UseSingleFileHostForPublish Condition="'$(PublishSingleFile)' == 'true' and
'$(SelfContained)' == 'true' and
Expand Down
149 changes: 102 additions & 47 deletions test/Microsoft.NET.Build.Tests/AppHostTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers.Binary;
using System.Diagnostics;
using System.Reflection.PortableExecutable;
using System.Text.RegularExpressions;
Expand Down Expand Up @@ -150,71 +151,68 @@ public void It_does_not_try_to_codesign_non_osx_app_hosts(string targetFramework
Directory.Delete(buildProjDir, true);
}

[PlatformSpecificTheory(TestPlatforms.OSX)]
[InlineData("netcoreapp3.1")]
[InlineData("net5.0")]
[InlineData(ToolsetInfo.CurrentTargetFramework)]
public void It_codesigns_a_framework_dependent_app(string targetFramework)
public static TheoryData<string, string, bool> OsxPublishingOptions()
Copy link
Member

Choose a reason for hiding this comment

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

nit: OsxPublishingOptions -> OsxBuildOptions?

{
var testAsset = _testAssetsManager
.CopyTestAsset("HelloWorld", identifier: targetFramework)
.WithSource()
.WithTargetFramework(targetFramework);

var buildCommand = new BuildCommand(testAsset);
buildCommand
.Execute()
.Should()
.Pass();

var outputDirectory = buildCommand.GetOutputDirectory(targetFramework);
var appHostFullPath = Path.Combine(outputDirectory.FullName, "HelloWorld");

// Check that the apphost is signed
var codesignPath = @"/usr/bin/codesign";
new RunExeCommand(Log, codesignPath, new string[] { "-s", "-", appHostFullPath })
.Execute()
.Should()
.Fail()
.And
.HaveStdErrContaining($"{appHostFullPath}: is already signed");
// osx-arm64 is only supported on net6.0+
string[] x64OnlyTfms = ["netcoreapp3.1", "net5.0"];
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just remove the netcoreapp3.1 and net5.0 coverage here. They've been out of support for 2+ years and I don't think there's anything particularly interesting about them from the host side (even for single-file, I think it is covered by our tests for various bundler options).

string[] tfms = ["net6.0", ToolsetInfo.CurrentTargetFramework];
string[] rids = ["osx-x64", "osx-arm64"];
bool[] selfContained = [true, false];

var sharedTfmParams = from t in tfms
from r in rids
from s in selfContained
select (t, r, s);
var x64OnlyParams = from t in x64OnlyTfms
from s in selfContained
select (t, "osx-x64", s);
TheoryData<string, string, bool> data = new TheoryData<string, string, bool>();
foreach (var (t, r, s) in sharedTfmParams.Concat(x64OnlyParams))
{
data.Add(t, r, s);
}
return data;
}

[PlatformSpecificTheory(TestPlatforms.OSX)]
[InlineData("netcoreapp3.1", false)]
[InlineData("netcoreapp3.1", true)]
[InlineData("net5.0", false)]
[InlineData("net5.0", true)]
[InlineData(ToolsetInfo.CurrentTargetFramework, false)]
[InlineData(ToolsetInfo.CurrentTargetFramework, true)]
public void It_codesigns_an_app_targeting_osx(string targetFramework, bool selfContained)
[Theory]
[MemberData(nameof(OsxPublishingOptions))]
public void It_codesigns_an_app_targeting_osx(string targetFramework, string rid, bool selfContained)
{
const string testAssetName = "HelloWorld";
var testAsset = _testAssetsManager
.CopyTestAsset("HelloWorld", identifier: targetFramework, allowCopyIfPresent: true)
.CopyTestAsset(testAssetName, identifier: targetFramework)
.WithSource()
.WithTargetFramework(targetFramework);

var buildCommand = new BuildCommand(testAsset);
var buildArgs = new List<string>() { $"/p:RuntimeIdentifier={RuntimeInformation.RuntimeIdentifier}" };
var buildArgs = new List<string>() { $"/p:RuntimeIdentifier={rid}" };
if (!selfContained)
buildArgs.Add("/p:PublishSingleFile=true");
Copy link
Member

Choose a reason for hiding this comment

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

Not part of your change, but I don't think this is necessary. PublishSingleFile shouldn't do anything interesting when just building (other than enable the analyzer I think, but that shouldn't be the point of this test).


buildCommand
.Execute(buildArgs.ToArray())
.Should()
.Pass();

var outputDirectory = buildCommand.GetOutputDirectory(targetFramework, runtimeIdentifier: RuntimeInformation.RuntimeIdentifier);
var appHostFullPath = Path.Combine(outputDirectory.FullName, "HelloWorld");
var outputDirectory = buildCommand.GetOutputDirectory(targetFramework: targetFramework, runtimeIdentifier: rid);
var appHostFullPath = Path.Combine(outputDirectory.FullName, testAssetName);

// Check that the apphost is signed
var codesignPath = @"/usr/bin/codesign";
new RunExeCommand(Log, codesignPath, new string[] { "-s", "-", appHostFullPath })
.Execute()
.Should()
.Fail()
.And
.HaveStdErrContaining($"{appHostFullPath}: is already signed");
HasMachOSignatureLoadCommand(new FileInfo(appHostFullPath)).Should().BeTrue();
// When on a Mac, use the codesign tool to verify the signature as well
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
{
var codesignPath = @"/usr/bin/codesign";
new RunExeCommand(Log, codesignPath, ["-s", "-", appHostFullPath])
.Execute()
.Should()
.Fail()
.And
.HaveStdErrContaining($"{appHostFullPath}: is already signed");
new RunExeCommand(Log, codesignPath, ["-v", appHostFullPath])
.Execute()
.Should()
.Pass();
}
}

[Theory]
Expand Down Expand Up @@ -509,5 +507,62 @@ private static bool IsPE32(string path)
return reader.PEHeaders.PEHeader.Magic == PEMagic.PE32;
}
}

// Reads the Mach-O load commands and returns true if an LC_CODE_SIGNATURE command is found, otherwise returns false
static bool HasMachOSignatureLoadCommand(FileInfo file)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't matter for this change, but we may want this in a shared test helper so that we can add cross-platform checks for single-file publish with signing after dotnet/runtime#110417 goes in.

{
/* Mach-O files have the following structure:
* 32 byte header beginning with a magic number and info about the file and load commands
* A series of load commands with the following structure:
* - 4-byte command type
* - 4-byte command size
* - variable length command-specific data
*/
const uint LC_CODE_SIGNATURE = 0x1D;
using (var stream = file.OpenRead())
{
// Read the MachO magic number to determine endianness
Span<byte> eightByteBuffer = stackalloc byte[8];
stream.ReadExactly(eightByteBuffer);
// Determine if the magic number is in the same or opposite endianness as the runtime
bool reverseEndinanness = BitConverter.ToUInt32(eightByteBuffer.Slice(0, 4)) switch
{
0xFEEDFACF => false,
0xCFFAEDFE => true,
_ => throw new InvalidOperationException("Not a 64-bit Mach-O file")
};
// 4-byte value at offset 16 is the number of load commands
// 4-byte value at offset 20 is the size of the load commands
stream.Position = 16;
ReadUints(stream, eightByteBuffer, out uint loadCommandsCount, out uint loadCommandsSize);
// Mach-0 64 byte headers are 32 bytes long, and the first load command will be right after
stream.Position = 32;
bool hasSignature = false;
for (int commandIndex = 0; commandIndex < loadCommandsCount; commandIndex++)
{
ReadUints(stream, eightByteBuffer, out uint commandType, out uint commandSize);
if (commandType == LC_CODE_SIGNATURE)
{
hasSignature = true;
}
stream.Position += commandSize-8;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this is a little clearer that it is accounting for what ReadUInts did.

Suggested change
stream.Position += commandSize-8;
stream.Position += commandSize - eightByteBuffer.Length;

}
Debug.Assert(stream.Position == loadCommandsSize + 32);
return hasSignature;

void ReadUints(Stream stream, Span<byte> buffer, out uint val1, out uint val2)
Copy link
Member

Choose a reason for hiding this comment

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

nit: ReadUints -> ReadUInts

{
stream.ReadExactly(buffer);
val1 = BitConverter.ToUInt32(buffer.Slice(0, 4));
val2 = BitConverter.ToUInt32(buffer.Slice(4, 4));
if (reverseEndinanness)
{
val1 = BinaryPrimitives.ReverseEndianness(val1);
val2 = BinaryPrimitives.ReverseEndianness(val2);
}
}
}
}

}
}
Loading