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

Add managed Mac signing implementation #107378

Closed
wants to merge 31 commits into from

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Sep 4, 2024

Adds a Mac signing library in managed code that can sign an apphost built on Windows for a Mac.

The signing is done with a modified version of https://github.com/filipnavara/CodeSign, with much of the code unrelated to ad-hoc signing removed. This managed implementation will only ever be used to ad-hoc sign; maintaining full signing functionality is out of scope.

I tried to get this byte-for-byte identical to codesign, but documentation is sparse. The output of this implementation is the same except for the length of the padding at the end of the file. This affects the headers at the beginning, so the hash of the first page of the file in the signature is also different. CreateAppHost.cs has a test for this (ManagedSignerMatchesCodesignOutput).

@jtschuster jtschuster force-pushed the AdHocSigningMac2 branch 2 times, most recently from 46fda6b to 0f8c240 Compare September 5, 2024 16:18
@jtschuster jtschuster marked this pull request as ready for review September 24, 2024 15:59
@elinor-fung
Copy link
Member

I haven't actually looked through the changes, but here are some high level comments/questions:

Deployment:

  • This is adding four new libraries. I know that it is keeping the structure from filipnavara/CodeSign, but for .NET SDK purposes, would it make sense to collapse them?
  • Is there any additional work needed to get the new libraries deployed as part of SDK? Or does that flow with the Microsoft.NET.HostModel package that the sdk repo consumes?

I think we should choose how we are viewing integrating the code from filipnavara/CodeSign into the runtime repo:

  1. As external libraries
    • I'd prefer having it clearly be external based on repo layout and marked with the source version/commit and changes we made - for example, what we do with external native libs under src/native/external
  2. Fully merged with the runtime repo
    • Naming, style, warnings, etc. should all be consistent with other code such that it looks like a normal part of the runtime repo instead of external libraries

I'd prefer (2) if possible. I'm expecting this to be more of a one-off port rather than something we'd need to keep syncing with upstream, so I'd prefer this be a more natural part of our repo.

@jtschuster
Copy link
Member Author

I haven't actually looked through the changes, but here are some high level comments/questions:

Deployment:

  • This is adding four new libraries. I know that it is keeping the structure from filipnavara/CodeSign, but for .NET SDK purposes, would it make sense to collapse them?

I think that's a good idea, then we can expose only the parts necessary for signing as public.

  • Is there any additional work needed to get the new libraries deployed as part of SDK? Or does that flow with the Microsoft.NET.HostModel package that the sdk repo consumes?

I was expecting it to flow with the Microsoft.NET.HostModel package, but it looks like it needs some changes to include the new libs in the package.

I think we should choose how we are viewing integrating the code from filipnavara/CodeSign into the runtime repo:

  1. As external libraries

    • I'd prefer having it clearly be external based on repo layout and marked with the source version/commit and changes we made - for example, what we do with external native libs under src/native/external
  2. Fully merged with the runtime repo

    • Naming, style, warnings, etc. should all be consistent with other code such that it looks like a normal part of the runtime repo instead of external libraries

I'd prefer (2) if possible. I'm expecting this to be more of a one-off port rather than something we'd need to keep syncing with upstream, so I'd prefer this be a more natural part of our repo.

I agree with going with (2) for integration. There's a large amount of code related to actual signing with a certificate that was removed and isn't expected to be added, so it's not likely we will be syncing with upstream.

@agocke
Copy link
Member

agocke commented Oct 4, 2024

One other thing: I know this isn't how Filip's code was originally written, but it would be great if we could make this based off of a memory-mapped file.

The reason being, we saw in the past that AV's really didn't like it when we wrote an executable file, then immediately opened it again for writing. We switched to memory-mapped files for editing the host and pretty much all of those AV bugs went away.

We now already have a memory-mapped view open in the HostWriter for the output. If we could reuse that view, we could avoid re-opening the binary and potentially repaging.

@jtschuster
Copy link
Member Author

it would be great if we could make this based off of a memory-mapped file.

Sure, I'll get started on that.

- Add alignment helpers
- Make closer to byte-for-byte identical to codesign
  - Add empty requirements blob
  - There is a non-constant size allocated for the CMS blob that is
    unused, but affects the file size in the headers of the file, which
    causes different code hashes, so the files aren't byte-for-byte
    identical.
  - There was a 4-byte zeroed field in the CodeDirectory header that
    was present in codesign's output, but I cannot find documentation
    for. I've added an additional field in CodeDirectoryPreencryptHeader
    to account for this.
- Add InternalsVisibleTo for testing MachO internals.
Use correct 'using' syntax in test
@jtschuster jtschuster marked this pull request as ready for review October 11, 2024 19:29
@am11
Copy link
Member

am11 commented Oct 11, 2024

Would it make sense to check in the generated code, remove the generator, and then reduce the code to the bare minimum required for code-signing use cases in this repo?

We had a similar approach during the managed objwriter rewrite (#92705), where we selected only the necessary parts of dependency libraries for long-term maintenance in the runtime repo.

@jtschuster
Copy link
Member Author

Would it make sense to check in the generated code, remove the generator, and then reduce the code to the bare minimum required for code-signing use cases in this repo?

That's not a bad idea, this could be pared down more, and now that I've spent enough time getting to know the internals I don't feel as worried about breaking it.

@jtschuster
Copy link
Member Author

Closing in favor of a more limited rewrite at #108992

@jtschuster jtschuster closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants