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]: Random.Get{Hex}String #111956

Open
stephentoub opened this issue Jan 29, 2025 · 4 comments · May be fixed by #112162
Open

[API Proposal]: Random.Get{Hex}String #111956

stephentoub opened this issue Jan 29, 2025 · 4 comments · May be fixed by #112162
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Jan 29, 2025

Background and motivation

We added GetString and GetHexString methods to RandomNumberGenerator. Random didn't get these methods as well, but they're useful there as well. There's little about these that are specific to something that's cryptographically-secure. Developers need to work around this by using string.Create and then Random.GetItems to fill the backing space. If the simple overloads were warranted for RandomNumberGenerator, they're just as applicable to Random.

API Proposal

namespace System;

public class Random
{
    // same signatures as on RandomNumberGenerator, but instance instead of static
+   public string GetString(ReadOnlySpan<char> choices, int length);
+   public string GetHexString(int stringLength, bool lowercase = false);
+   public void   GetHexString(Span<char> destination, bool lowercase = false);
}

API Usage

string s = Random.Shared.GetString("-._0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz", 8);

Alternative Designs

Making the members virtual. But other such members, including GetItems, that have been added recently haven't been virtual.

Risks

No response

@stephentoub stephentoub added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jan 29, 2025
@stephentoub stephentoub added this to the 10.0.0 milestone Jan 29, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 29, 2025
@stephentoub stephentoub added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 29, 2025
Copy link
Contributor

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

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Feb 4, 2025

If the simple overloads were warranted for RandomNumberGenerator, they're just as applicable to Random.

I want to push back a little against this statement, because I don't think it's universally true. A byte stream retrieved from RandomNumberGenerator is likely to be used as key material, or a nonce, etc. And a hex string is just a transmittable text representation of that data. It's essentially a helper over the existing RandomNumberGenerator.GetBytes method.

The Random.NextBytes method, on the other hand, has more restricted legitimate use cases. While getting one-off numbers (Random.Next) finds use in mechanisms like backoff and throttling, sequences of data (like via NextBytes) are much more likely to be transmitted or persisted. This merits a deeper investigation on where the data eventually flows to. And it's why callers of APIs like Random.NextBytes are occasionally audited by internal security teams -- because they're more likely to indicate misuse.

This isn't to say the API should be rejected. There are valid use cases, such as generating filenames in a trusted destination directory (emphasis on trusted since you don't want to implicitly rely on unpredictability). I'm just pushing back on the "it exists <here>, therefore it should exist <there>" justification as quoted above and I personally would like to see a stronger argument.

@stephentoub
Copy link
Member Author

There are plenty of uses, as you call out. Right now we make it harder to use the cheaper option, either pushing users to the cryptographically-secure api for convenience when it's not technically warranted, or making them write more complicated code. If there were zero uses, sure, but that's not the case, and my comment is in the context of there being valid uses. For both, GetHexString is a convenience method.

@bartonjs
Copy link
Member

bartonjs commented Feb 4, 2025

Video

  • When we added these to the CSPRNG we believed no one would need them on System.Random. New data has proven otherwise.
  • "Should these be virtual?" GetItems and Shuffle aren't. These are matching that. If we change any to virtual we should change all to virtual.
  • Should we add GetString that writes to a span? No, it's redundant with GetItems.
namespace System;

public partial class Random
{
    // same signatures as on RandomNumberGenerator, but instance instead of static
    public string GetString(ReadOnlySpan<char> choices, int length);
    public string GetHexString(int stringLength, bool lowercase = false);
    public void   GetHexString(Span<char> destination, bool lowercase = false);
}

@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 Feb 4, 2025
@stephentoub stephentoub self-assigned this Feb 4, 2025
@stephentoub stephentoub linked a pull request Feb 4, 2025 that will close this issue
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants