Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
akoeplinger merged 14 commits into from
Dec 15, 2025
Merged

Switch Apple's digest functionality to CryptoKit #120953

akoeplinger merged 14 commits into from
Dec 15, 2025

Conversation

@vcsjones
Copy link
Member

@vcsjones vcsjones commented Oct 21, 2025 •
edited
Loading

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:

  1. HMAC will be a follow up.
  2. The goal of this was to make the Swift function interface be a drop in replacement for the C one, calling convention aside. Any changes to the p/invoke shape can / should be a follow up.
  3. This will drop support for iOS / tvOS 12. Per discussion on Consider bumping minimum iOS / tvOS version to 13 #120457, this is "fine" but we should not merge this until it's been explicitly agreed upon that we are actually okay taking a change that will not work on iOS 12.2.

@vcsjones vcsjones self-assigned this Oct 21, 2025
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Oct 21, 2025

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@vcsjones vcsjones requested a review from akoeplinger October 22, 2025 01:50
@vcsjones
Copy link
Member Author

vcsjones commented Oct 22, 2025

@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.

@vcsjones vcsjones closed this Oct 23, 2025
@vcsjones
Copy link
Member Author

vcsjones commented Oct 23, 2025 •
edited
Loading

After thinking about this more, I don't think the copy semantics work the way I an expecting them to.

finalize on CryptoKit hash functions is not mutating, so it seems to work more like a getCurrentHash. But the documentation clearly says "do not do that" and it's also not clear to me if copy of the swift struct is intended to be a different or not.

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.

@vcsjones vcsjones reopened this Oct 23, 2025
@vcsjones
Copy link
Member Author

vcsjones commented Oct 23, 2025 •
edited
Loading

Okay, after doing some more digging and testing - using a let binding on the hash state does duplicate the state the way we intend.

@build-analysis build-analysis bot mentioned this pull request Oct 23, 2025
Closed
@vcsjones vcsjones marked this pull request as ready for review October 24, 2025 02:10
Copilot AI review requested due to automatic review settings October 24, 2025 02:10
Copilot AI reviewed Oct 24, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a 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

@vcsjones
Apply suggestions from code review
1461cae 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@build-analysis build-analysis bot mentioned this pull request Oct 24, 2025
Open
3 tasks
@bartonjs
Copy link
Member

bartonjs commented Oct 24, 2025

Assuming we're good with the min-ver bump, LGTM.

@vcsjones vcsjones dismissed a stale review October 24, 2025 18:53

Invalid review.

@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Oct 24, 2025
@build-analysis build-analysis bot mentioned this pull request Nov 22, 2025
Open
@vcsjones
Copy link
Member Author

vcsjones commented Dec 15, 2025

@akoeplinger are there any other pipelines we should run for confidence in mobile Apple platforms? I am not sure what is included in the "smoke" ones.

@build-analysis build-analysis bot mentioned this pull request Dec 15, 2025
Open
@akoeplinger
Copy link
Member

akoeplinger commented Dec 15, 2025

I am not sure what is included in the "smoke" ones.

Easiest way is to check the Helix workitems, right now it's running mostly System.Runtime.Tests.

@akoeplinger
Copy link
Member

akoeplinger commented Dec 15, 2025

/azp run runtime-ioslikesimulator

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 15, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

akoeplinger commented Dec 15, 2025

runtime-ioslikesimulator is green, merging :)

@akoeplinger akoeplinger merged commit 6ab804e into dotnet:main Dec 15, 2025
110 of 112 checks passed
@vcsjones vcsjones deleted the apple-digest-cryptokit branch December 15, 2025 19:58
@vcsjones vcsjones added this to the 11.0.0 milestone Dec 16, 2025
@vcsjones vcsjones mentioned this pull request Jan 5, 2026
Closed
This was referenced Jan 9, 2026
Closed
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@akoeplinger akoeplinger akoeplinger approved these changes

@bartonjs bartonjs bartonjs approved these changes

Assignees

@vcsjones vcsjones

Labels

area-System.Security cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release.

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@vcsjones @bartonjs @akoeplinger