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

Draft
yorick wants to merge 8 commits from yorick/rushlink:users into master
First-time contributor

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:

  • should be fool-proof
  • high degree of usability
  • easy to implement
  • allows users to view/delete their own pastes

Cons:

  • collects and stores personal data (username)
  • requires admin intervention to create new accounts
## Alternatives

A combination of these tools might also be possible.

ssl client certs

  • Stores less information, but is harder to use

proof-of-work

  • Loses compatibility with curl and other shortener interfaces
  • Malicious actors can still do this, although they're less likely to bother

TODO's

  • check sensitive code (password hashing, user id generation)
  • improve error handling and reporting
  • delete user data (username, password) upon deletion
  • implement paste view and deletion for users
  • improve code, since I don't know Go
  • Create a UI
  • Should FileUploads also have a CreatedBy?
## 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](https://transparencyreport.google.com/safe-browsing/search?url=https:%2F%2Fhashru.link%2F) 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:** - should be fool-proof - high degree of usability - easy to implement - allows users to view/delete their own pastes **Cons:** - collects and stores personal data (username) - requires admin intervention to create new accounts <details><summary> ## Alternatives </summary> A combination of these tools might also be possible. ### ssl client certs - Stores less information, but is harder to use ### proof-of-work - Loses compatibility with curl and other shortener interfaces - Malicious actors can still do this, although they're less likely to bother </details> ## TODO's - [ ] check sensitive code (password hashing, user id generation) - [ ] improve error handling and reporting - [ ] delete user data (username, password) upon deletion - [ ] implement paste view and deletion for users - [ ] improve code, since I don't know Go - [ ] Create a UI - [ ] Should FileUploads also have a CreatedBy?
yorick added 1 commit 2023-04-30 12:20:12 +02:00
yorick added 2 commits 2023-04-30 18:13:23 +02:00
yorick added 1 commit 2023-04-30 18:19:39 +02:00
mrngm requested changes 2023-04-30 20:47:47 +02:00
@ -32,6 +32,10 @@ func main() {
log.Fatal(errors.Wrap(err, "migrating database"))
}
if err := db.CreateAdminUser(database, "admin"); err != nil {
Collaborator

I would be fine with a command line option that allows the operator to create users.

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)
Collaborator

(not go fmted)

(not `go fmt`ed)
yorick marked this conversation as resolved
@ -276,0 +294,4 @@
return
}
}
func (rl *rushlink) createUserHandler(w http.ResponseWriter, r *http.Request) {
Collaborator

(I would be fine with leaving this out of the HTTP part)

(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"))
Collaborator

Missing err check

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)) {
Collaborator

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:

func (rl *rushlink) setWWWAuthenticate(w http.ResponseWriter, r *http.Request) {
	// Set authentication headers for Basic Authentication
	w.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`)
	w.WriteHeader(http.StatusUnauthorized)
}

and then implement the logic for authenticating the user

user, err := db.Authenticate(rl.db, username, password)
if err != nil {
	log.Printf("authentication failure: %v", err)
	rl.setWWWAuthenticate(w, r)
	return nil
}

if shouldBeAdmin && !user.Admin {
	log.Printf("user should be admin, but isn't: %v", err)
	rl.setWWWAuthenticate(w, r)
	return nil
}

// TODO: canAlsoBe?

// User has been authenticated

return 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: ```go func (rl *rushlink) setWWWAuthenticate(w http.ResponseWriter, r *http.Request) { // Set authentication headers for Basic Authentication w.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) w.WriteHeader(http.StatusUnauthorized) } ``` and then implement the logic for authenticating the user ```go user, err := db.Authenticate(rl.db, username, password) if err != nil { log.Printf("authentication failure: %v", err) rl.setWWWAuthenticate(w, r) return nil } if shouldBeAdmin && !user.Admin { log.Printf("user should be admin, but isn't: %v", err) rl.setWWWAuthenticate(w, r) return nil } // TODO: canAlsoBe? // User has been authenticated return user ```
yorick marked this conversation as resolved
@ -39,2 +39,4 @@
PubID uuid.UUID `gorm:"uniqueIndex"`
// User ID that created this file
CreatedBy uint `gorm:"index"`
Collaborator

(not go fmted)

(not `go fmt`ed)
yorick marked this conversation as resolved
@ -0,0 +66,4 @@
return &user, nil
}
const (
Collaborator

(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)
yorick marked this conversation as resolved
@ -0,0 +81,4 @@
}
// Hash the password using argon2id
hash := argon2.IDKey([]byte(password), salt, 2, 64*1024, 1, pwdHashSize)
Collaborator

(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)
yorick marked this conversation as resolved
@ -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)
Collaborator

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).
yorick marked this conversation as resolved
@ -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
Collaborator

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.
yorick marked this conversation as resolved
@ -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:]
Collaborator

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.
yorick marked this conversation as resolved
@ -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")
Collaborator

(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)
yorick marked this conversation as resolved
@ -0,0 +98,4 @@
encodedSalt, encodedHash := fields[2], fields[3]
// Decode the salt and hash from base64
salt, err := base64.URLEncoding.DecodeString(encodedSalt)
Collaborator

(See comment above on RawURLEncoding)

(See comment above on `RawURLEncoding`)
yorick marked this conversation as resolved
@ -0,0 +102,4 @@
if err != nil {
return false, err
}
hash, err := base64.URLEncoding.DecodeString(encodedHash)
Collaborator

(Idem)

(Idem)
yorick marked this conversation as resolved
@ -0,0 +112,4 @@
// Compare the computed hash with the stored hash
// todo constant time?
return bytes.Equal(hash, computedHash), nil
Collaborator

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

`return subtle.ConstantTimeCompare(hash, computedHash) == 1, nil` (from `crypto/subtle`)
yorick marked this conversation as resolved
@ -0,0 +141,4 @@
// Update the user fields
existingUser.User = updatedUser.User
existingUser.Password = updatedUser.Password
existingUser.Admin = updatedUser.Admin
Collaborator

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.

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.
yorick force-pushed users from e8fdc75183 to 0643176ed1 2023-04-30 21:00:03 +02:00 Compare
yorick added 1 commit 2023-04-30 21:06:36 +02:00
yorick added 1 commit 2023-04-30 21:15:48 +02:00
yorick added 1 commit 2023-04-30 21:21:16 +02:00
yorick force-pushed users from 588725b235 to 44e74b5d0c 2023-04-30 21:33:51 +02:00 Compare
yorick added 1 commit 2023-04-30 21:39:43 +02:00
This pull request is marked as a work in progress.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u users:yorick-users
git checkout yorick-users
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: electricdusk/rushlink#77
No description provided.