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

Implement AES-GCM with CryptoKit on macOS #76490

Merged
merged 10 commits into from
Nov 8, 2022

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Oct 1, 2022

This implements AES-GCM using CryptoKit on macOS.

As discussed below, this is also a breaking change. While macOS previously supported encrypting and decrypting with "short" authentication tags when using OpenSSL, CryptoKit only supports 128-bit tags.

Closes #29811

@ghost
Copy link

ghost commented Oct 1, 2022

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

Issue Details

This implements AES-GCM using CryptoKit on macOS.

Unlike ChaCha20Poly1305, CryptoKit has Opinions™ about the size of the authentication tag, namely that the authentication tag must be 16 bytes. Because .NET previously supported short authentication tags with OpenSSL, we will continue to fallback to OpenSSL if the authentication tag is short.

However, CryptoKit is the preferred mechanism for AES-GCM. I considered having CryptoKit as the fallback if OpenSSL is not available, but that would mean that CryptoKit would never get exercised in CI since CI always has OpenSSL, and my general believe that the primary platform's implementation should be preferred.

Closes #29811

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones vcsjones added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 7, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 7, 2022
@ghost
Copy link

ghost commented Nov 7, 2022

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 8, 2022

@bartonjs I haven't been able to get these 3 legs to pass after a few retries. It looks like they are timeouts being tracked in #76454. Do you concur this is okay to merge given the macOS runs are green?

@bartonjs
Copy link
Member

bartonjs commented Nov 8, 2022

I agree that the failures don't look related (and that there's not really a sane universe in which they could be).

@bartonjs bartonjs merged commit e029499 into dotnet:main Nov 8, 2022
@vcsjones vcsjones deleted the aes-gcm-crypto-kit branch November 8, 2022 20:24
@vcsjones
Copy link
Member Author

vcsjones commented Nov 8, 2022

Breaking change doc: dotnet/docs#32346

@vcsjones vcsjones removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 8, 2022
@MartyIX
Copy link
Contributor

MartyIX commented Nov 25, 2022

Any chance of this being backported to .NET 7, please?

@vcsjones
Copy link
Member Author

Any chance of this being backported to .NET 7, please?

Almost certainly not for a couple of reasons. The first of which is that this is a breaking change. Breaking changes in servicing releases in my experience won't be accepted unless there is a very good reason to do so, such an API not doing what it is documented to do or causing customer harm.

The second of which is that this change is dependent on #76317. That change makes changes to how native code is built for macOS, most notably by bumping the native macOS toolchain. That also feels inappropriate to do in a servicing release.

But thanks for asking and I am glad (it seems) that this change will be useful for you in .NET 8.

@MartyIX
Copy link
Contributor

MartyIX commented Nov 25, 2022

But thanks for asking and I am glad (it seems) that this change will be useful for you in .NET 8.

Yeah, it's gonna be great to have this :) Thank you for adding this.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit AES-GCM on macOS
4 participants