WIP: Add users system, required for uploading new pastes #77
No reviewers
Labels
No Label
bug
feature
good-beginner-bug
needs-test
question
wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: electricdusk/rushlink#77
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "yorick/rushlink:users"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem statement
Malicious actors have been using rushlink for their evil machinations for a while now. This didn't impact us directly. Recently, google started listing rushlink as an unsafe website. They're not wrong.
To resolve this, we should delete the malicious pastes and prevent people from making new ones.
Proposed solution
The easiest way to do this is with a user system.
Pros:
Cons:
## Alternatives
A combination of these tools might also be possible.
ssl client certs
proof-of-work
TODO's
@ -32,6 +32,10 @@ func main() {
log.Fatal(errors.Wrap(err, "migrating database"))
}
if err := db.CreateAdminUser(database, "admin"); err != nil {
I would be fine with a command line option that allows the operator to create users.
@ -33,2 +33,4 @@
}
if err := db.CreateAdminUser(database, "admin"); err != nil {
log.Fatalln(err)
(not
go fmt
ed)@ -276,0 +294,4 @@
return
}
}
func (rl *rushlink) createUserHandler(w http.ResponseWriter, r *http.Request) {
(I would be fine with leaving this out of the HTTP part)
@ -278,0 +309,4 @@
new_username := r.FormValue("username")
new_password := r.FormValue("password")
new_admin, _ := strconv.ParseBool(r.FormValue("admin"))
Missing
err
check@ -278,0 +334,4 @@
// Authenticate the user
user, err := db.Authenticate(rl.db, username, password)
if err != nil || (shouldBeAdmin && !user.Admin && (canAlsoBe == nil || *canAlsoBe != user.User)) {
This could be correct, but it's not exactly readable. It's not clear what
canAlsoBe
means in this context, and what kind of values it should represent.It's perhaps better to add a function like:
and then implement the logic for authenticating the user
@ -39,2 +39,4 @@
PubID uuid.UUID `gorm:"uniqueIndex"`
// User ID that created this file
CreatedBy uint `gorm:"index"`
(not
go fmt
ed)@ -0,0 +66,4 @@
return &user, nil
}
const (
(I would put these password-related
const
andfunc
s in a separate file, because they do nothing with regards to users)@ -0,0 +81,4 @@
}
// Hash the password using argon2id
hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize)
(nit: also define the
2
,64*1024
,1
in thevar
block above, such that you can reuse it incomparePassword
; alternatively, create a functioncreateHashFromPasswordAndSalt(password, salt []byte) []byte
that does this for you... also specifying why you chose these parameters)@ -0,0 +84,4 @@
hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize)
// Encode the salt and hash as a string in PHC format
encodedSalt := base64.URLEncoding.EncodeToString(salt)
According to the PHC format specification, padding characters are omitted.
base64.URLEncoding
includes padding characters. Usebase64.RawURLEncoding
instead.(Technically speaking, you should use
base64.RawStdEncoding
, because that follows the exact alphabet as the specification. If you decide thatbase64.URLEncoding
(orbase64.RawURLEncoding
) also works, then make it clear in the comment that you deviate from the specification).@ -0,0 +86,4 @@
// Encode the salt and hash as a string in PHC format
encodedSalt := base64.URLEncoding.EncodeToString(salt)
encodedHash := base64.URLEncoding.EncodeToString(hash)
return fmt.Sprintf("$%s$%s$%s$%s", pwdAlgo, pwdParams, encodedSalt, encodedHash), nil
Is there a specific reason you omitted the (optional) field
version
? Theargon2
package defines one, so it seems a good idea to include that as well.@ -0,0 +91,4 @@
func comparePassword(hashedPassword string, password string) (bool, error) {
// Extract the salt and hash from the hashed password string
fields := strings.Split(hashedPassword, "$")[1:]
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.
@ -0,0 +93,4 @@
// Extract the salt and hash from the hashed password string
fields := strings.Split(hashedPassword, "$")[1:]
if len(fields) != 4 || fields[0] != pwdAlgo || fields[1] != pwdParams {
return false, errors.New("invalid password format in db")
(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)@ -0,0 +98,4 @@
encodedSalt, encodedHash := fields[2], fields[3]
// Decode the salt and hash from base64
salt, err := base64.URLEncoding.DecodeString(encodedSalt)
(See comment above on
RawURLEncoding
)@ -0,0 +102,4 @@
if err != nil {
return false, err
}
hash, err := base64.URLEncoding.DecodeString(encodedHash)
(Idem)
@ -0,0 +112,4 @@
// Compare the computed hash with the stored hash
// todo constant time?
return bytes.Equal(hash, computedHash), nil
return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil
(fromcrypto/subtle
)@ -0,0 +141,4 @@
// Update the user fields
existingUser.User = updatedUser.User
existingUser.Password = updatedUser.Password
existingUser.Admin = updatedUser.Admin
I find it somewhat risky that this function may update the admin status. I would prefer a
SetUserAdminStatus(db *gorm.DB, username string, isAdmin bool) error
that only changes that bit. It also simplifies the code a bit that handles changing the user.e8fdc75183
to0643176ed1
588725b235
to44e74b5d0c
Checkout
From your project repository, check out a new branch and test the changes.