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

Use CryptoRandomBytes instead of CryptoRandomString #18439

Merged
merged 16 commits into from
Feb 4, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Jan 28, 2022

  • 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 CryptoRandomString gives 62^44 = 7.33 * 10^78 possible states.
  • Add a prefix, such that code scanners can easily grep these in source code.

- 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.
@Gusted Gusted added the type/enhancement An improvement of existing functionality label Jan 28, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jan 28, 2022
models/auth/oauth2.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 28, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Jan 28, 2022

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

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 28, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 28, 2022

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?

@6543 6543 added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Jan 30, 2022
@zeripath
Copy link
Contributor

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
Copy link
Contributor Author

Gusted commented Jan 30, 2022

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
Copy link
Contributor

wxiaoguang commented Jan 30, 2022

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
Copy link
Contributor Author

Gusted commented Jan 30, 2022

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

This comment was marked as resolved.

@6543

This comment was marked as resolved.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 3, 2022
@6543
Copy link
Member

6543 commented Feb 4, 2022

🤖

@6543 6543 merged commit aa23f47 into go-gitea:main Feb 4, 2022
@Gusted Gusted deleted the use-CryptoRandomBytes branch February 4, 2022 21:27
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 5, 2022
* 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)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
- 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
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants