Skip to content

Generate secure token using crypto rand #605

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Generate secure token using crypto rand #605

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ca5c89f
Generate secure token using crypto rand
Mar 6, 2024
27c38e6
simplier and faster casting random symbols
Mar 7, 2024
6fc33ad
Merge branch 'main' into tokensecure
Apr 11, 2024
e069bd2
Add fallback to math/rand instead of crypto/rand. Bypass logger to to…
Jun 3, 2024
0fbe374
Merge branch 'tokensecure' of github.com:rumanzo/transfer.sh into tok…
Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions server/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ THE SOFTWARE.
package server

import (
"math/rand"
"crypto/rand"
"log"
"math/big"
)

const (
Expand All @@ -37,8 +39,11 @@ const (
func token(length int) string {
result := ""
for i := 0; i < length; i++ {
x := rand.Intn(len(SYMBOLS) - 1)
result = string(SYMBOLS[x]) + result
x, err := rand.Int(rand.Reader, big.NewInt(int64(len(SYMBOLS))))
if err != nil {
Copy link
Collaborator

@paolafrancesca paolafrancesca Apr 11, 2024

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
if err != nil {
// log an error entry and fallback to `math/rand`

Copy link
Contributor Author

@rumanzo rumanzo Apr 11, 2024

Choose a reason for hiding this comment

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

I agree, will do it later. Should I pass logger into function?

Copy link
Collaborator

@paolafrancesca paolafrancesca Apr 11, 2024

Choose a reason for hiding this comment

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

yes, thanks

you should pass a null logger in the test: https://github.com/dutchcoders/transfer.sh/blob/main/server/token_test.go

Copy link
Contributor Author

@rumanzo rumanzo Jun 3, 2024

Choose a reason for hiding this comment

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

I did it. But error message in logger in this function provide three error messages in log, because token calls several times.

Copy link
Contributor Author

@rumanzo rumanzo Jun 14, 2024

Choose a reason for hiding this comment

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

@aspacca check pls

log.Fatal("Failed to generate token")
Copy link
Collaborator

@paolafrancesca paolafrancesca Mar 7, 2024

Choose a reason for hiding this comment

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

we should return an error and gracefully fail the request with a reason to the user

Copy link
Contributor Author

@rumanzo rumanzo Mar 7, 2024

Choose a reason for hiding this comment

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

Can we just ignore err in this case?
I really doubt that we can get error in this case ever using rand.Reader
https://cs.opensource.google/go/go/+/master:src/crypto/rand/util.go;l=85
And we defenetly won't panic there

Copy link
Collaborator

@paolafrancesca paolafrancesca Mar 7, 2024

Choose a reason for hiding this comment

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

digging further, that's on unix system:
https://cs.opensource.google/go/go/+/master:src/crypto/rand/rand_unix.go;l=62
https://cs.opensource.google/go/go/+/master:src/crypto/rand/rand_unix.go;l=72

It's not excluded 100% that they won't return an error.

Indeed I guess that on our docker image (that's stratch based) the first will always return an error: do you mind trying to build the docker image from your branch and test?

thanks!

Copy link
Contributor Author

@rumanzo rumanzo Mar 19, 2024

Choose a reason for hiding this comment

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

Indeed I guess that on our docker image (that's stratch based) the first will always return an error: do you mind trying to build the docker image from your branch and test?
I tried run scratch image with dropped all capabilites, and it works. But i can do something like fallback

Copy link
Collaborator

@paolafrancesca paolafrancesca Mar 20, 2024

Choose a reason for hiding this comment

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

I can reproduce and this seems to be the reason:

// On Linux, FreeBSD, Dragonfly, NetBSD and Solaris, Reader uses getrandom(2) if
// available, /dev/urandom otherwise.

but, please, note:

// On other Unix-like systems, Reader reads from /dev/urandom.

so I guess on other Unix-like systems it won't work.

not sure how to test

Copy link
Contributor Author

@rumanzo rumanzo Mar 20, 2024

Choose a reason for hiding this comment

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

I can reproduce and this seems to be the reason:
how?

so I guess on other Unix-like systems it won't work.
on other unix-like system should be /dev/urandom, but what another unix-like system? HP-UX may be?

Copy link
Collaborator

@paolafrancesca paolafrancesca Mar 29, 2024

Choose a reason for hiding this comment

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

I can reproduce and this seems to be the reason: how?

so I guess on other Unix-like systems it won't work. on other unix-like system should be /dev/urandom, but what another unix-like system? HP-UX may be?

let's check the error and panic with a message with the link to this PR

Copy link
Contributor Author

@rumanzo rumanzo Apr 8, 2024

Choose a reason for hiding this comment

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

I can reproduce and this seems to be the reason: how?
so I guess on other Unix-like systems it won't work. on other unix-like system should be /dev/urandom, but what another unix-like system? HP-UX may be?

let's check the error and panic with a message with the link to this PR

What you mean? How?

Copy link
Collaborator

@paolafrancesca paolafrancesca Apr 11, 2024

Choose a reason for hiding this comment

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

What you mean? How?

nevermind!

see my last suggestion please :)

}
result = string(SYMBOLS[x.Int64()]) + result
}

return result
Expand Down