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]: Add support for Ed25519 #63174

Open
annerajb opened this issue Dec 28, 2021 · 46 comments
Open

[API Proposal]: Add support for Ed25519 #63174

annerajb opened this issue Dec 28, 2021 · 46 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@annerajb
Copy link

annerajb commented Dec 28, 2021

Background and motivation

Background

I work a lot with Ed25519 and love dotnet to do development, Last year I reported a issue trying to use Ed25519 and the TLS 1.3 integration wit openssl on SslStream did not support Ed25519 certificates and keys. See here

I hope this proposal is more complete and actionable to get any process started.

Traction / usage

This has been requested for a while now or will be used more in the future.

  • This was set forth as a proposal all the way back in 2015 here
  • This will now be included on the upcoming FIPS 186-5 standard allowing it's usage on FIPS enabled systems.
  • It's also used in SSH keys and becoming more and more common making it beneficial for the dotnet ecosystem as a whole to support this suite of algorithms.
  • DNSSec with RFC 8080 adds ed448 and ed25519 as part of the signing.

Reference / resources:

DNS Sec ed25519
RFC 8080: Edwards-Curve Digital Security Algorithm (EdDSA) for DNSSEC
RFC 8032: Edwards-Curve Digital Signature Algorithm (EdDSA)
RFC 8410: Algorithm Identifiers for Ed25519, Ed448, X25519, and X448
for Use in the Internet X.509 Public Key Infrastructure

API Proposal

Ed25519 is a specific set of options from Edwards DSA (EdDSA).

Due to lack of general support for EdDSA from underlying stacks, this proposal ignores the general algorithm, and focuses on this specific variant.

namespace System.Security.Cryptography
{
    public abstract partial class Ed25519 : AsymmetricAlgorithm
    {
        public const int SignatureSizeInBytes = 64;

        // This will return `false` if Create() will throw, `true` if it will succeed.
        // Even though the type could be 3rd-party derived, the static bool is just reporting
        // what Create() will do.
        //
        // [does this need positive-platform assertions to balance against Create?]
        [UnsupportedOSPlatformGuard("android")]
        [UnsupportedOSPlatformGuard("browser")]
        [UnsupportedOSPlatformGuard("ios")]
        [UnsupportedOSPlatformGuard("tvos")]
        [UnsupportedOSPlatformGuard("windows")]
        public static bool IsSupported { get; }

        public abstract bool HasPrivateKey { get; }

        protected Ed25519() {}

        [UnsupportedOSPlatform("android")] // (might be possible to support).
        [UnsupportedOSPlatform("browser")]
        [UnsupportedOSPlatform("ios")]
        [UnsupportedOSPlatform("tvos")]
        [UnsupportedOSPlatform("windows")]
        public static new Ed25519 Create();
        
        // Exports the private key value as defined by IETF RFC 8032.
        // Not to be confused with ExportPkcs8PrivateKey or ExportEncryptedPkcs8PrivateKey
        // (Alternate candidate: ExportRfc8032PrivateKey ?)
        public byte[] ExportPrivateKey();
        public int ExportPrivateKey(Span<byte> destination);
        public bool TryExportPrivateKey(Span<byte> destination, out int bytesWritten);
        protected abstract int ExportPrivateKeyCore(Span<byte> destination);

        // Exports the public key value as defined by IETF RFC 8032.
        // Not to be confused with ExportSubjectPublicKeyInfo
        // (Alternate candidate: ExportRfc8032PublicKey ?)
        public byte[] ExportPublicKey();
        public int ExportPublicKey(Span<byte> destination);
        public bool TryExportPublicKey(Span<byte> destination, out int bytesWritten);
        protected abstract int ExportPublicKeyCore(Span<byte> destination);

        public abstract void GenerateKey();

        public void ImportPrivateKey(byte[] privateKey) { }
        public void ImportPrivateKey(ReadOnlySpan<byte> privateKey) { }
        protected abstract void ImportPrivateKeyCore(ReadOnlySpan<byte> privateKey);

        public void ImportPublicKey(byte[] publicKey) { }
        public void ImportPublicKey(ReadOnlySpan<byte> publicKey) { }
        protected abstract void ImportPublicKeyCore(ReadOnlySpan<byte> publicKey);

        // RSA/DSA/ECDsa all have both SignData and SignHash. (And VerifyData/VerifyHash)
        // Due to the different algorithm design of Ed25519, SignHash/VerifyHash is not an option
        public byte[] SignData(byte[] data);
        public byte[] SignData(ReadOnlySpan<byte> data);
        public int SignData(ReadOnlySpan<byte> data, Span<byte> destination);
        public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, out int bytesWritten);
        protected abstract int SignDataCore(ReadOnlySpan<byte> data, Span<byte> destination);

        public bool VerifyData(byte[] data, byte[] signature);
        public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature);
        protected abstract bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature);

        // Not shown: All of the overrides from AsymmetricAlgorithm
    }

    // A public type for working with OpenSSL EVP_PKEY* values.
    public sealed partial class Ed25519OpenSsl : Ed25519
    {
        public static new bool IsSupported { get; }

        [UnsupportedOSPlatform("android")]
        [UnsupportedOSPlatform("browser")]
        [UnsupportedOSPlatform("ios")]
        [UnsupportedOSPlatform("tvos")]
        [UnsupportedOSPlatform("windows")]
        public Ed25519OpenSsl(SafeEvpPKeyHandle pkeyHandle);

        [UnsupportedOSPlatform("android")]
        [UnsupportedOSPlatform("browser")]
        [UnsupportedOSPlatform("ios")]
        [UnsupportedOSPlatform("tvos")]
        [UnsupportedOSPlatform("windows")]
        public Ed25519OpenSsl();

        // Not shown: All of the overrides from AsymmetricAlgorithm or Ed25519
    }
}

namespace System.Security.Cryptography.X509Certificates
{
    public sealed partial class PublicKey
    {
        public Ed25519? GetEd25519PublicKey();
    }

    public partial class X509Certificate2
    {
        public Ed25519? GetEd25519PublicKey();
        public Ed25519? GetEd25519PrivateKey();
        public X509Certificate2 CopyWithPrivateKey(Ed25519 privateKey);
    }
}
@annerajb annerajb added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 28, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Dec 28, 2021
@ghost
Copy link

ghost commented Dec 28, 2021

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

Issue Details

Background and motivation

Background

I work a lot with Ed25519 and love dotnet to do development, Last year I reported a issue trying to use Ed25519 and the TLS 1.3 integration wit openssl on SslStream did not support Ed25519 certificates and keys. See here

I hope this proposal is more complete and actionable to get any process started.

traction / usage

This has been requested for a while now or will be used more in the future.

  • This was set forth as a proposal all the way back in 2015 here
  • This will now be included on the upcoming FIPS 186-5 standard allowing it's usage on FIPS enabled systems.
  • It's also used in SSH keys and becoming more and more common making it beneficial for the dotnet ecosystem as a whole to support this suite of algorithms.

summary of assembly with public API changes

  • System.Security.Cryptography.X509Certificates
  • System.Security.Cryptography.OpenSsl
  • System.Security.Cryptography

platforms

This would only support platforms where Openssl 1.1.1 is used by dotnet (right now afaik Linux only)
This would not support Windows since the Windows CNG.
This would not support MacOS since afaik only CryptKit thru swift supports it.

Todos

  • What should the casing / naming be? EdDsa or EDDsa?
  • For reference RFC 8410 mentions the human readable names for the algorithms
  • I need to create the Ed25519 derived class sample tho apart from naming it woudn't differ from the EdDsa one. This class implements a specific parameter choice such as Ed25519 and the EdDsa class itself should require inheritance.

Reference / resources:

RFC 8032: Edwards-Curve Digital Signature Algorithm (EdDSA)
RFC 8410: Algorithm Identifiers for Ed25519, Ed448, X25519, and X448
for Use in the Internet X.509 Public Key Infrastructure

API Proposal

namespace System.Security.Cryptography
{
    public partial struct EDDsaParameters
    {
        public ECCurveType Curve;
        //enum to say if it's a Ed25519 or Ed448 or w/e comes out next?
        public enum ECCurveType
        {
            Ed25519 = 0,
        }
        //opaque / raw 32 bytes private key
        public byte[]? PrivateKey;
        //opaque / raw 32 bytes of public key (if private is set we can derive public from it)
        public byte[]? PublicKey;
    }
    //Provides an abstract base class that encapsulates the Edwards-curve Digital Signature Algorithm (EdDSA).
    public abstract partial class EDDsa : AsymmetricAlgorithm
    {
        protected EDDsa();
        public override string SignatureAlgorithm;
        public static new EDDsa Create();
        //creates a eddsa with the specific public / private key
        public static EDDsa Create(EDDsaParameters parameters);
        //ImportFromEncryptedPem calls ImportEncryptedPkcs8PrivateKey
        public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
        public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, ReadOnlySpan<byte> source, out int bytesRead);
        public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
        public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
        //ExportEncryptedPkcs8PrivateKey calls TryExportEncryptedPkcs8PrivateKey
        public override bool TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);
        public override bool TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);
        //ExportPkcs8PrivateKey calls TryExportPkcs8PrivateKey
        public override bool TryExportPkcs8PrivateKey(Span<byte> destination, out int bytesWritten);
        //ExportSubjectPublicKeyInfo calls TryExportSubjectPublicKeyInfo
        public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
        //---------------------------------
        public abstract void ImportParameters(EDDsaParameters parameters);
        public abstract EDDsaParameters ExportParameters(bool includePrivateParameters);
        //-------- sign and verify
        public abstract byte[] SignData(byte[] data);
        public virtual byte[] SignData(byte[] data, int offset, int count);
        public virtual bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, out int bytesWritten);
        public abstract bool VerifyData(byte[] data, byte[] signature);
        public virtual bool VerifyData(byte[] data, int offset, int count, byte[] signature);
        public virtual bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature);
    }
    //should it be EdDsaOpenSsl or EDDsaOpenSsl
    public sealed partial class EDDsaOpenSsl : EDDsa
    {
        public EDDsaOpenSsl() { }
        public EDDsaOpenSsl(System.IntPtr handle) { }
        public EDDsaOpenSsl(System.Security.Cryptography.EDDsaParameters parameters) { }
        public EDDsaOpenSsl(System.Security.Cryptography.SafeEvpPKeyHandle pkeyHandle) { }
        protected override void Dispose(bool disposing);
        public SafeEvpPKeyHandle DuplicateKeyHandle();
        public override EDDsaParameters ExportParameters(bool includePrivateParameters);
        public override void ImportParameters(EDDsaParameters parameters);
        public override byte[] SignData(byte[] data);
        public override bool VerifyData(byte[] data, byte[] signature);
    }
}
namespace System.Security.Cryptography.X509Certificates
{
    class X509Certificate
    {
        public sealed partial class PublicKey
        {
            public EDDsa? GetEDDsaPublicKey();
        }
        //should it be EdDsaCertificateExtensions
        public static partial class EDDsaCertificateExtensions
        {
            public static X509Certificate2 CopyWithPrivateKey(this X509Certificate2 certificate, EDDsa privateKey);
            //should it be  GetEdDsaPrivateKey and GetEdDsaPublicKey
            public static EDDsa? GetEDDsaPrivateKey(this X509Certificate2 certificate);
            public static EDDsa? GetEDDsaPublicKey(this X509Certificate2 certificate);
        }
    }
}

API Usage

Not sure if I am doing this section correctly but expected usages are:

//load a ed25519 certificate or chain that can be passed to SslStream
var server_tls_cert = new X509Certificate2("server_chain25519.pfx", "qwerty");
var server_tls_cert = new X509Certificate2("server_tls.pem", "server.key");
// generate a new 25519 key
var ed25519 = EDDsaOpenSsl.Create("Ed25519"); 
//sign and verify with that key
var signature = ed25519.SignData("Hello");
Assert.IsTrue(ed25519.VerifyData("Hello",signature));
//load a existing public or private key
var existingkey = new EDDsaParameters() 
{ 
  Curve = Ed25519,
  PrivateKey = Convert.FromBase64String("CsOkD5npRC16IZ0NGue/oVBNixzwQbIKGNbDNS10w/g=")
};

var ed25519Loaded = EDDsaOpenSsl.Create(existingkey);
ed25519Loaded.SignData("Hello");
SslStream sslStream = new SslStream(client.GetStream(), false);
var server_tls_cert = new X509Certificate2("server_chain25519.pem", "key");
sslStream.AuthenticateAsServer(server_tls_cert , clientCertificateRequired: false, checkCertificateRevocation: true);

Alternative Designs

One option but sounded awful... was instead of making a EdDsa class making just a OpensslRawPrivateKey class

There are multiple algorithms that openssl supports that are opaque / raw and shared roughly the same set of functions.

  • EVP_PKEY_HMAC
  • EVP_PKEY_POLY1305
  • EVP_PKEY_SIPHASH
  • EVP_PKEY_X25519
  • EVP_PKEY_ED25519
  • EVP_PKEY_X448
  • EVP_PKEY_ED448

The downside being that if implemented as only a OpensslRaw class it would not support X509Certificates or SslStream

In addition it didn't seem like good API design since they are different algorithms / functions and is not extensible / usable by other platforms that would implement support for this.

Risks

One risk that unfortunately I can't say is that the change may not cover all areas of dotnet where AssymetricAlgorithms are used and leave places where even though it uses a AssymetricAlgorithm or a X509Certificate it would not support Ed25519 keys using this class.

one example of this would be SslStream wouldn't work if this is not updated to support it

Author: annerajb
Assignees: -
Labels:

api-suggestion, area-System.Security, untriaged

Milestone: -

@hamarb123
Copy link
Contributor

hamarb123 commented Dec 28, 2021

This would not support MacOS since afaik only CryptKit thru swift supports it.

I'm certainly not an expert on cryptography, but I do know about macOS interop:
You could always write an obj-c wrapper around CryptoKit and then use that in .NET through P/Invoke (wrapper could even be exported as c functions instead of obj-c functions for easier P/Invoke), this would probably not be too much effort for 1 set of APIs - https://stackoverflow.com/questions/65686717/can-cryptokit-be-used-with-objective-c, xamarin/xamarin-macios#6239. Then again, it may be better to wait for xamarin-macios's swift binding library solution, but I'm sure it would essentially be an easy drop-in if we were to change.

@vcsjones
Copy link
Member

vcsjones commented Dec 28, 2021

You could always write an obj-c wrapper around CryptoKit and then use that in .NET through P/Invoke (wrapper could even be exported as c functions instead of obj-c functions for easier P/Invoke)

We've had many discussions about CryptoKit (#52482 (comment), #29811) and it's not entirely straight forward.

Even if CryptoKit were used, that would only give capabilities for the raw Ed25519 primitive. It would not work with X509Certificate{2} because, the best of my knowledge, Key Services do not support Ed25519 and nor does macOS support RFC8410, in that macOS will reject X.509 certificates that contain an id-Ed25519 AlgorithmIdentifier.

The lack of platform support anywhere other than OpenSSL currently makes this a bit of a tough sell to me. I have some thoughts on the APIs

  1. Generally I think .NET is starting to shift away from the SymmetricAlgorithm and AsymmetricAlgorithm classes (e.g. ChaCha20Poly1305 in .NET 6). I don't think this class should inherit from AsymmetricAlgorithm. These base classes are an artifact of an older "crypto agility" design where everything had a common base class.

    We could clean up the API shape then. For example, instead of byte[] SignData(byte[] data, int offset, int count); We could just have SignData(ReadOnlySpan<byte> data). Same for VerifyData, etc. The offset / count is pre-span era.

  2. How would someone generate a Ed448 (if it exists in the future) with this API shape? I wonder if Create should accept ECCurveType so that, if in the future, more EdDSA algorithms are added, only the enum will need to be changed.

@annerajb
Copy link
Author

  1. Generally I think .NET is starting to shift away from the SymmetricAlgorithm and AsymmetricAlgorithm classes (e.g. ChaCha20Poly1305 in .NET 6). I don't think this class should inherit from AsymmetricAlgorithm. These base classes are an artifact of an older "crypto agility" design where everything had a common base class.

One of the initial benefit I saw from using AsymmetricAlgorithm as a base class was all the function it already contained to handle Pkcs8, SubjectPublicKey, Pem Loading some of this which where recently added to it.

On a closer look it seems the heavy lifting is really handled by PemKeyHelpers.

We could clean up the API shape then. For example, instead of byte[] SignData(byte[] data, int offset, int count); We could just have SignData(ReadOnlySpan<byte> data). Same for VerifyData, etc. The offset / count is pre-span era.

Updated the sample to switch to ReadOnlySpan and remove offset arguments.

  1. How would someone generate a Ed448 (if it exists in the future) with this API shape? I wonder if Create should accept ECCurveType so that, if in the future, more EdDSA algorithms are added, only the enum will need to be changed.

I noticed I missed renaming ECCurveType to be EDCurveType went ahead and did that change.

I also removed the parameter less Create static function and instead added a new one that accepts the curve type.

public static new EDDsa Create(EDCurveType curve);

Also added a public static bool IsSupported { get; } since it would be needed anyways

@bartonjs
Copy link
Member

bartonjs commented Jan 7, 2022

Generally I think .NET is starting to shift away from the SymmetricAlgorithm and AsymmetricAlgorithm classes

For SymmetricAlgorithm that's effectively true... not because of a strong desire to abandon the types, but because all we've added lately was AE(AD) and it has such a different shape than SymmetricAlgorithm and ICryptoTransform that we decided they were just their own thing. (We didn't make a new AE(AD) base type because the algorithms have enough variation that the base type wasn't providing value).

For AsymmetricAlgorithm, we've been increasing the places that can take one... like EnvelopedCms.Decrypt. It's just trickier in API because DSA and ECDSA don't need extra context, but RSA has the PKCS1 vs PSS or PKCS1 vs OAEP (and what OAEP) split... which is why CertificateRequest doesn't take AsymmetricAlgorithm.

I also removed the parameter less Create static function and instead added a new one that accepts the curve type.

Why remove the old one? If you're loading from an existing key then having to specify a parameter just makes things potentially misleading (e.g. it says "create Ed25519", but after the load it's Ed448). An overload that takes the parameter is fine, that tells the implicit key generation which curve to use.

public enum EDCurveType

Using an enum is very limiting. If we load an Ed448 from a key file but that's not in the enum we're in a bad state. It probably wants to be something like a strongly typed string.

What should the casing / naming be? EdDsa or EDDsa?

Either "EdwardsDsa" or "EdDsa", but definitely not "EDDsa"... the first D doesn't stand for anything, it's just "Ed" or "Edwards" concat (Digital Signature Algorithm)=>DSA=>Dsa.

This would only support platforms where Openssl 1.1.1 is used by dotnet

https://dotnet.microsoft.com/en-us/platform/telemetry says that 60% of the command line usage is now on Linux (which is higher than I expected... guess we did a good job on cross-plat!). Assuming that all Linux usages have OpenSSL 1.1.1 or OpenSSL 3, and that none of them have disabled EdDSA, that leaves a 60/40 split that we don't have any current expectation of healing.

If macOS supported certificates with EdDSA then we could at least assume/pretend that Windows will eventually follow; and if Windows were to support it in preview builds then we'd know that eventually 38% (the 98% that install updates of the 39% Windows usage) more would have IsSupported=>true, and we'd be at 98/2 (and then Apple might come on board and we get 99/1).

Right now having something that only works on one OS family is a bit of a hard sell.

@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jan 7, 2022
@bartonjs bartonjs added this to the Future milestone Jan 7, 2022
@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2022

For AsymmetricAlgorithm, we've been increasing the places that can take one... like EnvelopedCms.Decrypt. It's just trickier in API because DSA and ECDSA don't need extra context, but RSA has the PKCS1 vs PSS or PKCS1 vs OAEP (and what OAEP) split... which is why CertificateRequest doesn't take AsymmetricAlgorithm.

I think that's kind of what bugs me a little about it. It's a cryptoagility-esq API that doesn't quite work. For that particular example, even though it accepts an AsymmetricAlgorithm it's going to throw for anything other than RSA (and Windows gives special treatment for FFDH).

RSA? key = privateKey as RSA;
if (privateKey != null && key == null)
{
exception = new CryptographicException(SR.Cryptography_Cms_Ktri_RSARequired);
return null;
}

EdDSA can't encrypt, but you'll be able to pass an instance in anyway. Maybe X25519 could if we ever added something like ECIES if we ever got there.

And because AsymmetricAlgorithm doesn't actually have sign, encrypt, or anything on it, you can't rely on the polymorphism to "just work" for actual asymmetric operations.

@bartonjs
Copy link
Member

Well, in your quoted example, we already know that we're a KeyTrans, which means that the relevant cert/KeyRecipientInfo was RSA (the only algorithm that would have matched a KeyTrans). For KeyAgree, if we supported that, and we supported both DH and ECDH, it'd look like

if (privateKey is DiffieHellman dh)
{
}
else if (privateKey is ECDiffieHellman ecdh)
{
}
else
{
    throw something;
}

We could make the public entrypoint be Decrypt(RecipientInfo, RSA), Decrypt(RecipientInfo, DiffieHellman), Decrypt(RecipientInfo, ECDiffieHellman)... or just Decrypt(RecipientInfo, AsymmetricAlgorithm) and you have to give us the right key (since they're all going to fail for "the recipientinfo and key don't agree" and/or "I tried this key, but it didn't work")

Encrypt is harder, since there's not already a RecipientInfo (etc) to tell us if we're supposed to use PKCS1/OAEP-SHA1/OAEP-SHA-2-256/...

I agree it's a bit asymmetric (semi-pun not intended, but it's the best word), but it feels better than object or unnecessary overload explosion.

@mregen
Copy link

mregen commented May 8, 2022

Imho this discussion should also include the support for X509Chain validation for EdDSA/Curve25519/Curve448 certs.
I'd love to see the support for the EdDsa based certificates, but in our use case with the OPC UA protocol without PKI chaining and signature validation by the platform we can at best just use self signed.

Right now having something that only works on one OS family is a bit of a hard sell.

There are many use cases in the industrial space where this makes a lot of sense. The edge devices run Linux and embedded sensors may only support the EdDsa/Curve25519/ChaChaPoly flavors due to their minimal CPUs.
With the recent push around cybersecurity .NET may put itself on the sidelines if it doesn't support it...

@Fabi
Copy link

Fabi commented Jun 23, 2022

Still not implemented???

@Blackclaws
Copy link

Seeing how JWTs with EdDSA are becoming more popular this is definitely something that is needed sooner rather than later if we want to keep verifying those without having to rely on third party libraries.

@vcsjones
Copy link
Member

vcsjones commented Jan 27, 2023

@bartonjs

As a first step, I did a rough implementation of Ed25519 for macOS and OpenSSL, enough to get passing with DJB's test vectors.

This meets our "two of three" semi-requirement, but it would mean we lack the ability to implement this on Windows.

Here is the API shape that I came up with:

namespace System.Security.Cryptography
{
    public abstract partial class Ed25519 : System.Security.Cryptography.AsymmetricAlgorithm
    {
        public const int SignatureSizeInBytes = 64;

        public static bool IsSupported { get; }
        public abstract bool HasPrivateKey { get; }

        [UnsupportedOSPlatformAttribute("android")] // MAYBE this does not apply.
        [UnsupportedOSPlatformAttribute("browser")]
        [UnsupportedOSPlatformAttribute("ios")]
        [UnsupportedOSPlatformAttribute("tvos")]
        [UnsupportedOSPlatformAttribute("windows")]
        public static new System.Security.Cryptography.Ed25519 Create();
        
        public byte[] ExportPrivateKey();
        public int ExportPrivateKey(System.Span<byte> destination);
        public bool TryExportPrivateKey(System.Span<byte> destination, out int bytesWritten);
        protected abstract int ExportPrivateKeyCore(System.Span<byte> destination);

        public byte[] ExportPublicKey();
        public int ExportPublicKey(System.Span<byte> destination);
        public bool TryExportPublicKey(System.Span<byte> destination, out int bytesWritten);
        protected abstract int ExportPublicKeyCore(System.Span<byte> destination);

        public abstract void GenerateKey();

        public void ImportPrivateKey(byte[] privateKey) { }
        public void ImportPrivateKey(System.ReadOnlySpan<byte> privateKey) { }
        protected abstract void ImportPrivateKeyCore(System.ReadOnlySpan<byte> privateKey);

        public void ImportPublicKey(byte[] publicKey) { }
        public void ImportPublicKey(System.ReadOnlySpan<byte> publicKey) { }
        protected abstract void ImportPublicKeyCore(System.ReadOnlySpan<byte> publicKey);

        public byte[] SignData(byte[] data);
        public byte[] SignData(System.ReadOnlySpan<byte> data);
        public int SignData(System.ReadOnlySpan<byte> data, System.Span<byte> destination);
        public bool TrySignData(System.ReadOnlySpan<byte> data, System.Span<byte> destination, out int bytesWritten);
        protected abstract int SignDataCore(System.ReadOnlySpan<byte> data, System.Span<byte> destination);

        public bool VerifyData(byte[] data, byte[] signature);
        public bool VerifyData(System.ReadOnlySpan<byte> data, System.ReadOnlySpan<byte> signature);
        protected abstract bool VerifyDataCore(System.ReadOnlySpan<byte> data, System.ReadOnlySpan<byte> signature);

        // Not included: All of the overrides from AsymmetricAlgorithm
    }

    public sealed partial class Ed25519OpenSsl : System.Security.Cryptography.Ed25519
    {
        public static new bool IsSupported { get; }

        [UnsupportedOSPlatformAttribute("android")]
        [UnsupportedOSPlatformAttribute("browser")]
        [UnsupportedOSPlatformAttribute("ios")]
        [UnsupportedOSPlatformAttribute("tvos")]
        [UnsupportedOSPlatformAttribute("windows")]
        public Ed25519OpenSsl(System.Security.Cryptography.SafeEvpPKeyHandle pkeyHandle);

        [UnsupportedOSPlatformAttribute("android")]
        [UnsupportedOSPlatformAttribute("browser")]
        [UnsupportedOSPlatformAttribute("ios")]
        [UnsupportedOSPlatformAttribute("tvos")]
        [UnsupportedOSPlatformAttribute("windows")]
        public Ed25519OpenSsl();

        // Not included: All of the overrides from AsymmetricAlgorithm or Ed25519
    }
}

Some comparisons with the proposal above:

  1. To keep mine simpler to read, all of the overrides from AsymmetricAlgorithm I removed. However yes, consider that we will override things like PKCS8 and SPKI exports.

  2. No EdDsa type. The algorithm is the type. If we ever get to adding something like Ed448, we will add it as another type. If it proves to be useful to have an EdDsa "intermediate" abstract class, we can extract it out later like we did with ECAlgorithm.

  3. The type directly exposes key imports and exports for the opaque keys. Since keys are opaque and by design don't have structure (like a Q.X and Q.Y) I saw no obvious need for a public / private tuple. Similarly, Ed25519 more strongly implies that having the private key without the public key is enough. So most key transport won't carry both (like ECDSA likes to carry D and Q together), the keys are just "sequences of bytes".

  4. I'm more uncertain about the work in X509Certificate2 and X509Chain. That's where our "two of three" becomes a "one of three". Apple implements the primitive only in CryptoKit. We can always do this as follow up work, but the situation for just the primitives is more clear as something that seems feasible to move forward with.

  5. We might be able to do Android. If I am reading Android's docs correctly, they implemented it in API Level 33, which is very new. The most recent, at the time of writing. We would need to do some kind of light-up work for it.

  6. I went back and forth on "should there be an Ed25519OpenSsl class that is public?" I went with "yes" to match the existing landscape. Note that the IntPtr constructor is not needed because Ed25519 works exclusively off of EVP_PKEY.

@bartonjs bartonjs 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 Mar 1, 2023
@bartonjs
Copy link
Member

bartonjs commented Mar 1, 2023

@vcsjones The original proposal reminded me that we should do the API for X509Certificate2+Ed25519, so I've gone ahead and merged that with your proposal into the top post.

Even if we cut it apart into two implementation PRs, it's probably worth getting the X.509 part pre-signed-off.

@vcsjones
Copy link
Member

vcsjones commented Mar 1, 2023

The original proposal reminded me that we should do the API for X509Certificate2+Ed25519, so I've gone ahead and merged that with your proposal into the top post.

That's fine if we just want to get the API approved... but I don't know that we should do that for .NET 8. From my earlier post:

I'm more uncertain about the work in X509Certificate2 and X509Chain. That's where our "two of three" becomes a "one of three". Apple implements the primitive only in CryptoKit. We can always do this as follow up work, but the situation for just the primitives is more clear as something that seems feasible to move forward with.

We can't implement Ed25519 + X.509 on Apple (and Apple summarily explodes on an Ed25519 cert).

Would you take that for .NET 8 if we can only implement those APIs for OpenSSL?

@bartonjs bartonjs modified the milestones: Future, 8.0.0 Mar 1, 2023
@bartonjs
Copy link
Member

bartonjs commented Mar 1, 2023

Would you take that for .NET 8 if we can only implement those APIs for OpenSSL?

Assuming that an Ed25519 (public only) cert already opens on Linux, it feels reasonable to add GetPublicKey with the algorithm... and then to add Ed25519 to the PFX association table and add GetPrivateKey and CopyWithPrivateKey. I think it'd be in good* company with DSA FIPS 186-3 in that I believe that "big DSA" certs work on Linux-only.

If loading the cert fails, then all bets are off, of course.

* debatable. OK, not really debatable, it's bad company, but it's a stock English phrase...

@terrajobst
Copy link
Member

terrajobst commented Mar 28, 2023

Video

  • The IsSupported properties should use [UnsupportedOSPlatformGuard] annotations
namespace System.Security.Cryptography;

public abstract partial class Ed25519 : AsymmetricAlgorithm
{
    public const int SignatureSizeInBytes = 64;

    public static bool IsSupported { get; }

    public static new Ed25519 Create();

    protected Ed25519();
    public abstract bool HasPrivateKey { get; }

    public byte[] ExportPrivateKey();
    public int ExportPrivateKey(Span<byte> destination);
    public bool TryExportPrivateKey(Span<byte> destination, out int bytesWritten);
    protected abstract int ExportPrivateKeyCore(Span<byte> destination);
    public byte[] ExportPublicKey();
    public int ExportPublicKey(Span<byte> destination);
    public bool TryExportPublicKey(Span<byte> destination, out int bytesWritten);
    protected abstract int ExportPublicKeyCore(Span<byte> destination);

    public abstract void GenerateKey();

    public void ImportPrivateKey(byte[] privateKey);
    public void ImportPrivateKey(ReadOnlySpan<byte> privateKey);
    protected abstract void ImportPrivateKeyCore(ReadOnlySpan<byte> privateKey);

    public void ImportPublicKey(byte[] publicKey);
    public void ImportPublicKey(ReadOnlySpan<byte> publicKey);
    protected abstract void ImportPublicKeyCore(ReadOnlySpan<byte> publicKey);
    public byte[] SignData(byte[] data);
    public byte[] SignData(ReadOnlySpan<byte> data);
    public int SignData(ReadOnlySpan<byte> data, Span<byte> destination);
    public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, out int bytesWritten);
    protected abstract int SignDataCore(ReadOnlySpan<byte> data, Span<byte> destination);

    public bool VerifyData(byte[] data, byte[] signature);
    public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature);
    protected abstract bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature);
}
public sealed partial class Ed25519OpenSsl : Ed25519
{
    [UnsupportedOSPlatformGuard("android")]
    [UnsupportedOSPlatformGuard("browser")]
    [UnsupportedOSPlatformGuard("ios")]
    [UnsupportedOSPlatformGuard("tvos")]
    [UnsupportedOSPlatformGuard("windows")]
    public static new bool IsSupported { get; }

    [UnsupportedOSPlatform("android")]
    [UnsupportedOSPlatform("browser")]
    [UnsupportedOSPlatform("ios")]
    [UnsupportedOSPlatform("tvos")]
    [UnsupportedOSPlatform("windows")]
    public Ed25519OpenSsl(SafeEvpPKeyHandle pkeyHandle);

    [UnsupportedOSPlatform("android")]
    [UnsupportedOSPlatform("browser")]
    [UnsupportedOSPlatform("ios")]
    [UnsupportedOSPlatform("tvos")]
    [UnsupportedOSPlatform("windows")]
    public Ed25519OpenSsl();
}
namespace System.Security.Cryptography.X509Certificates;

public sealed partial class PublicKey
{
    public Ed25519? GetEd25519PublicKey();
}

public partial class X509Certificate2
{
    public Ed25519? GetEd25519PublicKey();
    public Ed25519? GetEd25519PrivateKey();
    public X509Certificate2 CopyWithPrivateKey(Ed25519 privateKey);
}

@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 Mar 28, 2023
@bartonjs bartonjs modified the milestones: 8.0.0, Future May 25, 2023
@vcsjones vcsjones removed their assignment May 25, 2023
@filipnavara
Copy link
Member

Thanks both for double-checking 👍

@Blackclaws
Copy link

I do not like one for reasons I already hinted at: even well intentioned, Apple's implementation is not "real" Ed25519. They
are generating a nonce in a non-deterministic way, which two specifications say not to do it that way. We would not, for example, be able to claim that our implementation follows an RFC or NIST design.

I think that in the end it comes down to what the end goal is. For me as a user of the implementation I only care about the signatures being compatible between implementations. If the signatures produced on Apple are verifiable by other EdDsa implementations and don't produce invalid signatures those would suffice in my opinion for anything I would use them for (jwt token generation etc.).

It might be that in some cases where government contracts or the likes are affected people need FIPS compliant implementations but iirc those are sometimes actually worse than state of the art implementations of common crypto.

Seeing as how Apple doesn't provide an alternative in this case and the only alternative is non-implementation I'd say go ahead but mark it explicitly as non-conformant for Apple platforms with a big warning sign.

@bartonjs
Copy link
Member

mark it explicitly as non-conformant for Apple platforms with a big warning sign.

How/where would you recommend we do that? If someone wrote a platform neutral library that used Ed25519 so long as Ed25519.IsSupported returned true, that library would now need to carry the viral "if you run me on macOS I won't produce answers that necessarily pass all tests".

We'd essentially have to make a clone of the UnsupportedOS analyzer to carry something like NonConformantImplementation. Even that's not really good enough, because annotations are a design time problem, and conformance is a runtime problem... if .NET 8 says that Ed25519 is non-conformant on macOS, then any library building against .NET 8 as a minimum version believes that to be true. But, by .NET 9, we learn that Apple has pushed out an update to make their algorithm conform. Well, we can't remove the annotation from the .NET 8 targeting pack... so now the annotations lie.

While the RFC doesn't literally say EdDSA signatures MUST be deterministic, it's a MUST-level statement in it; so Apple's algorithm isn't actually EdDSA and thus isn't actually Ed25519, so we shouldn't call it Ed25519 in .NET. We could call it Ed25519SignatureCompatible... but that's clearly just an attempt to ship something.

Another problem that I'd have with "go ahead" here is that we try really hard to make .NET be .NET everywhere. When we can't, we usually want the experience to be PlatformNotSupportedException (or a similar "sorry, this doesn't work"). Saying that the same code gives a deterministic signature on some OSes, and a non-deterministic signature on others, isn't staying in that spirit. So, yes, while the alternative is non-implementation, I feel that non-implementation is the better option.

@Blackclaws
Copy link

How/where would you recommend we do that?

Mostly documentation regarding this function. In the end anyone that uses it should have read the documentation and know what they are getting themselves into. I get that this might be an issue for libraries that are cross platform and just link back to the base implementation and as you say the non-conformancy should spread to those sort of.

Well, we can't remove the annotation from the .NET 8 targeting pack... so now the annotations lie.

Might it be a better solution to not push this directly into the runtime but instead have it as a NuGet package that stays in preview until this issue is resolved? That way people that know what they are getting themselves into can use it but it doesn't appear as readily available to the general public.

Another problem that I'd have with "go ahead" here is that we try really hard to make .NET be .NET everywhere.

I think this is the biggest argument in favor of non-implementation. However there are enough things still in .NET that behave differently on Windows vs. other platforms and others that don't work the same on macOS either (see HTTP1/2 over HTTPS) which don't necessarily lead to PlatformNotSupported but just slightly different behaviour.

In the end I get that you'd not want to add to the pile of things that work differently though so fair enough. To me it seems that unless you'd consider shipping just the Linux Openssl implementation this might stay in Limbo for a rather long time. Is there any specific reason why a macOS Openssl version wouldn't work? Is it just that this is a dependency you don't want to pull into the runtime?

@vcsjones
Copy link
Member

Is there any specific reason why a macOS Openssl version wouldn't work? Is it just that this is a dependency you don't want to pull into the runtime?

Two things here.

First, when I say "platform", I mean different cryptographic implementations, not operating systems. OpenSSL on macOS and Linux is still one "platform", so that does not help us with our desire to support multiple platforms with cryptographic primitives.

Second, shipping OpenSSL in-box is not an option - .NET's current stance, by policy, is that we do not ship implementations with cryptographic implementations. We have in the past used OpenSSL on macOS if happened to be on the system - this is still used for AesCcm, for example. However this is an anti-goal. We want to cut ties from OpenSSL on macOS, not take further dependencies on it.

@ranma42
Copy link
Contributor

ranma42 commented May 26, 2023

Would it make sense to only support signature verification? (maybe not, AFAICT the current interfaces expect that either both signing and verification are supported, or none of them is)

@vcsjones
Copy link
Member

Would it make sense to only support signature verification? (maybe not, AFAICT the current interfaces expect that either both signing and verification are supported, or none of them is)

It's a reasonable thing to consider. It's an option I laid out in my earlier comment:

We split the difference: we can proceed with verify-only on Apple. Producing signatures would not function.

I largely chalked that up to being 1.5 platforms, not 2. To me it seems like it would be done for the sake of just meeting our minimum platform requirements, and would be a subpar experience on macOS. Though, without signing, I think leaving half the purpose of the algorithm out would be a stretch to say it supports two platforms in the first place.

@jeffhandley
Copy link
Member

Is there value in verify-only across the board? What are some scenarios for verifying but not producing?

@vcsjones
Copy link
Member

What are some scenarios for verifying but not producing?

The most common use case for verifying is when you want to verify a signature another party produced. JWT is a good example - a server may respond with a signed JWT, and a client has the public key for that party and wants to verify the signature exists.

I don't doubt there are many use cases for a verify only implementation - but in my personal opinion, it still teeters too close to having only one implementation.

@krwq
Copy link
Member

krwq commented Jun 14, 2023

Another option here could be to by default throw PNSE for non conformant stuff and create a feature switch "AllowNonConformantEd25519" - PNSE could provide detail about existence of that switch. I'm personally also fine with not supporting this until Windows/Android add their implementations

@vcsjones
Copy link
Member

Another option here could be to by default throw PNSE for non conformant stuff and create a feature switch "AllowNonConformantEd25519"

That more or less I think falls in line with something @bartonjs reasoned through:

Adding a property like public bool IsDeterministic { get; }... but that could just be renamed to public bool IsConformantImplementation { get; }, and once we're saying "this is a thing that's like, but not actually, Ed25519" that just sounds like a mistake.

@Neustradamus

This comment was marked as off-topic.

@dariogriffo
Copy link

Any update on this? At the moment we rely on a nuget package based on bouncy castle to sign our jwr bit is outdated and still references microsoft.jwt.model 5.4.0 with critical unresolved security issues

@vcsjones
Copy link
Member

Any update on this?

Unfortunately not. The status remains the same as noted in this previous comment. The implementation remains blocked.

@Fabi
Copy link

Fabi commented Mar 26, 2024

Then again go damn back to openssl used crypto stuff. You guys block so many things on mac or functionality in general just because you don't want to use openssl on mac for example. It worked before (see aes gcm) where it was removed for now reasons too and replaced with the apple implementation for no real reasons.

@filipnavara
Copy link
Member

filipnavara commented Mar 26, 2024

OpenSSL is not distributed in-box with operating system on macOS. That would shift the burden of distributing it to you as a user, including all security updates.

Moreover, no one is stopping you from using third-party libraries such as NSec.

@Fabi
Copy link

Fabi commented Mar 26, 2024

Did you really just suggest using another 3rd party lib that has a dep on another 3rd party library instead of having people just use a simple brew install openssl or so on mac which is a very common thing, especially for devs. And having linux/mac implementations just working the same way out of the box and having a great similar experience? It worked fine for everyone before when some things used openssl on mac here and it would still do and be a consistent and better experience for everyone.

@filipnavara
Copy link
Member

Did you really just suggest using another 3rd party lib that has a dep on another 3rd party library...

No. NSec is port of libsodium to C#, no extra dependencies and it is self-contained. It's merely a suggestion, not an endorsement. As much as I would like .NET to ship an in-box implementation of this cryptographic primitive it's not currently possible within the given constrains (cryptography stack is shipped with OS and .NET doesn't implement encryption algorithms by itself; at least two of the three major platforms support a given cryptographic primitive).

...instead of having people just use a simple brew install openssl or so on mac which is a very common thing, especially for devs.

Firstly, you somehow assume that all consumers of .NET are developers. That's not the case. Neither is brew installed on macOS machines by default, and it doesn't come with a built-in update mechanism for security updates either. That makes it a non-starter as a general purpose solution.

It worked fine for everyone before when some things used openssl on mac

It didn't work fine for everyone. In fact, it was a common pit of failure because developers often had OpenSSL installed on their machines and mistakenly assumed that AesGcm universally works, and they came in for a surprise once they started testing their application on customer machines.

@Fabi
Copy link

Fabi commented Mar 26, 2024

NSec pinvokes libsodium so it is a dependency and not a port.

I don't think there were many open issues anywhere (can't speak for you doing internal stuff) regarding openssl and issues on it.

Sure brew is not installed by default but it's still the most common way to install any kind of packages like openssl or others. Including updating those. It's very rare that you use other things on macOS regarding that. Even some MS docs use it as suggestion for some deps that are required for several things already because it's probably the most use dep/library tool out there.

Also why not keep it as an option as proposed before through an environment variable to use openssl? That would have been nice for example for things like AesGcm which is fully unusable for me now on macOS as .NET "implementation" due to how a client that I have to work with. Sure I got a 'workaround' by copying the code before it got changed but still that wasn't nice to do. Such a switch (more like the openssl lib path) kind of worked at that time and turned AesGcm from unsupported to supported already.

@filipnavara
Copy link
Member

NSec pinvokes libsodium so it is a dependency and not a port.

I stand corrected. There was some other managed port and I got them confused. That said, it solves the distribution problem by shifting it into NuGets that are linked into your application and where you are responsible for their timely updates. libsodium was also audited several times in the past so it comes with some indirect guarantees. It may not be a perfect solution but it's a solution that you can use right now.

Sure brew is not installed by default but it's still the most common way to install any kind of packages like openssl or others.

I am not in any way disputing that, when it comes to developers. It's totally different story when it comes to end users who obtain most applications from the App Store or as web downloads. I happen to be a developer with brew and openssl on my machine, but I also distribute an end user application to millions of users among multiple platforms and that's vastly different from a scenario where you have total control of your machine or the machines where the software is deployed.

Also why not keep it as an option as proposed before through an environment variable to use openssl? That would have been nice for example for things like AesGcm which is fully unusable for me now on macOS as .NET "implementation" due to how a client that I have to work with.

This is off-topic and we have already discussed this to death in the past. The decision to drop the OpenSSL support from AesGcm on macOS was done with the same pit-of-failure argument. I was not the one presenting the argument and I was not the one making the decision, I simply agree with it. You are free to disagree, open an issue, present your use case and ask for the decision to be re-evaluated.

@amirvenus
Copy link

amirvenus commented Jun 16, 2024

Would be great if the support includes .net Android as well.

@vcsjones
Copy link
Member

Android 13 (API level 33) is documented as supporting Ed25519. https://developer.android.com/reference/kotlin/java/security/Signature

Let's see if we can land Ed25519 for Linux and Android.

@vcsjones vcsjones self-assigned this Aug 30, 2024
@vcsjones vcsjones modified the milestones: Future, 10.0.0 Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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