-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Use CryptoRandomBytes
instead of CryptoRandomString
#18439
Conversation
- Switch to use `CryptoRandomBytes` instead of `CryptoRandomString`, OAuth's secrets are copied pasted and don't need to avoid dubious characters etc. - `CryptoRandomBytes` gives 44^256 = 5.295 * 10^420 `CryptoRandomString` gives 44^62 = 7.83 * 10^101 possible states.
I've somewhere confused bits with bytes and some other things. TL;DR math was flawed, updated OP comment + code to reflect correct math. |
With Could we use base32 encoding (lower & no-padding) to reduce the size? |
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? |
🤷🏽 I'm a bit allergic to all the base{32,64} going around, so I'd just follow the given advice on base32.
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. |
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) |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
🤖 |
* giteaofficial/main: Use `CryptoRandomBytes` instead of `CryptoRandomString` (go-gitea#18439) Remove the spurious space in the .ui.right additional selector (go-gitea#18605) Ensure commit-statuses box is sized correctly in headers (go-gitea#18538) [skip ci] Updated translations via Crowdin Prevent merge messages from being sorted to the top of email chains (go-gitea#18566) Fix pushing to 1-x-dev docker tag (go-gitea#18578) Replace `sync.Map` with normal maps (go-gitea#18584) Fix oauth docs usage for 2fa (go-gitea#18581) Update .gitattributes for .tmpl files (go-gitea#18576) Prevent panic on prohibited user login with oauth2 (go-gitea#18562) Fix manifest.tmpl (go-gitea#18573) Make docker gitea/gitea:v1.16-dev etc refer to the latest build on that branch (go-gitea#18551) Add dropdown icon to template loading dropdown (go-gitea#18564)
- Switch to use `CryptoRandomBytes` instead of `CryptoRandomString`, OAuth's secrets are copied pasted and don't need to avoid dubious characters etc. - `CryptoRandomBytes` gives ![2^256 = 1.15 * 10^77](https://render.githubusercontent.com/render/math?math=2^256%20=%201.15%20\cdot%2010^77) `CryptoRandomString` gives ![62^44 = 7.33 * 10^78](https://render.githubusercontent.com/render/math?math=62^44%20=%207.33%20\cdot%2010^78) possible states. - Add a prefix, such that code scanners can easily grep these in source code. - 32 Bytes + prefix
CryptoRandomBytes
instead ofCryptoRandomString
, OAuth's secrets are copied pasted and don't need to avoid dubious characters etc.CryptoRandomBytes
givesCryptoRandomString
gives