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

Conversation

jtschuster
Copy link
Member

In dotnet/runtime#108992, we added a managed ad-hoc signer that's used in HostWriter.CreateAppHost. This PR enables it to be used in the sdk when publishing for osx on non-Mac hosts so that developers can distribute an ad-hoc signed binary without using a Mac.

Tests on all platforms ensure that there is a load command with the LC_CODE_SIGNATURE command type in the Mach-O binary. On MacOS, codesign is used to fully validate the signature in addition.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Nov 21, 2024
Copy link

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.

1 similar comment
Copy link

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.

.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).

Comment on lines 189 to 190
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).

[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?

@@ -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.

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

{
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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Host untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants