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

Draft
yorick wants to merge 8 commits from yorick/rushlink:users into master
2 changed files with 43 additions and 12 deletions
Showing only changes of commit 8e949f837b - Show all commits

View File

@ -338,6 +338,9 @@ func (rl *rushlink) authenticateUser(w http.ResponseWriter, r *http.Request, sho
// Authentication failed, return a 401 Unauthorized response // Authentication failed, return a 401 Unauthorized response
w.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) w.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`)
w.WriteHeader(http.StatusUnauthorized) w.WriteHeader(http.StatusUnauthorized)
if err != nil {
log.Printf("authentication failure: %s", err)
}
return nil return nil
} }
return user return user

View File

@ -6,6 +6,7 @@ import (
"encoding/base64" "encoding/base64"
"errors" "errors"
"fmt" "fmt"
"strings"
"time" "time"
"github.com/google/uuid" "github.com/google/uuid"
@ -54,37 +55,64 @@ func Authenticate(db *gorm.DB, username string, password string) (*User, error)
} }
// Compare the hashed password with the provided password // Compare the hashed password with the provided password
if !comparePassword(user.Password, password) { valid, err := comparePassword(user.Password, password)
if err != nil {
return nil, err
}
if !valid {
return nil, errors.New("invalid password") return nil, errors.New("invalid password")
} }
return &user, nil return &user, nil
} }
const (
yorick marked this conversation as resolved
Review

(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
pwdHashSize = 32
pwdParams = "m=65536,t=2,p=1"
pwdAlgo = "argon2id"
)
func HashPassword(password string) (string, error) { func HashPassword(password string) (string, error) {
// Generate a salt for the password hash // Generate a salt for the password hash
salt := make([]byte, 16) salt := make([]byte, pwdSaltSize)
if _, err := rand.Read(salt); err != nil { if _, err := rand.Read(salt); err != nil {
return "", err return "", err
} }
// Hash the password using argon2id // Hash the password using argon2id
hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, 64) hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize)
yorick marked this conversation as resolved
Review

(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)
// Encode the salt and hash as a string // Encode the salt and hash as a string in PHC format
return string(salt) + string(hash), nil encodedSalt := base64.RawStdEncoding.EncodeToString(salt)
yorick marked this conversation as resolved
Review

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).
encodedHash := base64.RawStdEncoding.EncodeToString(hash)
return fmt.Sprintf("$%s$%s$%s$%s", pwdAlgo, pwdParams, encodedSalt, encodedHash), nil
yorick marked this conversation as resolved
Review

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.
} }
func comparePassword(hashedPassword string, password string) bool { func comparePassword(hashedPassword string, password string) (bool, error) {
// Decode the salt and hash from the hashed password string // Extract the salt and hash from the hashed password string
salt := []byte(hashedPassword)[:16] fields := strings.Split(hashedPassword, "$")[1:]
yorick marked this conversation as resolved
Review

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.
hash := []byte(hashedPassword)[16:] if len(fields) != 4 || fields[0] != pwdAlgo || fields[1] != pwdParams {
return false, errors.New("invalid password format in db")
yorick marked this conversation as resolved
Review

(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, encodedHash := fields[2], fields[3]
// Hash the password using the same salt and parameters // Decode the salt and hash from base64
computedHash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, 64) salt, err := base64.RawStdEncoding.DecodeString(encodedSalt)
yorick marked this conversation as resolved
Review

(See comment above on RawURLEncoding)

(See comment above on `RawURLEncoding`)
if err != nil {
return false, err
}
hash, err := base64.RawStdEncoding.DecodeString(encodedHash)
yorick marked this conversation as resolved
Review

(Idem)

(Idem)
if err != nil {
return false, err
}
// Hash the password using the extracted salt and parameters
computedHash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize)
// Compare the computed hash with the stored hash // Compare the computed hash with the stored hash
return bytes.Equal(hash, computedHash) // todo constant time?
return bytes.Equal(hash, computedHash), nil
yorick marked this conversation as resolved
Review

return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil (from crypto/subtle)

`return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil` (from `crypto/subtle`)
} }
// DeleteUser deletes a user with the specified username from the database. // DeleteUser deletes a user with the specified username from the database.