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

API proposal: ChaCha20Poly1305 cryptographic algorithm #45130

Closed
GrabYourPitchforks opened this issue Nov 23, 2020 · 12 comments
Closed

API proposal: ChaCha20Poly1305 cryptographic algorithm #45130

GrabYourPitchforks opened this issue Nov 23, 2020 · 12 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Nov 23, 2020

Background and Motivation

We've received requests to add more cryptographic primitives to the BCL. Among these requests is support for ChaCha20-Poly1305 (see #16010), an AEAD algorithm standardized in RFC 7539. This algorithm is beginning to see widespread use in networking protocols - notably, for TLS.

We're not considering shipping APIs for standalone ChaCha20 and Poly1305 primitives. This proposal is only for the combined ChaCha20Poly1305 AEAD construction.

Proposed API

Since this is an AEAD algorithm, the proposed API surface exactly mirrors the API surface of System.Security.Cryptography.AesGcm. We're not using the existing SymmetricAlgorithm class, as it's not well-suited for AEAD algorithms.

namespace System.Security.Cryptography
{
    [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
    public sealed partial class ChaCha20Poly1305 : System.IDisposable
    {
        public ChaCha20Poly1305(byte[] key) { }
        public ChaCha20Poly1305(System.ReadOnlySpan<byte> key) { }
        // public static System.Security.Cryptography.KeySizes NonceByteSizes { get { throw null; } } // (removed - see discussion)
        // public static System.Security.Cryptography.KeySizes TagByteSizes { get { throw null; } } // (removed - see discussion)
        public void Decrypt(byte[] nonce, byte[] ciphertext, byte[] tag, byte[] plaintext, byte[]? associatedData = null) { }
        public void Decrypt(System.ReadOnlySpan<byte> nonce, System.ReadOnlySpan<byte> ciphertext, System.ReadOnlySpan<byte> tag, System.Span<byte> plaintext, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { }
        public void Dispose() { }
        public void Encrypt(byte[] nonce, byte[] plaintext, byte[] ciphertext, byte[] tag, byte[]? associatedData = null) { }
        public void Encrypt(System.ReadOnlySpan<byte> nonce, System.ReadOnlySpan<byte> plaintext, System.Span<byte> ciphertext, System.Span<byte> tag, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { }
    }
}

The key, nonce, and tag lengths are fixed at 256, 96, and 128 bits, respectively. Callers are expected to know this information. We could consider getting rid of the NonceByteSizes and TagByteSizes properties here, which were copied over from the AesGcm approved API surface.

Implementation and usage notes

We are not shipping an implementation of this algorithm. We rely on the underlying OS or a third-party crypto library to provide the implementation on our behalf. The support matrix is then:

  • Windows 10: Build 20142 or later.
  • OpenSSL: Version 1.1 or later.
  • Mac OSX: Unsupported unless the OpenSSL shim is enabled, same as AES-GCM.
  • Browser: Unsupported.

Open questions

Do we need to attribute the type with "if on Windows, supported in 20142+ only"? I don't know how useful this would be, since normally some high-level orchestration layer or config dictates what algorithms to use, and the low-level code which would be using this proposed type would be far-removed from any orchestration code. That low-level code normally wouldn't be responsible for checking support, as any PNSE that's thrown would indicate a failure on the part of the orchestration layer, not the component that attempted to create a ChaCha20Poly1305 instance.

@GrabYourPitchforks GrabYourPitchforks added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Nov 23, 2020
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone Nov 23, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 23, 2020
@ghost
Copy link

ghost commented Nov 23, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

We've received requests to add more cryptographic primitives to the BCL. Among these requests is support for ChaCha20-Poly1305 (see #16010), an AEAD algorithm standardized in RFC 7539. This algorithm is beginning to see widespread use in networking protocols - notably, for TLS.

Proposed API

Since this is an AEAD algorithm, the proposed API surface exactly mirrors the API surface of System.Security.Cryptography.AesGcm. We're not using the existing SymmetricAlgorithm class, as it's not well-suited for AEAD algorithms.

    [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")]
    public sealed partial class ChaCha20Poly1305 : System.IDisposable
    {
        public ChaCha20Poly1305(byte[] key) { }
        public ChaCha20Poly1305(System.ReadOnlySpan<byte> key) { }
        public static System.Security.Cryptography.KeySizes NonceByteSizes { get { throw null; } }
        public static System.Security.Cryptography.KeySizes TagByteSizes { get { throw null; } }
        public void Decrypt(byte[] nonce, byte[] ciphertext, byte[] tag, byte[] plaintext, byte[]? associatedData = null) { }
        public void Decrypt(System.ReadOnlySpan<byte> nonce, System.ReadOnlySpan<byte> ciphertext, System.ReadOnlySpan<byte> tag, System.Span<byte> plaintext, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { }
        public void Dispose() { }
        public void Encrypt(byte[] nonce, byte[] plaintext, byte[] ciphertext, byte[] tag, byte[]? associatedData = null) { }
        public void Encrypt(System.ReadOnlySpan<byte> nonce, System.ReadOnlySpan<byte> plaintext, System.Span<byte> ciphertext, System.Span<byte> tag, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { }
    }

The key, nonce, and tag lengths are fixed at 256, 96, and 128 bits, respectively. Callers are expected to know this information. We could consider getting rid of the NonceByteSizes and TagByteSizes properties here, which were copied over from the AesGcm approved API surface.

Implementation and usage notes

We are not shipping an implementation of this algorithm. We rely on the underlying OS or a third-party crypto library to provide the implementation on our behalf. The support matrix is then:

  • Windows 10: Build 20142 or later.
  • OpenSSL: Version 1.1 or later.
  • Mac OSX: Unsupported unless the OpenSSL shim is enabled, same as AES-GCM.
  • Browser: Unsupported.

Open questions

Do we need to attribute the type with "if on Windows, supported in 20142+ only"? I don't know how useful this would be, since normally some high-level orchestration layer or config dictates what algorithms to use, and the low-level code which would be using this proposed type would be far-removed from any orchestration code. That low-level code normally wouldn't be responsible for checking support, as any PNSE that's thrown would indicate a failure on the part of the orchestration layer, not the component that attempted to create a ChaCha20Poly1305 instance.

Author: GrabYourPitchforks
Assignees: -
Labels:

api-suggestion, area-System.Security

Milestone: 6.0.0

@bartonjs
Copy link
Member

We could consider getting rid of the NonceByteSizes and TagByteSizes properties here

For AES-GCM and AES-CCM the specifications allow for multiple sizes, so the properties make sense... and the implementations don't necessarily allow for the full range the spec does, so they make even more sense.

Since ChaCha20-Poly1305 does restrict the key, nonce, and tag sizes to only single values, any caller would immediately get the feedback that they tried using the algorithm wrong. Therefore, I agree that removing the properties is justified... and if any future spec/standard enables varying those values we don't know now that they'll even be something that can be expressed in a single KeySizes value (it might require something more complicated)... so having it now is even a liability.

Do we need to attribute the type with "if on Windows, supported in 20142+ only"?

I don't have a good feeling here. If we expanded EnvelopedCms to use it, for example, we'd not paint the class as having problems, since it's a data-dependent problem. If I remember how we defined the attributes correctly, saying that it's unsupported on low-Windows ranges means that you'd only get the warning if specifically compiling for a Windows TFM-variant... which makes it not very useful at this layer... so maybe it's not worth painting.

@am11
Copy link
Member

am11 commented Nov 23, 2020

Just curious, why is the Browser unsupported? ChaCha20Poly1305 seems to be supported in most modern browsers: https://caniuse.com/chacha20-poly1305. 🤔

@vcsjones
Copy link
Member

Just curious, why is the Browser unsupported? ChaCha20Poly1305 seems to be supported in most modern browsers

That's in the context of a TLS cipher suite. To my knowledge, no browser supports ChaChaPoly1305 as a standalone cryptographic primitive in subtle crypto.

@bartonjs
Copy link
Member

To my knowledge, no browser supports ChaChaPoly1305 as a standalone cryptographic primitive in subtle crypto.

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto doesn't even mention the algorithm

@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 24, 2020
@GrabYourPitchforks
Copy link
Member Author

I've tentatively marked this "ready for review". Review sessions will take a hiatus through the end of the year. That gives some time for people to chime in on this if they care. And if something comes up in the comments during the next few weeks we can mark this "not ready" as needed.

@vcsjones
Copy link
Member

The support matrix is then

Since there appears to be ongoing work to use Android's native functionality for AES-GCM/CCM instead of OpenSSL, what would this mean for Android support? I see that very recent versions of Java support ChaCha20-Poly1305 in Cipher.getInstance, but no idea what that maps to in supported / shipped versions of Android.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 12, 2021
@terrajobst
Copy link
Member

terrajobst commented Jan 12, 2021

Video

  • We should remove the attribute and tell people they need to call an IsSupported
    • We should add an IsSupported property to AesGcm and AesCcm as well.
namespace System.Security.Cryptography
{
    public sealed partial class ChaCha20Poly1305 : IDisposable
    {
        public static bool IsSupported { get; }
        public ChaCha20Poly1305(byte[] key);
        public ChaCha20Poly1305(ReadOnlySpan<byte> key);
        public void Decrypt(byte[] nonce, byte[] ciphertext, byte[] tag, byte[] plaintext, byte[]? associatedData = null);
        public void Decrypt(ReadOnlySpan<byte> nonce, ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> tag, Span<byte> plaintext, ReadOnlySpan<byte> associatedData = default(ReadOnlySpan<byte>));
        public void Dispose();
        public void Encrypt(byte[] nonce, byte[] plaintext, byte[] ciphertext, byte[] tag, byte[]? associatedData = null);
        public void Encrypt(ReadOnlySpan<byte> nonce, ReadOnlySpan<byte> plaintext, Span<byte> ciphertext, Span<byte> tag, ReadOnlySpan<byte> associatedData = default(ReadOnlySpan<byte>));
    }
}

@GrabYourPitchforks GrabYourPitchforks removed the untriaged New issue has not been triaged by the area owner label Jan 12, 2021
@am11
Copy link
Member

am11 commented Jan 12, 2021

Mac OSX: Unsupported unless the OpenSSL shim is enabled, same as AES-GCM.

Looks like cryptokit for macOS 10.15+ also has ChaCha20-Poly1305 cipher. We would probably still need the shim fallback for older macs.

@vcsjones
Copy link
Member

Looks like cryptokit for macOS 10.15+ also has ChaCha20-Poly1305 cipher

Calling CryptoKit is.. not straight forward. It has no C bindings. Only Swift. It's possible to make this work, I've experimented with this in #29811. The biggest issue is getting all of the tooling in the right place. Since there is renewed interest in CryptoKit for macOS with this issue, the linked issue, and 25519 support in others issues, I may as well try to cobble together something.

@terrajobst
Copy link
Member

I may as well try to cobble together something.

Famous last words. Next you know you're the community leader for generating C# bindings for Swift APIs.

@GrabYourPitchforks
Copy link
Member Author

This was resolved by #52030.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

No branches or pull requests

6 participants