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

Update minimum password length requirements #25946

Merged
merged 6 commits into from Aug 21, 2023

Conversation

techknowlogick
Copy link
Member

No description provided.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 18, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2023
@techknowlogick techknowlogick added backport/v1.20 This PR should be backported to Gitea 1.20 type/bug labels Jul 18, 2023
@wxiaoguang
Copy link
Contributor

I do not think it really makes sense, for example: 12345678 perfectly matches MIN_PASSWORD_LENGTH = 8

To make the password secure, it needs more rules.

@KN4CK3R
Copy link
Member

KN4CK3R commented Jul 18, 2023

Rules aren't really helping. We could add a "password strength meter" like https://github.com/dropbox/zxcvbn to show how secure a password is.

Reasonable Password Policy Example

  • Passwords must be at between 12 and 4,096 characters in length.
  • Passwords can contain any characters (including Unicode).
  • We strongly encourage the use of a password manager to generate and store your passwords.
  • Your password strength must be at least level 3 (on the 0-4 scale).

That's it. Don't tell people what their password can or cannot contain. Don't refuse longer passwords. Do stop people from shooting themselves in the foot, but don't interfere beyond what's necessary to prevent foot-bullets.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 18, 2023

IMO the "password strength meter" is also "rule-based" (scoring rules, date rules, variation rules, etc) 🤣

And one more thing, even if some passwords are likely safe, but they are already in the password dict, so sometimes pwned-check is also necessary (well, it depends ....)

But I think making the password checking too complex is not in Gitea's scope, in most cases, SSO and 2FA are better solutions.

@KN4CK3R
Copy link
Member

KN4CK3R commented Jul 18, 2023

Yes, just want to prevent rules like "must contain 2 lower, 3 middle and 4 upper case letters". Then as user you think "wtf are middle letters?".

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 18, 2023

There are already some simple builtin rules:

;;Comma separated list of character classes required to pass minimum complexity.
;;If left empty or no valid values are specified, the default is off (no checking)
;;Classes include "lower,upper,digit,spec"
;PASSWORD_COMPLEXITY = off

And even:

;; Validate against https://haveibeenpwned.com/Passwords to see if a password has been exposed
;PASSWORD_CHECK_PWN = false

I think that's enough for daily usage (with 2FA).

The real problem is: make users set the values they like during their installation ......

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 18, 2023
@jolheiser
Copy link
Member

There are already some simple builtin rules:

;;Comma separated list of character classes required to pass minimum complexity.
;;If left empty or no valid values are specified, the default is off (no checking)
;;Classes include "lower,upper,digit,spec"
;PASSWORD_COMPLEXITY = off

And even:

;; Validate against https://haveibeenpwned.com/Passwords to see if a password has been exposed
;PASSWORD_CHECK_PWN = false

I think that's enough for daily usage (with 2FA).

The real problem is: make users set the values they like during their installation ......

In general I agree with this, however not every user sees an installation page. Many deploy from packages or other "auto" installations that provide a minimal working config and perhaps don't check it.

I also very much agree that 12345678 isn't secure, but it's arguably "better:tm:" than 123456, but that boils down to a human/user problem. I think increasing minimum length, while not perfect, is likely better on average than "complexity".

(Obligatory I am not a security expert 😅)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 19, 2023
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do more later

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 29, 2023
@silverwind silverwind changed the title Update minimum length requirements Update minimum password length requirements Jul 29, 2023
@silverwind
Copy link
Member

e2e test failure may be related.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2023
@techknowlogick
Copy link
Member Author

@silverwind thanks :) I've updated e2e tests, and they are now passing.

@techknowlogick techknowlogick enabled auto-merge (squash) August 21, 2023 18:53
@techknowlogick techknowlogick added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed backport/v1.20 This PR should be backported to Gitea 1.20 labels Aug 21, 2023
@techknowlogick techknowlogick merged commit b3f7137 into go-gitea:main Aug 21, 2023
24 checks passed
@GiteaBot GiteaBot added this to the 1.21.0 milestone Aug 21, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 21, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 22, 2023
* giteaofficial/main: (21 commits)
  Update minimum password length requirements (go-gitea#25946)
  cynkra is covered via oc links now (go-gitea#26641)
  update config docs url (go-gitea#26640)
  devpod use go1.21 (go-gitea#26637)
  Use correct minio error (go-gitea#26634)
  Remove avatarHTML from template helpers (go-gitea#26598)
  Add optimistic lock to ActionRun table (go-gitea#26563)
  Improve the branch selector tab UI (go-gitea#26631)
  Improve translation of milestone filters (go-gitea#26569)
  Add `branch_filter` to hooks API endpoints (go-gitea#26599)
  Replace box-shadow for `floating` dropdown as well (go-gitea#26581)
  Add link to job details and tooltip to commit status in repo list in dashboard (go-gitea#26326)
  Ignore the trailing slashes when comparing oauth2 redirect_uri (go-gitea#26597)
  Update tool dependencies (go-gitea#26607)
  bump go to 1.21 (go-gitea#26608)
  Update 1.20.3 changelog (go-gitea#26609)
  Fix NPM packages name validation (go-gitea#26595)
  Use "input" event instead of "keyup" event for migration form (go-gitea#26602)
  Do not use deprecated log config options by default (go-gitea#26592)
  fix reopen logic for agit flow pull request (go-gitea#26399)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants