Codesign apphosts on Mac #53913
Codesign apphosts on Mac #53913
Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke Issue DetailsUse
|
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
Outdated
Show resolved
Hide resolved
...ts/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.AppHost.Tests/AppHostUpdateTests.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
Outdated
Show resolved
Hide resolved
| @@ -233,6 +239,19 @@ void FindBundleHeader() | |||
| return headerOffset != 0; | |||
| } | |||
|
|
|||
| private static void CodeSign(string args) | |||
| { | |||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | |||
There was a problem hiding this comment.
I'd throw or assert here. If this ever gets called on a non-mac, that's a bug, not a recoverable failure
There was a problem hiding this comment.
I think it's fine to assert, but I don't think we should throw. Especially since we build the entire feature as "nice to have" - it only works on mac. If I build on Linux and target mac I will have to codesign manually. So if I build on Linux and for some reason we try to codesign, it should not fail the build as the signing is not expected or it's optional.
Unfortunately we don't have a way to write to the msbuild log. Ideally we would produce at least an Info message about this so that it's easier to diagnose. I filed #53923 to hopefully improve on this one day.
There was a problem hiding this comment.
If we eventually intend to implement codesigning on non-Mac that makes sense, but otherwise I'm actually fine with failing if we're on a Mac and the codesign tool is not found. As far as I can tell it's available on every mac back to 10.4 -- that looks like it should be plenty for all our releases of CoreCLR.
| return; | ||
|
|
||
| using (var p = Process.Start(codesign, args)) | ||
| p.WaitForExit(); |
There was a problem hiding this comment.
I think we should check the result - what if signing fails for whatever reason?
Can we make it fail to test this? (maybe give it a nonMACHO input?)
Ideally this would print out the error from the tool (or there would at least be a way to get that) and also fail the build.
Please also consider making codesigning optional even on mac - maybe just an internal MSBuild property (_EnableMacOSCodeSign or something like that). To have a workaround for cases where this may cause issues.
Another consideration is publish. Since build runs as part of publish, the executable will get signed and then it will be published. What happens if the developer then tries to sign the executable again? This time with a real cert.
There was a problem hiding this comment.
I changed the approach so we have an argument to enable the signing in macOS. This will be set from within the SDK in the CreateAppHost task.
Can we make it fail to test this? (maybe give it a nonMACHO input?)
I added a test case in which we sign it two times, causing codesign to fail, thus throwing an exception with the error printed by the tool. The idea here is to catch the exception in the CreateAppHost and use the SDK logging mechanism for printing the error.
What happens if the developer then tries to sign the executable again? This time with a real cert.
If signing with codesign -s <cert> <executable> it will complain that the app was already sign; this can be worked around by forcing the signing (codesign -s <cert> -f <executable>).
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/AppHost/AppHostSigningException.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/AppHost/AppHostSigningException.cs
Outdated
Show resolved
Hide resolved
Modify apphost exceptions inheritance
src/installer/managed/Microsoft.NET.HostModel/AppHost/AppHostUpdateException.cs
Outdated
Show resolved
Hide resolved
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Kotas <[email protected]>
| { | ||
| p.WaitForExit(); | ||
| if (p.ExitCode != 0) | ||
| throw new AppHostSigningException(p.StandardError.ReadToEnd()); |
There was a problem hiding this comment.
Ideally the exception would also record the exit code (And it would be shown as part of the error message from SDK).
| if (enableMacOSCodeSign && RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | ||
| CodeSign(appHostDestinationFilePath); |
There was a problem hiding this comment.
Either here or in the SDK we should handle the case where we're on macOS but we're building for a different platform (Windows or Linux). In that case we should not attempt signing (as it will fail).
I "think" this should be done in the SDK since here we don't have a good way to detect the target platform (we kind of rely on the SDK to tell us the right things).
ghost
locked as resolved and limited conversation to collaborators
Use
codesignfor signing apphosts on macOS. Related to #50375.