crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.32k stars 1.61k forks source link

Suggestion: Add Random#alphanumeric #8993

Open straight-shoota opened 4 years ago

straight-shoota commented 4 years ago

UPDATED: while originally proposed as password generator, this is not a good idea because of the reasons mentioned below. But instead the idea was changed to adding Random#alphanumeric method. This could be used for similar use cases, but doesn't associate any strong properties a password generator would do.

ORIGINAL DESCRIPTION: It happend that I needed to assign some randomly generated passwords. In Crystal's stdlib there are a few methods on Random that could be used for that, but nothing specific for passwords that are easy to use for humans and avoid characters that can be easily mistaken (l and I for example).

So I'd suggest to add a Random#password method which deals specifically with generating a string that can be used as a password or token of some kind. Ideally, the character set can be chosen. I think this is a common enough feature to be in stdlib. It's also a sensible topic because custom implentations are not that difficult to write, but could easily create security issues.

A basic implementation could look like this:

module Random
  LEGIBLE_ALPHANUM = "234679ACDEFGHJKMNPQRTWXYZabcdefghjkmnopqrstwxy"

  def self.password(size : Int = 16, charset : String = LEGIBLE_ALPHANUM)
    charset = charset.chars

    String.build(size) do |io|
      size.times do
        io << charset.sample(self)
      end
    end
  end
end
Sija commented 4 years ago

@straight-shoota You've surprised me by this one! Isn't that a perfect example for a shard? I can imagine many more functionalities from a password generator than the ones you've mentioned - word-like passwords, different rules regarding mixing the characters and so on.

bararchy commented 4 years ago

Not even taking into account the discussion about what should be suggested as a possible password, the complexity needed, etc.. in your example above this is really a bad password, the charset is quite simple, it's either use a very long memorable phrase that can be simple but very hard to brute force, or, use a very complex shorter string.

straight-shoota commented 4 years ago

Isn't that a perfect example for a shard?

If you want more advanced features, yes. This is supposed to provide just a simple randomly generated string. Nothing fancy at all. IMO that can easily fit in stdlib.

There are already methods to generate strings based on base64 and base16, this is essentially just an enhancement to allow generating strings from arbitrary char sets.

bararchy commented 4 years ago

@straight-shoota Generating "password" means you are taking responsibility for the security of the user using that, do we really want to do that? there is a difference in conception when talking "random x string" or "random password", also, is this password PCI compliant? is it ISO27001 compliant? we will need to know those answers before taking those responsibilities no?

straight-shoota commented 4 years ago

Let's call it "random x string" then. Or character-set based random string generation. I'd rather have a dedicated feature than people using base64 or hex for this.

straight-shoota commented 4 years ago

I suppose this could also be understood as a mathematical feature for converting a list of integers in one base to a list of integers in another base. It could serve as a generic implementation for any BaseX encoding (there are way more than Base64).

j8r commented 4 years ago

It is a good idea to have a more flexible way to generate random strings. Just not call it password and we are good.

j8r commented 4 years ago

For reference Ruby has SecureRandom.alphanumeric(size).

didactic-drunk commented 4 years ago

Is PCG appropriate for password generation or any CSPRNG use? I don't see any analysis suggesting so, actually the opposite.

"The author admits in the PCG paper [0] that PCG isn't meant to stand up to cryptographic scrutiny and that it hasn't been reviewed as such, while also demonstrating that, by construction, the predictability of PCG "

straight-shoota commented 4 years ago

I don't see how such details about PCG are relevant to this discussion.

didactic-drunk commented 4 years ago

sample uses Random::DEFAULT which is PCG

charset.sample(self)
straight-shoota commented 4 years ago

No, self is passed as argument to sample, so it uses the Random implementation that this method is called on.

ysbaddaden commented 4 years ago

I don't see how such details about PCG are relevant to this discussion.

Your reaction is a perfect demonstration why such a feature is a _very bad idea__. By introducing Random#password you directly encourage to use whatever random source to generate a password, which includes insecure sources such as PCG, the default random generator.

This "detail" can become a security issue. Because such sources are insecure and can be predicted.

The only trusted source to generate passwords MUST be Random::Secure. Maybe ChaCha20 could be used. Certainly NOT the PCG or xorshift families.

The problem is actually the naming. Having generic methods on Random such as #base64 or #hex or #alphanumeric isn't a problem. There is no implied security or anything, unlike #password which MUST be safe, secure & unpredictable.

ysbaddaden commented 4 years ago

And @bararchy is also right: generating a password is complex.

Let's close this, and let a security researcher, or someone working with a security researcher write a passwords shard.

straight-shoota commented 4 years ago

The current proposal is actually about adding an alphanumeric method.

ysbaddaden commented 4 years ago

By the way, Ruby doesn't have much methods on Random itself. Only rand and bytes mostly. The #hex, #base64, #alphanumeric and #uuid (+ #base58 with Rails) methods are only defined for SecureRandom. I believe this is much better for a stdlib.

In Rails, generating ASCII alpha-numeric chars without 0, O, I, l is called #base58. I think it's a better name than #alphanumeric, which could be understood as returning any Unicode alphanumeric, not just ASCII alphanumeric. Ideally, such a method should return characters for a given language.

straight-shoota commented 4 years ago

Moving these methods to Random::Secure is a different issue.

Base58 would be covered by a generic encoding implementation (see https://github.com/crystal-lang/crystal/issues/8993#issuecomment-608103884).

I think the difference between a generic baseX encoder and a basic Random#alphanumeric is that the latter doesn't need decoding, so it's much easier to implement. But a generic baseX encoder is obviously much more versatile and could be helpful for other use cases, too. I have a basic example including an implementation of base58 based on top of it here: https://carc.in/#/r/8w8w

crysbot commented 2 months ago

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/is-this-a-good-way-to-generate-a-random-string/6986/20