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 base32 for 2FA scratch token #18384

Merged
merged 10 commits into from
Jan 26, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jan 24, 2022

Close #6267

Changes:

  1. refactor util.Random* to util.CryptoRandom* to indicate that they are generating crypto/secure random data.
    1. indeed it's different from something like MathRandomInt because it is much slower and more secure.
  2. clarify the comment of CryptoRandomBytes
  3. bytes is an imported package, so the local variables are renamed to buf from bytes
  4. use base32 for 2FA scratch token (only for the scratch token)
    1. the new token is slightly longer, but easier to be written down
    2. the security level doesn't change. 6 * log(256) > 8 * log(62)

Before

image

After

image

@silverwind
Copy link
Member

silverwind commented Jan 24, 2022

I think we should only output non-ambigous readable strings, e.g. exclude 1,l,0,O from the corpus, similar to pwgen -B:

  -B or --ambiguous
	Don't include ambiguous characters in the password

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 24, 2022
@wxiaoguang
Copy link
Contributor Author

I think we should only output non-ambigous readable strings, e.g. exclude 1,l,0,O from the corpus, similar to pwgen -B:

  -B or --ambiguous
	Don't include ambiguous characters in the password

Yes, it is, see the code const base32Chars = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789", I choose customized chars.

@silverwind
Copy link
Member

Ah, I missed that. Looks fine.

Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good btw. Just a few things that we should address.

models/auth/twofactor.go Show resolved Hide resolved
modules/util/util.go Outdated Show resolved Hide resolved
@codecov-commenter

This comment has been minimized.

@lunny
Copy link
Member

lunny commented Jan 25, 2022

I don't think we should change all tokens generation methods. Only those users needs to input but cannot copy/paste should be changed.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jan 25, 2022

@lunny Sorry I didn't get the point. Do you mean this PR is fine or not fine?

This PR only improves the scratch token generation (GenerateScratchToken), such scratch token might need to be written down or printed, see the original issue #6267, and from the screenshot, we can see the new token is much better and clear than before. I didn't change other tokens.

@lunny
Copy link
Member

lunny commented Jan 25, 2022

@lunny Sorry I didn't get the point. Do you mean this PR is fine or not fine?

This PR only improves the scratch token generation (GenerateScratchToken), such scratch token might need to be written down or printed, see the original issue #6267, and from the screenshot, we can see the new token is much better and clear than before. I didn't change other tokens.

It seems you have changed NewToken and UserSalt?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jan 25, 2022

@lunny Sorry I didn't get the point. Do you mean this PR is fine or not fine?
This PR only improves the scratch token generation (GenerateScratchToken), such scratch token might need to be written down or printed, see the original issue #6267, and from the screenshot, we can see the new token is much better and clear than before. I didn't change other tokens.

It seems you have changed NewToken and UserSalt?

Where? That's just a refactoring of RandomString => CryptoRandomString and RandomBytes => CryptoRandomBytes. Nothing else is changed. It has been explained in the issue comment: refactor util.Random* to util.CryptoRandom* to indicate that they are generating crypto/secure random data.

@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 25, 2022
@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 Jan 25, 2022
@wxiaoguang wxiaoguang merged commit 49dd906 into go-gitea:main Jan 26, 2022
@wxiaoguang wxiaoguang deleted the use-base32-otp-scratch branch January 26, 2022 04:10
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Jan 26, 2022
@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Jan 26, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 26, 2022
* giteaofficial/main:
  Use base32 for 2FA scratch token (go-gitea#18384)
  [skip ci] Updated translations via Crowdin
  Fix broken oauth2 authentication source edit page (go-gitea#18412)
  Prevent deadlocks in persistable channel pause test (go-gitea#18410)
  Bump golangci-lint version (go-gitea#18411)
  Unexport git.GlobalCommandArgs (go-gitea#18376)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Use base32 for 2FA scratch token
* rename Secure* to Crypto*, add comments
@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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2FA scratch token includes ambiguous characters
6 participants