Use CryptoRandomBytes instead of CryptoRandomString

[Gusted]

• Switch to use CryptoRandomBytes instead of CryptoRandomString, OAuth's secrets are copied pasted and don't need to avoid dubious characters etc.
• CryptoRandomBytes gives CryptoRandomString gives possible states.
• Add a prefix, such that code scanners can easily grep these in source code.

[Gusted]

I've somewhere confused bits with bytes and some other things. TL;DR math was flawed, updated OP comment + code to reflect correct math.

[wxiaoguang]

With hex.EncodeToString, it makes the final string length=72(34*2+4), it's pretty long.

Could we use base32 encoding (lower & no-padding) to reduce the size?

[zeripath]

CI failure is related.

---

I suspect that 34 is a bit too long - certainly it's a weird length to choose - although I know you're fitting the previous implementation if I had to guess the original length will have been chosen to fit a certain number of characters in the original encoding and to take account for the weirdness of the original algorithm. I agree that maybe we can drop back to a more sensible number of bits.

---

I'm not sure I understand why we need these to be case-insensitive and thus why we can't use base64-no-padding here?

[Gusted]

I'm not sure I understand why we need these to be case-insensitive and thus why we can't use base64-no-padding here?

🤷🏽 I'm a bit allergic to all the base{32,64} going around, so I'd just follow the given advice on base32.

I suspect that 34 is a bit too long - certainly it's a weird length to choose - although I know you're fitting the previous implementation if I had to guess the original length will have been chosen to fit a certain number of characters in the original encoding and to take account for the weirdness of the original algorithm. I agree that maybe we can drop back to a more sensible number of bits.

I wanted to take 32 bits at first, but that would mean that it reduces the possible amount of states than the current implementation, which people will bark about. The only thing that I'd like with this PR is to switch to CryptoRandomBytes instead of CryptoRandomString, so any kind of encoding/bits is welcome as long it doesn't mean it will be easy to predict/brute-force it.

[wxiaoguang]

Actually, base64 doesn't help much. base64 expansion factor = 8/6 = 1.33, base32 = 8/5 = 1.6

So if we have 10 random bytes, then we have 14 base64 chars, and 16 base32 chars, not too much difference.

And it seems that zeripath also agrees to shorten the length of random bytes. I also vote for it. 😊

And I believe the old magic number 44 was not designed or chosen carefully, so we do not need to follow it.

---

(fixed the math expression)

[Gusted]

I've now chosen for 32, which is more sensible amount of bytes, alternatives we can use 24 or 16 to lower the output length.

[6543]

🤖