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: Obsolete RNGCryptoServiceProvider #40169

Closed
GrabYourPitchforks opened this issue Jul 31, 2020 · 13 comments · Fixed by #52373
Closed

API proposal: Obsolete RNGCryptoServiceProvider #40169

GrabYourPitchforks opened this issue Jul 31, 2020 · 13 comments · Fixed by #52373
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

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.

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

ghost commented Jul 31, 2020

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

@vcsjones
Copy link
Member

vcsjones commented Jul 31, 2020

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
Copy link
Member Author

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

@vcsjones
Copy link
Member

vcsjones commented Aug 1, 2020

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
Copy link
Member Author

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
Copy link

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

@marek-safar
Copy link
Contributor

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

@bartonjs
Copy link
Member

@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
Copy link

KalleOlaviNiemitalo commented Oct 23, 2020

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
Copy link
Member

@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
Copy link
Member Author

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 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 Jan 13, 2021
@bartonjs bartonjs modified the milestones: Future, 6.0.0 Jan 13, 2021
@bartonjs
Copy link
Member

bartonjs commented Jan 15, 2021

Video

Looks good as proposed.

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

@bartonjs bartonjs 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 15, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 6, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 8, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 7, 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

Successfully merging a pull request may close this issue.

7 participants