API proposal: Obsolete RNGCryptoServiceProvider

[GrabYourPitchforks]

Background and Motivation

The RNGCryptoServiceProvider class is a relic from the old Windows CAPI days of yore. The original Win32 API is no longer recommended, preferring CNG for all new work.

In .NET Core, the RNGCryptoServiceProvider type is marked [EditorBrowsable(Never)], and the implementation ignores all provided constructor parameters and delegates to the underlying preferred OS implementation anyway.

There's no reason for an application targeting .NET 6.0+ to use this API. Apps should instead use RandomNumberGenerator.Create(). For AOT and linker trimming scenarios, this could also help eliminate the app's dependency on the package which contains the RNGCryptoServiceProvider type, reducing overall memory usage and disk footprint.

Proposed API

namespace System.Security.Cryptography
{
    [EditorBrowsable(EditorBrowsableState.Never)] // existing attribute
    [Obsolete("This type is obsolete. Use RandomNumberGenerator.Create() instead.")] // new attribute
    public sealed class RNGCryptoServiceProvider : RandomNumberGenerator
    { /* ... */ }
}

This could be accompanied by a fixer with two behaviors:

• All calls to RNGCryptoServiceProvider ctors become calls to the parameterless overload RandomNumberGenerator.Create().
• All fields / locals / parameters of type RNGCryptoServiceProvider instead become type RandomNumberGenerator.

The obsoletion would not affect apps targeting netstandard or .NET versions prior to 6.0, as the reference assemblies would not contain these annotations. However, the fixer could apply to all target frameworks.

[ghost]

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

[vcsjones]

On the topic of RandomNumberGenerator, maybe we should steer people to not using Create + GetBytes at all, but instead use the static Fill member. Perhaps that can be part of the fixer, or at least a suggestion in the documentation.

[GrabYourPitchforks]

If we really wanted, we could also have RandomNumberGenerator.Create return a singleton whose dispose method no-ops.

[vcsjones]

If we really wanted, we could also have RandomNumberGenerator.Create return a singleton

Hm, I'll play around with that idea. What problems / concerns arise from calling GC.SuppressFinalize repeatedly? The RNG uses that dispose pattern where Dispose() is non-virtual and calls Dispose(bool disposing) for inheritors to override, then the parameterless one suppresses finalization.

type is marked [EditorBrowsable(Never)]

There are many other crypto classes marked as such, SHA256Managed and other Managed friends, etc. Others are constructors that don't make sense in Core like HMACSHA1(byte[] key, bool useManagedSha1).

Does it make sense to broaden this issue to include them?

[GrabYourPitchforks]

Does it make sense to broaden this issue to include them?

Possibly. We could try making the changes to all callers within the dotnet/* repos before marking things as obsolete, just to see how much churn it might cause.

[SteveSyfuhs]

Would the call to Create() seed the RNG differently than what Fill() ends up using, or are they sourced from the same seed?

[bartonjs]

Would the call to Create() seed the RNG differently than what Fill() ends up using, or are they sourced from the same seed?

There are no per-instance seeds. All of our CSPRNGs use stateless functions (or, at least, functions that only use hidden state).

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/RandomNumberGeneratorImplementation.OSX.cs

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/RandomNumberGeneratorImplementation.Unix.cs

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/RandomNumberGeneratorImplementation.Windows.cs

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/RandomNumberGeneratorImplementation.Browser.cs

[marek-safar]

@GrabYourPitchforks @terrajobst is this something what will be done for 6.0? It'd help us to limit browser crypto implementation.

[bartonjs]

@marek-safar I don't think the timeline on marking it Obsolete helps browser. The RNGCryptoServiceProvider class is fake, it just wraps an instance of RandomNumberGenerator.Create: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Csp/src/System/Security/Cryptography/RNGCryptoServiceProvider.cs

So there shouldn't be any special work for Browser to do.

[KalleOlaviNiemitalo]

Currently, RNGCryptoServiceProvider is documented as thread-safe, while RandomNumberGenerator is not (dotnet/dotnet-api-docs#3741). And I don't think you can document RandomNumberGenerator as thread-safe in general, because existing software may define derived classes that are not thread-safe.

• All calls to RNGCryptoServiceProvider ctors become calls to the parameterless overload RandomNumberGenerator.Create().

I think you should then document that RandomNumberGenerator.Create() always returns a thread-safe instance in .NET 5, but RandomNumberGenerator.Create(string) might not.

• All fields / locals / parameters of type RNGCryptoServiceProvider instead become type RandomNumberGenerator.

That will make it harder for developers to be sure that the field / local / parameter refers to a thread-safe instance.

However, the fixer could apply to all target frameworks.

On .NET Framework, RandomNumberGenerator.Create() can be configured to create an instance of a type whose instances are not thread-safe, so the fixer would not be entirely safe there.

[bartonjs]

@KalleOlaviNiemitalo The analyzer/fixer would probably only apply to libraries and applications targeting .NET 6.0 or higher, so .NET Framework concerns don't really apply.

@GrabYourPitchforks Honestly, if anyone's using instances of RNGCryptoServiceProvider they should just switch to using the static methods (it's a sealed type, so there's no DRBG mocking/substitution going on, (except maybe via CspParameters ctors? but those already don't work in core)). So no local/field replacement is warranted.

[GrabYourPitchforks]

I don't think we'd document that RandomNumberGenerator.Create() returns a thread-safe instance. It would just be an implementation detail.

And honestly it's just an optimization anyway. Probably a premature optimization, as if allocating RNG instances is the bottleneck in your code you're certainly doing something unique. :) If we steer devs toward the static members instead of the instance members it's all moot anyway.

[bartonjs]

Video

Looks good as proposed.

namespace System.Security.Cryptography
{
    [EditorBrowsable(EditorBrowsableState.Never)] // existing attribute
    [Obsolete(DiagnosticId: nextId)] // new attribute
    public sealed class RNGCryptoServiceProvider : RandomNumberGenerator
    { /* ... */ }
}