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

RNGCryptoServiceProvider offers unappropriate API #30341

Closed
rillig opened this issue Jul 22, 2019 · 11 comments
Closed

RNGCryptoServiceProvider offers unappropriate API #30341

rillig opened this issue Jul 22, 2019 · 11 comments
Assignees
Labels
area-System.Security documentation Documentation bug or enhancement, does not impact product or test code
Milestone

Comments

@rillig
Copy link

rillig commented Jul 22, 2019

The RNGCryptoServiceProvider should have an API that provides all the convenience methods of the standard Random class. It looks ridiculous having an example code snippet that is 34 lines longer than necessary.

If there were a decorator class providing the NextInt functions for rolling a dice or selecting a random element from an array or collection, many of the Stack Overflow answers could be written with a reasonable amount of code. As it is now, there is actual danger of people using the secure random number generator and then forgetting about the bias, just because they don't want to copy and paste the boilerplate code from this example.

This boilerplate code should be implemented exactly once, in the .NET standard library, and not millions of times by inexperienced programmers in a hurry of meeting a deadline.

References:

@vcsjones
Copy link
Member

vcsjones commented Jul 22, 2019

GetInt32 was added to System.Security.Cryptography.RandomNumberGenerator and will be available in .NET Core 3 and .NET Standard 2.1. Does this provide the solution you were looking for?

https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.randomnumbergenerator.getint32?view=netcore-3.0

@rillig
Copy link
Author

rillig commented Jul 24, 2019

Partly. The documentation is still bloated. The documentation still needs to be adjusted to use this shiny new API. Having bloated code in the API documentation makes the API team look bad for no reason.

@danmoseley
Copy link
Member

@bartonjs is going through our API docs I believe.

@danmoseley
Copy link
Member

@rillig
Copy link
Author

rillig commented Jul 25, 2019

@danmosemsft Thanks for the offer, but no. It's Microsoft's job to fix the documentation. And I don't write C#, Visual Basic and F# and assembly fluently to correct all of them. I strongly believe that these example snippets are all generated from a common template, otherwise this job would be really frustratingly boring and error-prone, and I cannot believe Microsoft is living that far in the past.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@GrabYourPitchforks GrabYourPitchforks self-assigned this May 7, 2021
@owlstead
Copy link

owlstead commented Aug 6, 2021

@bartonjs I have created and tested a SecureRandom class that implements Random and takes a RandomNumberGenerator, if you're interested. Disadvantage: if functions rely on NextDouble() then you may not get a normal distribution; not evenly spread out and only 53 bits of mantissa.

@owlstead
Copy link

owlstead commented Aug 6, 2021

By the way, I'm trying to create an issue w.r.t. to this example: https://docs.microsoft.com/en-us/dotnet/api/system.random?view=net-5.0#generate-random-64-bit-integers which should be rewritten to use 8 random bytes and a conversion to long, but I don't know where. Sorry to put that here.

@vcsjones
Copy link
Member

vcsjones commented Aug 6, 2021

but I don't know where. Sorry to put that here.

If you mean an issue about the documentation itself, it should be filed in https://github.com/dotnet/dotnet-api-docs.

@danmoseley
Copy link
Member

Or better still @owlstead you can offer a PR in dotnet-api-docs, which would get it fixed immediately. 😸

@owlstead
Copy link

owlstead commented Aug 6, 2021

@danmoseley I couldn't find the documentation page, so I went with the middle ground, raising an issue but with the code included.

@GrabYourPitchforks
Copy link
Member

Transferred to dotnet/dotnet-api-docs#6997.

@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests

8 participants