Generate secure token using crypto rand #605
Conversation
| result = string(SYMBOLS[x]) + result | ||
| x, err := rand.Int(rand.Reader, big.NewInt(int64(len(SYMBOLS)))) | ||
| if err != nil { | ||
| log.Fatal("Failed to generate token") |
There was a problem hiding this comment.
we should return an error and gracefully fail the request with a reason to the user
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What you mean? How?
nevermind!
see my last suggestion please :)
|
hello @rumanzo , thanks for the PR, please see my comment |
| result = string(SYMBOLS[x]) + result | ||
| x, err := rand.Int(rand.Reader, big.NewInt(int64(len(SYMBOLS)))) | ||
| if err != nil { | ||
| log.Fatal("Failed to generate token") |
There was a problem hiding this comment.
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!
|
please see my comment in code review |
| 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 { |
There was a problem hiding this comment.
| if err != nil { | |
| if err != nil { | |
| // log an error entry and fallback to `math/rand` |
There was a problem hiding this comment.
I agree, will do it later. Should I pass logger into function?
There was a problem hiding this comment.
yes, thanks
you should pass a null logger in the test: https://github.com/dutchcoders/transfer.sh/blob/main/server/token_test.go
rand package https://pkg.go.dev/math/rand notice us
Package rand implements pseudo-random number generators suitable for tasks such as simulation, but it should not be used for security-sensitive work.I purpose use crypto/rand function that's more secure