[API Proposal]: Introduce a common base type for ECDsa and ECDiffieHellman

[vcsjones]

Background and motivation

Today, ECDsa and ECDiffieHellman directly derive from AsymmetricAlgorithm. Though they offer two different purposes, signing and key exchange respectively, they have quite a bit of shared API surface and functionality because they both use EC keys.

The lack of a common base type means there is a lot of duplication between the two, from unit tests to actual implementation in dotnet/runtime itself. For example, many of the key tests use generics and indirection so the tests are not duplicated^{1, 2}.

Given the increasing shared API surface, I would propose introducing a new abstract class between ECDsa / ECDiffieHellman and AsymmetricAlgorithm.

After looking are the breaking change docs, I believe this type of change is allowed:

✓ Allowed
Introducing a new base class

So long as it does not introduce any new abstract members or change the semantics or behavior of existing members, a type can be introduced into a hierarchy between two existing types. For example, between .NET Framework 1.1 and .NET Framework 2.0, we introduced DbConnection as a new base class for SqlConnection which previously derived from Component.

and for pushing the virtuals down:

✓ Allowed
Moving a method onto a class higher in the hierarchy tree of the type from which it was removed

API Proposal

namespace System.Security.Cryptography
{
+    public abstract class ECAlgorithm : AsymmetricAlgorithm
+    {
#       Existing implementations on ECDsa and ECDiffieHellman that throw NotImplementedException
#       These will throw NotImplementedException, as they do now.
+       public virtual void ImportParameters(ECParameters parameters);
+       public virtual ECParameters ExportParameters(bool includePrivateParameters);
+       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
+       public virtual void GenerateKey(ECCurve curve);

#       Virtuals on ECDsa and ECDiffieHellman with identical implementations.
#       We could push the implementation down to this type.
+       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public virtual byte[] ExportECPrivateKey();
+       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#       Overrides from ECDsa and ECDiffieHellman that have identical implementation and
#       can be pushed down to this type.
+       public override void ImportFromPem(ReadOnlySpan<char> input);
+       public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
+       public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
+       public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
+       public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#       APIs that are new in .NET 7 and are not too late to move from ECDsa and ECDiffieHellman
#       since they are non-virtual but identical implementation.
+       public string ExportECPrivateKeyPem();
+       public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
+    }

-   public abstract class ECDsa : AsymmetricAlgorithm
+   public abstract class ECDsa : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }

-   public abstract class ECDiffieHellman : AsymmetricAlgorithm
+   public abstract class ECDiffieHellman : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }
}

API Usage

No particular API usage to demonstrate but this box is required. Hovering over this text will have a tooltip of a squid, though.

Alternative Designs

Do nothing.

Risks

Still perhaps somehow disruptive in an unforeseen way.

[ghost]

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

Issue Details

---

Background and motivation

Today, ECDsa and ECDiffieHellman directly derive from AsymmetricAlgorithm. Though they offer two different purposes, signing and key exchange respectively, they have quite a bit of shared API surface and functionality because they both use EC keys.

The lack of a common base type means there is quite a bit of duplication between the two, from unit tests to actual implementation in dotnet/runtime itself. For example, many of the key tests use generics and indirection so the tests are not duplicated^{1, 2}.

Given the increasing shared API surface, I would propose introducing a new abstract class between ECDsa / ECDiffieHellman and AsymmetricAlgorithm.

After looking are the breaking change docs, I believe this type of change is allowed:

✓ Allowed
Introducing a new base class

So long as it does not introduce any new abstract members or change the semantics or behavior of existing members, a type can be introduced into a hierarchy between two existing types. For example, between .NET Framework 1.1 and .NET Framework 2.0, we introduced DbConnection as a new base class for SqlConnection which previously derived from Component.

API Proposal

namespace System.Security.Cryptography
{
+    public abstract class ECAlgorithm : AsymmetricAlgorithm
+    {
#       Existing implementations on ECDsa and ECDiffieHellman that throw NotImplementedException
#       These will throw NotImplementedException, as they do now.
+       public virtual void ImportParameters(ECParameters parameters);
+       public virtual ECParameters ExportParameters(bool includePrivateParameters);
+       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
+       public virtual void GenerateKey(ECCurve curve);

#       Virtuals on ECDsa and ECDiffieHellman with identical implementations.
#       We could push the implementation down to this type.
+       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public virtual byte[] ExportECPrivateKey();
+       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#       Overrides from ECDsa and ECDiffieHellman that have identical implementation and
#       can be pushed down to this type.
+       public override void ImportFromPem(ReadOnlySpan<char> input);
+       public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
+       public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
+       public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
+       public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#       APIs that are new in .NET 7 and are not too late to move from ECDsa and ECDiffieHellman
#       since they are non-virtual but identical implementation.
+       public string ExportECPrivateKeyPem();
+       public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
+    }

-   public abstract class ECDsa : AsymmetricAlgorithm
+   public abstract class ECDsa : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }

-   public abstract class ECDiffieHellman : AsymmetricAlgorithm
+   public abstract class ECDiffieHellman : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }
}

API Usage

No particular API usage to demonstrate but this box is required. Hovering over this text will have a tooltip of a squid, though.

Alternative Designs

Do nothing.

Risks

Still perhaps somehow disruptive in an unforeseen way.

Author:	vcsjones
Assignees:	-
Labels:	api-suggestion, area-System.Security
Milestone:	-

[bartonjs]

@terrajobst / @stephentoub Would this cause a problem for/from .NET Standard 2.0? I... think it'd be fine; but I'm clearly not confident enough to bring this to the big meeting without first seeking a second assention explicitly 😄.

[stephentoub]

Adding a new intermediate base type shouldn't be a breaking change.

[terrajobst]

Yes, we have inserted new types in an existing hierarchy before, most notably TypeInfo. We consider this a non-breaking change, but there are cornercases, like overrides that call base. The compiler burned in a non-virtual call o the old base class. If you run against the new version that code doesn't run against the inserted type, it runs against the old base. So your design cannot assume that the new inserted base is called.

[GrabYourPitchforks]

The compiler burned in a non-virtual call o the old base class. If you run against the new version that code doesn't run against the inserted type, it runs against the old base. So your design cannot assume that the new inserted base is called.

See also #59289.

[bartonjs]

Video

Looks good as proposed

namespace System.Security.Cryptography
{
+    public abstract class ECAlgorithm : AsymmetricAlgorithm
+    {
#       Existing implementations on ECDsa and ECDiffieHellman that throw NotImplementedException
#       These will throw NotImplementedException, as they do now.
+       public virtual void ImportParameters(ECParameters parameters);
+       public virtual ECParameters ExportParameters(bool includePrivateParameters);
+       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
+       public virtual void GenerateKey(ECCurve curve);

#       Virtuals on ECDsa and ECDiffieHellman with identical implementations.
#       We could push the implementation down to this type.
+       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public virtual byte[] ExportECPrivateKey();
+       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#       Overrides from ECDsa and ECDiffieHellman that have identical implementation and
#       can be pushed down to this type.
+       public override void ImportFromPem(ReadOnlySpan<char> input);
+       public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
+       public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
+       public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
+       public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
+       public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#       APIs that are new in .NET 7 and are not too late to move from ECDsa and ECDiffieHellman
#       since they are non-virtual but identical implementation.
+       public string ExportECPrivateKeyPem();
+       public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
+    }

-   public abstract class ECDsa : AsymmetricAlgorithm
+   public abstract class ECDsa : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }

-   public abstract class ECDiffieHellman : AsymmetricAlgorithm
+   public abstract class ECDiffieHellman : ECAlgorithm
    {
#       existing virtuals that are now handled by the base class.
#       if we need the members to explicitly exist on this type, then they
#       can become overrides that simply call `base.` I've been told that the
#       CLR correctly handles dispatching to the base type when virtuals are removed.
-       public virtual void ImportParameters(ECParameters parameters);
-       public virtual ECParameters ExportParameters(bool includePrivateParameters);
-       public virtual ECParameters ExportExplicitParameters(bool includePrivateParameters);
-       public virtual void GenerateKey(ECCurve curve);
-       public virtual void ImportECPrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-       public virtual byte[] ExportECPrivateKey();
-       public virtual bool TryExportECPrivateKey(Span<byte> destination, out int bytesWritten);

#      overrides that can be removed since they are now handled by the base
-      public override void ImportFromPem(ReadOnlySpan<char> input);
-      public override void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
-      public override bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
-      public override void ImportEncryptedPkcs8PrivateKey(ReadOnlySpan<byte> passwordBytes, ReadOnlySpan<byte> source, out int bytesRead);
-      public override void ImportPkcs8PrivateKey(ReadOnlySpan<byte> source, out int bytesRead);
-      public override void TryExportEncryptedPkcs8PrivateKey(ReadOnlySpan<char> password, PbeParameters pbeParameters, Span<byte> destination, out int bytesWritten);

#      These are non virtual with identical implementations between ECDsa and ECDiffieHellman
#      They have not shipped in any .NET 7 so we can move them down if this proposal gets accepted for .NET 7.
-      public string ExportECPrivateKeyPem();
-      public bool TryExportECPrivateKeyPem(Span<char> destination, out int charsWritten);
    }
}