-
Notifications
You must be signed in to change notification settings - Fork 847
Explicitly enable FullAssemblySigningSupported for non-Windows builds #19236
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
Conversation
Ensures that F# maintains assembly signing on Linux/macOS by explicitly opting in. This prevents potential breaks in repo-specific workflows (like bootstrapping) before the global Arcade SDK default is changed to 'off' for non-Windows platforms. Relates to dotnet/runtime#123010.
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
Directory.Build.props
Outdated
| All settings below can be overridden via CLI switches if needed. --> | ||
|
|
||
| <PropertyGroup> | ||
| <FullAssemblySigningSupported Condition="'$(OS)' != 'Windows_NT'">true</FullAssemblySigningSupported> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this is the right change.
Instead, it should be a fix in the F# compiler, so that project listed in the repro steps of #17451 compiles fine on RH with locked down crypto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, @jkotas. You're right—fixing the compiler logic to handle locked-down crypto gracefully is a much cleaner approach than a property override.
I’ve been looking into src/Compiler/AbstractIL/ilwrite.fs as we discussed earlier. It seems the issue arises when StrongNameSignatureSize is called.
Could you provide some guidance on the preferred way to fix this in the compiler? Should we ensure it skips the SHA-1 dependent signing calls entirely when certain conditions are met, or is there a specific abstraction in the F# codebase I should leverage for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation in F# compiler should mirror the implementation in Roslyn. Check https://github.com/dotnet/roslyn/blob/main/src/Compilers/Core/Portable/PEWriter/SigningUtilities.cs and the callers of these methods.
… on restricted crypto systems
Decoupled signer creation from CreateILModule to support modern RSA-based signature calculations, following the logic used in Microsoft.CodeAnalysis.
…peline Updated AbstractIL.StrongNameSign.fsi by replacing legacy Open* methods with unified CreatePublicSigner and CreateKeyPairSigner factories. This aligns the interface with the modern Roslyn-based signing approach and prepares for improved RSA signature size calculations.
- Update SignatureSize calculation in ILStrongNameSigner to match Roslyn's logic. - Use (keySize - 32) for public/delay signing to prevent PE file corruption. - Maintain legacy signature size for full key-pair signing to ensure backward compatibility.
* Fix strong name signature size to align with Roslyn for public signing, addressing issues on Linux distributions with restricted crypto. ([PR dotnet#19236](dotnet#19236))
…x Windows bootstrap failure
|
Closing this PR in favor of #19242. Apologies for the noise here—I was initially focused on a build-property workaround. Following the guidance from @jkotas, I have now implemented the fix directly in the compiler's signing logic to ensure cross-platform compatibility, mirroring Roslyn's behavior. All consolidated changes and the verified fix can be found in the new PR. Thanks for the patience and the guidance! |
Summary
Following the discussion in dotnet/runtime#123401, this PR explicitly opts-in to assembly signing for the F# repository on non-Windows platforms.
Context
We are planning a global change in dotnet/arcade to disable full assembly signing by default on non-Windows systems to address RSA+SHA-1 signing issues on modern Linux distributions (e.g., RHEL 9/10).
As pointed out by @jkotas, F# requires strong-name signing for its bootstrapping process and repo-specific workflows. This change ensures that F# remains stable and continues to use the OpenSSL-based signing logic provided by Arcade, even after the global default is switched to off.
Changes
Modified Directory.Build.props to set FullAssemblySigningSupported to true when the OS is not Windows.
This acts as a local override that takes precedence over the upcoming Arcade SDK global defaults.
Related Issues
Foundational work for dotnet/runtime#123010
Referenced in dotnet/runtime#123401
Addresses concerns in #17451
/cc @jkotas @jkoritzinsky