Preserve entitlements in MacOS signer #115800
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Write out the updated symtab command when writing Mach-O file - Pass macosCodesign through to test CreateAppHost methods - Remove redundant `codesign` checks - Warn when bundler is told to sign the bundle for a non-macos target
- Allow macosCodesign to be true for non-mac bundles - Allow some padding between string table and signature
- Use the same memory-mapped file instance for placeholder replacement and signing - formatting changes
…ntime into ManagedSignBundles
|
Tagging subscribers to this area: @vitek-karas, @agocke |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the MacOS signer to preserve entitlements in the code signature. Key changes include:
- Enabling nullable reference types and updating signature-related types to support optional code signature blobs.
- Incorporating entitlements and DER entitlements into signature creation with new size estimation logic.
- Removing the signature-presence check in bundle creation to allow signatures with entitlements.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.cs | Updated signature field types; added EmbeddedSignatureBlob getter; commented out a signature equivalence check. |
| src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.CodeSignature.cs | Modified signature creation to include entitlements; updated size estimation logic. |
| src/installer/managed/Microsoft.NET.HostModel/MachO/BinaryFormat/SymbolTableLoadCommand.cs | Renamed structure usage from SymbolTableCommand to SymbolTableLoadCommand. |
| src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs | Removed existing signature check before bundling. |
| Other files | Adjusted or added blob handling to support entitlements and updated enums accordingly. |
Comments suppressed due to low confidence (3)
src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.CodeSignature.cs:279
- The switch from GetCodeSignatureSize to GetLargestSizeEstimate must be validated to ensure that the new estimation accurately reflects the additional size needed for entitlements and related blobs.
return CodeSignature.GetLargestSizeEstimate(fileSize, identifier) + (AlignUp(fileSize, CodeSignatureAlignment) - fileSize);
src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs:244
- The removal of the signature-presence check in HostWriter.cs should be reviewed to ensure that bypassing this validation does not allow unintended signed content into bundles.
if (machObjectFile.HasSignature)
src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.cs:251
- Commenting out the equivalence check for the code signature blob may allow mismatched signatures to be treated as equivalent. Confirm that disabling this check is an intentional change for preserving entitlements.
// if (!CodeSignature.AreEquivalent(a._codeSignatureBlob, b._codeSignatureBlob))
Open
3 tasks
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the managed MacOS signing tool to preserve existing entitlements (and DER entitlements) when re-signing, adjusts signature sizing logic, and renames/refactors related types.
- Make the
CodeSignatureblob nullable and thread through the old signature to preserve entitlements - Update
CreateSignature, slot counts, and size estimates to include entitlements/DER entitlements - Rename
SymbolTableCommandtoSymbolTableLoadCommand; remove obsolete signature‐present check in host bundling
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.cs | Made _codeSignatureBlob nullable, threaded oldSignature, renamed symtab type, disabled signature equivalence |
| src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.CodeSignature.cs | Added oldSignature overload, preserved entitlements, updated slot/hash logic, new GetSignatureSize |
| src/installer/managed/Microsoft.NET.HostModel/MachO/Enums/CodeDirectorySpecialSlot.cs | Added Entitlements and DerEntitlements slots |
| src/installer/managed/Microsoft.NET.HostModel/MachO/Enums/BlobMagic.cs | Added magic values for entitlements and DER entitlements |
| src/installer/managed/Microsoft.NET.HostModel/MachO/BinaryFormat/SymbolTableLoadCommand.cs | Renamed SymbolTableCommand to SymbolTableLoadCommand |
| src/installer/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs | Removed check that prevented bundling over existing signatures |
Comments suppressed due to low confidence (2)
src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.CodeSignature.cs:39
- The new entitlements preservation logic in
CreateSignatureand related size calculations lack targeted unit tests for both entitlements and DER entitlements. Adding tests to cover cases whereoldSignaturecontains each blob will ensure correct slot ordering and hash computation.
internal static CodeSignature CreateSignature(MachObjectFile machObject, MemoryMappedViewAccessor file, string identifier, CodeSignature? oldSignature)
src/installer/managed/Microsoft.NET.HostModel/MachO/MachObjectFile.cs:251
- Disabling the
CodeSignature.AreEquivalentcheck inAreEquivalentcauses signature differences to be ignored, potentially marking non-equivalent binaries as equivalent. Consider restoring or updating this logic to compare entitlements and DER entitlements as part of equivalence.
// if (!CodeSignature.AreEquivalent(a._codeSignatureBlob, b._codeSignatureBlob))
| @@ -219,7 +223,7 @@ public static void RemoveCodeSignatureIfPresent(FileStream bundle) | |||
| } | |||
| if (resized) | |||
| { | |||
| bundle.SetLength(newLength.Value); | |||
| bundle.SetLength(newLength!.Value); | |||
There was a problem hiding this comment.
Using the null-forgiving operator here bypasses null-checks and may cause a NullReferenceException if newLength is unexpectedly null. Consider validating newLength before calling .Value or avoiding force-unwrapping.
Suggested change
| bundle.SetLength(newLength!.Value); | |
| if (newLength == null) | |
| { | |
| throw new InvalidOperationException("newLength cannot be null when resized is true."); | |
| } | |
| bundle.SetLength(newLength.Value); |
Copilot uses AI. Check for mistakes.
| size += sizeof(BlobMagic); // Signature blob Magic number | ||
| size += sizeof(uint); // Size field | ||
| size += sizeof(uint); // Blob count | ||
| size += sizeof(BlobIndex) * embeddedSignatureSubBlobCount; // EmbeddedSignature sub-blobs |
There was a problem hiding this comment.
There are several large commented-out blocks and legacy code paths; consider removing or refactoring them to keep the codebase clean and maintainable.
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing CI to preserve entitlements in the managed MacOS signer