-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Switch Apple's digest functionality to CryptoKit #120953
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Outdated
Show resolved
Hide resolved
|
@akoeplinger added you as a reviewer largely for awareness and to better understand if we are ready to take a "break iOS 12" change for NET 11 or if we should sit on this longer. |
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Outdated
Show resolved
Hide resolved
|
After thinking about this more, I don't think the copy semantics work the way I an expecting them to.
I think for SHA-2 we should stick with CommonCrypto to make clone and current work. For SHA-3 we can implement with CryptoKit, but we will probably need to omit GetCurrentDigest and Clone. At least until CryptoKit has a clear way to "fork" a hash in a documented way. |
|
Okay, after doing some more digging and testing - using a |
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.
Pull Request Overview
This PR migrates Apple platform's digest/hashing implementation from CommonCrypto to CryptoKit, positioning the codebase for future SHA-3 support while dropping support for iOS/tvOS 12.
Key Changes:
- Replaces C-based CommonCrypto digest implementation with Swift-based CryptoKit implementation
- Updates P/Invoke signatures to use Swift calling conventions
- Implements digest operations (one-shot, create, update, final, reset, clone, current) using CryptoKit's HashFunction protocol
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pal_swiftbindings.swift | Adds HashBox wrapper class and implements all digest functions using CryptoKit hash types (MD5, SHA1, SHA256, SHA384, SHA512) |
| pal_swiftbindings.h | Adds extern C declarations for the new Swift-implemented digest functions |
| pal_digest.h | Removes function declarations that are now implemented in Swift |
| pal_digest.c | Removes entire C implementation of digest functions based on CommonCrypto |
| CMakeLists.txt | Removes pal_digest.c from build sources |
| Interop.Digest.cs | Adds Swift calling convention attributes to P/Invoke declarations |
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
Show resolved
Hide resolved
...libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Digest.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
|
Assuming we're good with the min-ver bump, LGTM. |
This changes our implementation of hashing to be based on CryptoKit instead of CommonCrypto. This will set us up on a path for success to eventually add handling for SHA-3.
A couple of notes on implementation: