electricdusk/rushlink
3
3
Fork 3
Code Issues 21 Pull Requests 2 Releases Activity

WIP: Add users system, required for uploading new pastes #77

Draft
yorick wants to merge 8 commits from yorick/rushlink:users into master
pull from: yorick/rushlink:users
merge into: electricdusk:master
Conversation 0 Commits 8 Files Changed 8 +16 -6
1 changed files with 16 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 44e74b5d0c - Show all commits

22
internal/db/user.go
View File

@ -69,11 +69,21 @@ func Authenticate(db *gorm.DB, username string, password string) (*User, error)
const ( const (
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:07:52 +02:00
Review
Copy Link

(I would put these password-related const and funcs in a separate file, because they do nothing with regards to users)

(I would put these password-related `const` and `func`s in a separate file, because they do nothing with regards to users)
pwdSaltSize = 16 pwdSaltSize = 16
pwdHashSize = 32 pwdHashSize = 32
pwdParams = "m=65536,t=2,p=1"
// chosen from https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
pwdMemory = 64 * 1024
pwdThreads = 1
pwdIterations = 2
pwdParams = "m=65536,t=2,p=1,v=19"
pwdVersion = "v=19"
pwdAlgo = "argon2id" pwdAlgo = "argon2id"
) )
func HashPassword(password string) (string, error) { func HashPassword(password string) (string, error) {
if argon2.Version != 19 {
// go has no static asserts
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:45:45 +02:00
Review
Copy Link

(nit: also define the 2, 64*1024, 1 in the var block above, such that you can reuse it in comparePassword; alternatively, create a function createHashFromPasswordAndSalt(password, salt []byte) []byte that does this for you... also specifying why you chose these parameters)

(nit: also define the `2`, `64*1024`, `1` in the `var` block above, such that you can reuse it in `comparePassword`; alternatively, create a function `createHashFromPasswordAndSalt(password, salt []byte) []byte` that does this for you... also specifying why you chose these parameters)
panic("Unexpected argon2 version")
}
// Generate a salt for the password hash // Generate a salt for the password hash
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:23:20 +02:00
Review
Copy Link

According to the PHC format specification, padding characters are omitted. base64.URLEncoding includes padding characters. Use base64.RawURLEncoding instead.

(Technically speaking, you should use base64.RawStdEncoding, because that follows the exact alphabet as the specification. If you decide that base64.URLEncoding (or base64.RawURLEncoding) also works, then make it clear in the comment that you deviate from the specification).

According to the [PHC format specification](https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md#b64), padding characters are omitted. `base64.URLEncoding` includes padding characters. Use `base64.RawURLEncoding` instead. (Technically speaking, you should use `base64.RawStdEncoding`, because that follows the exact alphabet as the specification. If you decide that `base64.URLEncoding` (or `base64.RawURLEncoding`) also works, then make it clear in the comment that you deviate from the specification).
salt := make([]byte, pwdSaltSize) salt := make([]byte, pwdSaltSize)
if _, err := rand.Read(salt); err != nil { if _, err := rand.Read(salt); err != nil {
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:18:04 +02:00
Review
Copy Link

Is there a specific reason you omitted the (optional) field version? The argon2 package defines one, so it seems a good idea to include that as well.

Is there a specific reason you omitted the (optional) field `version`? The `argon2` package [defines one](https://pkg.go.dev/golang.org/x/crypto/argon2#Version), so it seems a good idea to include that as well.
@ -81,22 +91,22 @@ func HashPassword(password string) (string, error) {
} }
// Hash the password using argon2id // Hash the password using argon2id
hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize) hash := argon2.IDKey([]byte(password), salt, pwdIterations, pwdMemory, pwdThreads, pwdHashSize)
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:15:34 +02:00
Review
Copy Link

It's not safe to do a slice expression that requires at least two returned elements on a function that may return an empty slice, or a slice of length one.

It's not safe to do a slice expression that requires at least two returned elements on a function that may return an empty slice, or a slice of length one.
// Encode the salt and hash as a string in PHC format // Encode the salt and hash as a string in PHC format
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:31:09 +02:00
Review
Copy Link

(another option: add var errInvalidDBPasswordFormat = errors.New("invalid password format in db") in the global scope, and return that here. Works nice with `errors.Is(err, errInvalidDBPasswordFormat), as I suppose we want to log such occurrences)

(another option: add `var errInvalidDBPasswordFormat = errors.New("invalid password format in db")` in the global scope, and return that here. Works nice with `errors.Is(err, errInvalidDBPasswordFormat), as I suppose we want to log such occurrences)
encodedSalt := base64.RawStdEncoding.EncodeToString(salt) encodedSalt := base64.RawStdEncoding.EncodeToString(salt)
encodedHash := base64.RawStdEncoding.EncodeToString(hash) encodedHash := base64.RawStdEncoding.EncodeToString(hash)
return fmt.Sprintf("$%s$%s$%s$%s", pwdAlgo, pwdParams, encodedSalt, encodedHash), nil return fmt.Sprintf("$%s$%s$%s$%s$%s", pwdAlgo, pwdVersion, pwdParams, encodedSalt, encodedHash), nil
} }
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:23:49 +02:00
Review
Copy Link

(See comment above on RawURLEncoding)

(See comment above on `RawURLEncoding`)
var errInvalidDBPasswordFormat = errors.New("invalid password format in db") var errInvalidDBPasswordFormat = errors.New("invalid password format in db")
func comparePassword(hashedPassword string, password string) (bool, error) { func comparePassword(hashedPassword string, password string) (bool, error) {
// Extract the salt and hash from the hashed password string // Extract the salt and hash from the hashed password string
fields := strings.Split(hashedPassword, "$") fields := strings.Split(hashedPassword, "$")
yorick marked this conversation as resolved
mrngm commented 2023-04-30 20:31:46 +02:00
Review
Copy Link

(Idem)

(Idem)
if len(fields) != 5 || fields[1] != pwdAlgo || fields[2] != pwdParams { if len(fields) != 6 || fields[1] != pwdAlgo || fields[2] != pwdVersion || fields[3] != pwdParams {
return false, errInvalidDBPasswordFormat return false, errInvalidDBPasswordFormat
} }
encodedSalt, encodedHash := fields[3], fields[4] encodedSalt, encodedHash := fields[4], fields[5]
// Decode the salt and hash from base64 // Decode the salt and hash from base64
salt, err := base64.RawStdEncoding.DecodeString(encodedSalt) salt, err := base64.RawStdEncoding.DecodeString(encodedSalt)
@ -109,7 +119,7 @@ func comparePassword(hashedPassword string, password string) (bool, error) {
} }
// Hash the password using the extracted salt and parameters // Hash the password using the extracted salt and parameters
computedHash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize) computedHash := argon2.IDKey([]byte(password), salt, pwdIterations, pwdMemory, pwdThreads, pwdHashSize)
// Compare the computed hash with the stored hash // Compare the computed hash with the stored hash
return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil