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.

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

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

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

jkoritzinsky
Copy link
Member

Now that we have support for frozen structs in the Swift calling convention support in the VM, use that support to reimplement the CryptoKit PAL using basic Swift types that implement the necessary protocols for CryptoKit APIs (Unsafe{Mutable}BufferPointer<T>) at the PAL boundary.

Also refactor our CryptoKit bindings to reduce duplication.

This PR represents us reaching Phase 2 of our .NET 9 Swift Interop project (using all of the JIT calling convention support, minimal projection support, still using a Swift wrapper).

Copy link
Contributor

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

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.

Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM!

…ome assert for some reason I can't figure out.
@jkoritzinsky
Copy link
Member Author

After looking at the Mono failure, this looks like the remaining failure is a bug in the Mono MiniJIT support for the Swift calling convention on x64. @matouskozak can you take a look?

I added a print statement in encrypt to dump the key and the buffer size I got was way too big (123145356760544), so I think there was some corruption in the interop layer/incorrect register assignments.

My LLDB is crashing locally, so I'm having trouble getting much further.

@kotlarmilos
Copy link
Member

kotlarmilos commented May 30, 2024

The Unsafe{Mutable}BufferPointer structs are not lowered at the interop boundary due to the following condition in the

// Non-value types are illegal at the interop boundary.
if (type->type == MONO_TYPE_GENERICINST && !mono_type_generic_inst_is_valuetype (type)) {
lowering.by_reference = TRUE;
return lowering;
} else if (type->type != MONO_TYPE_VALUETYPE && !mono_type_is_primitive(type)) {
lowering.by_reference = TRUE;
return lowering;
}

The parameter is of type MONO_TYPE_GENERICINST, and mono_type_is_primitive returns FALSE. Here is one suggestion to address this:

if (!(type->type == MONO_TYPE_GENERICINST && mono_type_generic_inst_is_valuetype (type))
    && type->type != MONO_TYPE_VALUETYPE
    && !mono_type_is_primitive (type))) {
        lowering.by_reference = TRUE;
        return lowering;
    }

@jkoritzinsky
Copy link
Member Author

The next problem looks like some sort of corruption in the caller function (somehow we're ending up with a non-zero-length null span). Any other ideas? Maybe an issue with SwiftError* with this many parameters?

@jkoritzinsky
Copy link
Member Author

Looks like @kotlarmilos's suggested workaround was successful.

Failures are unrelated, so I'll merge this in.

@jkoritzinsky
Copy link
Member Author

/ba-g wasm timeouts unrelated (CryptoKit is apple-only)

@jkoritzinsky jkoritzinsky merged commit 43dbffa into dotnet:main Jun 4, 2024
107 of 119 checks passed
@jkoritzinsky jkoritzinsky deleted the cryptokit-phase-2 branch June 4, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants