(Should we) Provide implementations for {RSA,DSA,ECDsa}.HashData(?)

[bartonjs]

All of our derived types for these algorithms now call the exact same helpers (except maybe DSACryptoServiceProvider, which fails if you ask for something other than SHA-1, because of FIPS 186-2 limitations).

Perhaps instead of re-implementing these methods identically 14 times ({CNG,CSP,OpenSSL,Android,Apple} * {RSA,DSA,ECDSA} - DSACSP)) we should define them just the 3 times.

[ghost]

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

Issue Details

---

All of our derived types for these algorithms now call the exact same helpers (except maybe DSACryptoServiceProvider, which fails if you ask for something other than SHA-1, because of FIPS 186-2 limitations).

Perhaps instead of re-implementing these methods identically 14 times ({CNG,CSP,OpenSSL,Android,Apple} * {RSA,DSA,ECDSA} - DSACSP)) we should define them just the 3 times.

Author:	bartonjs
Assignees:	-
Labels:	design-discussion, area-System.Security
Milestone:	Future

[vcsjones]

Seems reasonable. I can throw up a PR to see what it'd look like.

[bartonjs]

Really this is a bit "I have no idea why we made these methods as virtual+throw" (except where they're, maybe, abstract). Possibly it was just that we didn't have access to the dispatchers we wanted to use on .NET Framework (System.dll vs System.Core.dll).

Unless someone can remember a good reason why they're virtual+throw then virtual+work seems good to me.

@nguerrera any chance you have the answer in the back of your head? I feel like some of these popped up in 4.6.0 😄

[vcsjones]

Well, TryHashData is not virtual + throw, it has an implementation that calls HashData. That is… probably… a breaking change of some kind if we stopped calling a virtual that others were implementing.

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/RSA.cs

Line 100 in 3ae8739

protected virtual bool TryHashData(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, out int bytesWritten)

We would want to continue to override TryHashData in our implementations since we can provide a better implementation than the one that is provided.

Or, we stop calling the virtual and take it as a breaking change, if we want.

[nguerrera]

I'm afraid any details I had about this have long escaped my memory. 😂

[bartonjs]

I was going to suggest something like using reflection to ask if the current type was defined in the same assembly as typeof(RSA), and wonder if the JIT would drop the body for our implementations (since they're sealed)... and decided it's probably just simpler to keep the overrides.

Though, if the JIT does do a good job there (or has a similar pattern for it) then it would let us delete more trivia.

So... less a suggestion than an area to maybe investigate.

[vcsjones]

I was going to suggest something like using reflection to ask if the current type was defined in the same assembly

If we make it a static readonly field doesn’t the JIT do a decent job of burning those in? I think we did that for “Is this Windows 10 and have pseudo handles” where we determined that works.

Ohhh I misunderstood. It can’t be a static readonly.

[GrabYourPitchforks]

I think you can implement an internal interface IMySpecialMarker on all of the sealed types, then if (this is IMySpecialMarker) { /* fast-path */ } within the base class method body. In theory, the trimmer / linker should be able to deduce that there are no concrete RSA/DSA/etc.-derived types other than the sealed inbox ones that implement this special interface, so it will turn this into if (true) at trim time.

[vcsjones]

Since #66349 was merged, closing this.