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.

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

Convert our CryptoKit bindings to use Swift structs at the boundary #102583

Merged
merged 9 commits into from
Jun 4, 2024
Merged

Convert our CryptoKit bindings to use Swift structs at the boundary #102583

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5be8561
Try converting our CryptoKit bindings to use more Swift types.
jkoritzinsky May 22, 2024
bad0a95
Fix build on macos
jkoritzinsky May 22, 2024
d9705c2
Remove target-typed new and other style changes
jkoritzinsky May 22, 2024
e185237
Don't pass down null with UnsafeBufferPointer
jkoritzinsky May 24, 2024
3220d06
Call copyBytes with a pointer because everything else seems to fail s…
jkoritzinsky May 24, 2024
a642184
Make copies and use copyBytes(to: buffer) again.
jkoritzinsky May 28, 2024
f22ddae
Fix generic value types condition
jkoritzinsky May 31, 2024
b13139b
Put SwiftError parameter first to try working around Mono register al…
jkoritzinsky Jun 3, 2024
9fda4d6
Comma
jkoritzinsky Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 23 additions & 12 deletions ...ibraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Aead.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ internal static partial class Interop
{
internal static partial class AppleCrypto
{
private static byte NullSentinel;

// CryptoKit doesn't do well with a null pointer for the buffer data,
vcsjones marked this conversation as resolved.
Show resolved Hide resolved
// so provide a sentinel pointer instead.
private static ref readonly byte GetSwiftRef(ReadOnlySpan<byte> b)
{
return ref (b.Length == 0
? ref NullSentinel
: ref MemoryMarshal.GetReference(b));
}

internal static unsafe void ChaCha20Poly1305Encrypt(
ReadOnlySpan<byte> key,
ReadOnlySpan<byte> nonce,
Expand All @@ -26,10 +37,10 @@ internal static partial class AppleCrypto
{
fixed (byte* keyPtr = key)
fixed (byte* noncePtr = nonce)
fixed (byte* plaintextPtr = plaintext)
fixed (byte* ciphertextPtr = ciphertext)
fixed (byte* plaintextPtr = &GetSwiftRef(plaintext))
fixed (byte* ciphertextPtr = &GetSwiftRef(ciphertext))
fixed (byte* tagPtr = tag)
fixed (byte* aadPtr = aad)
fixed (byte* aadPtr = &GetSwiftRef(aad))
{
AppleCryptoNative_ChaCha20Poly1305Encrypt(
new UnsafeBufferPointer<byte>(keyPtr, key.Length),
Expand Down Expand Up @@ -59,10 +70,10 @@ internal static partial class AppleCrypto
{
fixed (byte* keyPtr = key)
fixed (byte* noncePtr = nonce)
fixed (byte* ciphertextPtr = ciphertext)
fixed (byte* ciphertextPtr = &GetSwiftRef(ciphertext))
fixed (byte* tagPtr = tag)
fixed (byte* plaintextPtr = plaintext)
fixed (byte* aadPtr = aad)
fixed (byte* plaintextPtr = &GetSwiftRef(plaintext))
fixed (byte* aadPtr = &GetSwiftRef(aad))
{
AppleCryptoNative_ChaCha20Poly1305Decrypt(
new UnsafeBufferPointer<byte>(keyPtr, key.Length),
Expand Down Expand Up @@ -99,10 +110,10 @@ internal static partial class AppleCrypto
{
fixed (byte* keyPtr = key)
fixed (byte* noncePtr = nonce)
fixed (byte* plaintextPtr = plaintext)
fixed (byte* ciphertextPtr = ciphertext)
fixed (byte* plaintextPtr = &GetSwiftRef(plaintext))
fixed (byte* ciphertextPtr = &GetSwiftRef(ciphertext))
fixed (byte* tagPtr = tag)
fixed (byte* aadPtr = aad)
fixed (byte* aadPtr = &GetSwiftRef(aad))
{
AppleCryptoNative_AesGcmEncrypt(
new UnsafeBufferPointer<byte>(keyPtr, key.Length),
Expand Down Expand Up @@ -132,10 +143,10 @@ internal static partial class AppleCrypto
{
fixed (byte* keyPtr = key)
fixed (byte* noncePtr = nonce)
fixed (byte* ciphertextPtr = ciphertext)
fixed (byte* ciphertextPtr = &GetSwiftRef(ciphertext))
fixed (byte* tagPtr = tag)
fixed (byte* plaintextPtr = plaintext)
fixed (byte* aadPtr = aad)
fixed (byte* plaintextPtr = &GetSwiftRef(plaintext))
fixed (byte* aadPtr = &GetSwiftRef(aad))
{
AppleCryptoNative_AesGcmDecrypt(
new UnsafeBufferPointer<byte>(keyPtr, key.Length),
Expand Down
2 changes: 1 addition & 1 deletion src/native/libs/System.Security.Cryptography.Native.Apple/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ endif()

add_custom_command(
OUTPUT pal_swiftbindings.o
COMMAND xcrun swiftc -emit-object -static -parse-as-library -runtime-compatibility-version none -sdk ${CMAKE_OSX_SYSROOT} -target ${SWIFT_COMPILER_TARGET} ${CMAKE_CURRENT_SOURCE_DIR}/pal_swiftbindings.swift -o pal_swiftbindings.o
COMMAND xcrun swiftc -emit-object -static -parse-as-library -enable-library-evolution -g -runtime-compatibility-version none -sdk ${CMAKE_OSX_SYSROOT} -target ${SWIFT_COMPILER_TARGET} ${CMAKE_CURRENT_SOURCE_DIR}/pal_swiftbindings.swift -o pal_swiftbindings.o
MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/pal_swiftbindings.swift
COMMENT "Compiling Swift file pal_swiftbindings.swift"
)
Expand Down
2 changes: 2 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native.Apple/pal_swiftbindings.swift
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can see that the protocols and extensions do de-duplicate some code I feel like it has impacted the straight forwardness of what was there before. If the eventual end-game is to get rid of these bindings and just go straight to CryptoKit, I am not sure what this buys us since CryptoKit does not have a protocol between ChaCha and AES-GCM.

I don”t feel intensely strong either way about this, but I am curious what others think, or if there is a benefit I am overlooking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was making the changes, it was easier to (at least in the first pass) deduplicate. I can split it back out now that I've converted everything to the new shapes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we plan to deprecate this wrapper soon, I believe it is not necessary to revert the changes. My perspective is that for security-related use cases like this one, we should avoid adding layers that are not provided by design, as they may introduce complexity and lead to unintended usage patterns.

Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ protocol SealedBoxProtocol {
protocol AEADSymmetricAlgorithm {
associatedtype SealedBox : SealedBoxProtocol

static func seal<Plaintext>(_ plaintext: Plaintext, using key: SymmetricKey, nonce: SealedBox.Nonce?) throws -> SealedBox where Plaintext: DataProtocol
static func seal<Plaintext, AuthenticatedData>(_ plaintext: Plaintext, using key: SymmetricKey, nonce: SealedBox.Nonce?, authenticating additionalData: AuthenticatedData) throws -> SealedBox where Plaintext: DataProtocol, AuthenticatedData: DataProtocol
static func open<AuthenticatedData>(_ sealedBox: SealedBox, using key: SymmetricKey, authenticating additionalData: AuthenticatedData) throws -> Data where AuthenticatedData: DataProtocol
static func open(_ sealedBox: SealedBox, using key: SymmetricKey) throws -> Data
}

extension AES.GCM.Nonce: NonceProtocol {}
Expand Down