Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature request] entropy & logic based password strength requirements #47

Open
Oracuda opened this issue Mar 4, 2024 · 15 comments · May be fixed by #65
Open

[feature request] entropy & logic based password strength requirements #47

Oracuda opened this issue Mar 4, 2024 · 15 comments · May be fixed by #65

Comments

@Oracuda
Copy link

Oracuda commented Mar 4, 2024

It is often incredibly frustrating for users to be told that their commonly used password isn't strong enough, so I suggest discuit adds something like https://github.com/dropbox/zxcvbn or https://github.com/wagslane/go-password-validator to calculate the strength of passwords.

  • zxcvbn calculates password strength based upon commonly used words and repeating patterns, has go bindings via https://github.com/nbutton23/zxcvbn-go aswell as c++ which can be compiled to wasm if necassary, but that's probably OTT for this use case.
  • go-password-validator calculated password strength based upon the password's calculated entropy

A combination could probably be inplemented and Site Administrators could use the config file to change the requirements for passwords, and which algorithms are used.

passwordRequirements:
  zxcvbn:
    min_score: 4
  go-password-validator:
    min_entropy: 60
@noClaps
Copy link
Contributor

noClaps commented Mar 14, 2024

Alternatively, the password can be hashed and salted when it's input on the client so that it doesn't matter what the password is. There would be no way to determine the user's credentials from the server, and the responsibility of security then lies with the user to have good passwords to avoid them being guessed.

@Codycody31
Copy link
Contributor

Alternatively, the password can be hashed and salted when it's input on the client so that it doesn't matter what the password is. There would be no way to determine the user's credentials from the server, and the responsibility of security then lies with the user to have good passwords to avoid them being guessed.

I believe this would bring unnecessary work. You would need to let the client know how many rounds the password was hashed for them to correctly hash the password and then send it. Another thing is we should never trust what a user gives us, and the server should be the one the hash and salt the password to ensure the client isn't using some outdated algorithm or something else that could cause trouble.

@noClaps
Copy link
Contributor

noClaps commented Mar 14, 2024

That's true, and something that the clients and server devs would need to figure out. However, I don't think hashing on the server makes sense for security, since the credentials could be intercepted before they even reach the server. Hashing on the client makes for greater security and less hassle to the user since they no longer have to worry about some arbitrary password strength requirements.

There's also the option of passkey login, which would (in my opinion) be the best implementation. It would take some time to get everyone to switch over though, and I don't think Firefox supports passkeys yet so that's also an issue.

Edit: Actually, it seems that Firefox recently added passkey support on desktop. There's still no mobile support though.
https://caniuse.com/passkeys. I think we could offer this as an option to users that want to use it, but not make it mandatory for those who don't/can't.

@Codycody31
Copy link
Contributor

Codycody31 commented Mar 14, 2024

Supporting passkeys would be nice. However, what you are talking about is a man-in-the-middle attack which normally can only really occur on HTTP traffic due to it being unencrypted, which shouldn't even be supported for a site like this. Another thing is that password strength requirements would still need to be met, as we would want to at least enforce some kind of proper password rather than someone using say 123456 or qwerty12, etc. ALSO if we are worried about an MITM then the authentication sent via the client to the server could also just allow for session hijacking if they are capable of grabbing the password sent on signing or a cookie sent with it.

@noClaps
Copy link
Contributor

noClaps commented Mar 14, 2024

authentication sent via the client to the server could also just allow for session hijacking if they are capable of grabbing the password sent on signing or a cookie sent with it.

Isn't that possible right now anyway? The passwords are sent as part of the request body when calling the login or signup endpoints.

@Codycody31
Copy link
Contributor

I don't believe so. The site uses https and as long as the password is in a post request everything should be encrypted via SSL or TLS ( I don't know which)

@noClaps
Copy link
Contributor

noClaps commented Mar 14, 2024

It is in a POST request. The cookie only stores the CSRF token (at least on the official client), and from what I can tell, the password isn't stored anywhere. The server also checks if the user is already logged in (though I'm not sure how it verifies that), which means that check isn't done on the client either.

I did some research and I think you're right. Hashing the password client side doesn't make much sense if the password is sent over HTTPS. It seems like the password is hashed on the server already:

discuit/core/user.go

Lines 433 to 436 in ab514cf

hash, err := HashPassword([]byte(password))
if err != nil {
return nil, err
}

so that's not a concern either.

I suppose the password strength requirement check makes sense if we're trying to encourage strong passwords, but it ends up being annoying more often than not.

@Codycody31
Copy link
Contributor

Yeah, while a password strength may be annoying. It is important for a user to have a better password rather then the default ones people use like 1234566, etc lol

@imbev
Copy link

imbev commented Mar 21, 2024

https://xkcd.com/936/

Whichever password strength checking implementation is chosen, I strongly suggest that it doesn't specifically require special characters, numbers, etc. A generic entropy or length check is much preferred.

@Codycody31 Codycody31 linked a pull request Apr 19, 2024 that will close this issue
@previnder
Copy link
Member

It is in a POST request. The cookie only stores the CSRF token (at least on the official client), and from what I can tell, the password isn't stored anywhere. The server also checks if the user is already logged in (though I'm not sure how it verifies that), which means that check isn't done on the client either.

If a user id is present in the session of the user—the data is stored in Redis—then the user is considered to be logged in (the SID cookie is matched against a key-set in Redis to find the session of the user). Take a look at the sessions package for details on the implementation.

Yes, the password is properly hashed and salted in the server before saving. And there's no point at all in doing that client-side if the password is sent over an unencrypted connection, as anyone in the middle of the connection would still be able to read the hashed password and get access to the account. The password must always be sent over an encrypted connection.

@previnder
Copy link
Member

I'm inclined to leave the password requirements as they are; that is, to keep only the minimum length requirement that exists now. Reasons being:

  1. Discuit is not a security critical service; it's an anonymous forum platform/software. The value of an account, therefore, is much less than that of a banking service or email. Because of this, some of the users treat their account as a fairly unimportant thing, and this makes it less likely that they'll care to store their password somewhere or remember it. And so password strength requirements would mean more users losing access to their account at some point.
  2. There should be the option for someone to choose a short, easy to remember password if that's what they wanted.
  3. I find it condescending when sites have weird password rules or when they enforce 2FA. I use a password manager and I'm careful about my security—when I care about it—and these rules and policies sometimes actively work against good security practices.

@noClaps
Copy link
Contributor

noClaps commented Apr 23, 2024

Well, we could still provide a password strength indicator. It doesn't have to be enforced, so people can still keep weak passwords if they want to, but having the indicator there would encourage good security practices for the people that like seeing that password strength bar go green.

@previnder
Copy link
Member

That's an excellent suggestion, @noClaps. I love it.

@Codycody31
Copy link
Contributor

now that I have properly taken the time to read through this, I think I misinterpreted what noClaps said. I only updated my PR to make it optional to entirely disable the entropy logic. Maybe in another PR, we could have a button like Check Password Strength or have it automatically check it against something, and add an API endpoint so users can see the cool strength bar based on the entropy score.

@noClaps
Copy link
Contributor

noClaps commented Apr 23, 2024

I was more so imagining a password strength bar like other websites have. Although that could probably just be implemented client side, since the server accepts the password regardless of its strength, as long as it meets the length requirements. I don't think having an extra click makes sense for it since most people aren't going to click on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants