Page MenuHomePhabricator

Intended use of MinimumPasswordLengthToLogin not so clear
Closed, ResolvedPublic

Description

Yesterday https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ was deployed to raise the password requirement for "privileged users" to 10 characters, and have MW suggest that they change the password if it doesn't meet this policy.

It was reported to security@ that a user was then unable to login, or re-auth (during an active session) with their current password. Which meant they were unable to change their password

Something with the suggestChangeOnLogin workflow looks like it might be funky and not working as expected. It seems more like forceChange, but they weren't been given the password change form either

534707 was reverted in https://gerrit.wikimedia.org/r/537474 and deployed to mitigate in the short term

Event Timeline

Reedy created this task.Sep 17 2019, 4:11 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptSep 17 2019, 4:11 PM

Ping T211621 and CC author of the patch and reviewers

Reedy triaged this task as High priority.Sep 17 2019, 4:20 PM
jrbs added a subscriber: jrbs.Sep 17 2019, 4:20 PM

We had a similar email sent to Trust-and-Safety the other day related to this as well.

jrbs moved this task from Backlog to Other team on the Trust-and-Safety board.Sep 17 2019, 4:21 PM
Reedy added a comment.Sep 17 2019, 4:21 PM

We had a similar email sent to Trust-and-Safety the other day related to this as well.

Same person as the security one? Any more specific than "the other day"? Just wondering if it was related to bumping to 10 characters, or the enforcing of the blacklist

jrbs added a comment.EditedSep 17 2019, 4:22 PM

We had a similar email sent to Trust-and-Safety the other day related to this as well.

Same person as the security one? Any more specific than "the other day"? Just wondering if it was related to bumping to 10 characters, or the enforcing of the blacklist

Different person - just someone forgetting their new password and not having an email assigned to the account. I don't think there's a technical solution to that, just wanted to mention it here.

Reedy updated the task description. (Show Details)Sep 17 2019, 4:23 PM
dmaza added a comment.Sep 17 2019, 5:23 PM

Looking into this

Been looking into this with @dmaza - seems to be a difference in the type of status returned by the checks for the different policies:

In the latter case, TemporaryPasswordPrimaryAuthenticationProvider::beginPrimaryAuthentication then returns an AuthenticationResponse failure.

Ideally we'd want to show the password reset form, but without the option to skip.

Change 537512 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/core@master] Change PasswordPolicy failed status from fatal to error

https://gerrit.wikimedia.org/r/537512

Tgr added a subscriber: Tgr.Sep 17 2019, 6:51 PM

As the name implies, MinimumPasswordLengthToLogin means that you are not allowed to log in with a password shorter than that. The assumption is that the password is so short that in all likelihood it has already been cracked, and the user must find another login method that we still trust (e.g. triggering a password reset). Of course is feasible for staff, and maybe advanced permission holders like stewards, where we know such method exists, and not really feasible for admins in general. So MinimumPasswordLength should be used instead (note the different prefix).

  • PasswordPolicyChecks::checkMinimumPasswordLengthToLogin calls Status::fatal (sets Status::ok to false).

Was the idea behind doing this was to prevent people from logging in if their password is too short?

Possible ways to do that after https://gerrit.wikimedia.org/r/537512:

  1. Always remember to add 'forceChange' => true for MinimumPasswordLengthToLogin, and not for MinimalPasswordLength - messy, and would need to be backed up by documentation
  2. Discard MinimumPasswordLengthToLogin and instead use MinimalPasswordLength with and without forceChange - seems the most sensible to me, though you lose the ability to have the two policies set to different password lengths (but is that really needed?)
  3. Automatically add forceChange to MinimumPasswordLengthToLogin somewhere in the code - this doesn't seem to be a good idea from a usability perspective; setting the policies should lead to predictable behaviour

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Change 537518 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537518

Change 537512 abandoned by Dmaza:
Change PasswordPolicy failed status from fatal to error

https://gerrit.wikimedia.org/r/537512

Tgr added a comment.Sep 17 2019, 7:41 PM

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Yeah, we do want to keep that for staff and other highly sensitive accounts as a paranoia measure. Forcing password change on login does not protect the account if the owner never logs in.

Change 537518 merged by jenkins-bot:
[mediawiki/core@master] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537518

Reedy added a comment.Sep 17 2019, 8:20 PM

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Yeah, we do want to keep that for staff and other highly sensitive accounts as a paranoia measure. Forcing password change on login does not protect the account if the owner never logs in.

I wonder if we should be giving the user some sort of hint in this case... ie use Special:PasswordReset...

Otherwise it's basically a dead end from usability...

Change 537545 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_33] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537545

Tgr added a comment.Sep 17 2019, 9:06 PM

I wonder if we should be giving the user some sort of hint in this case... ie use Special:PasswordReset...

Otherwise it's basically a dead end from usability...

Turns out I did that! T214215: MinimumPasswordLengthToLogin error message is unhelpful / https://gerrit.wikimedia.org/r/c/mediawiki/core/+/485480
It was a long time ago so I'm not quite sure, but I think the patch was functional and just got lost in the code review limbo.

Reedy renamed this task from 'suggestChangeOnLogin' password apparently not working as intended to Intended use of MinimumPasswordLengthToLogin not so clear.Sep 17 2019, 9:08 PM
Reedy added a project: Documentation.

Change 537545 merged by jenkins-bot:
[mediawiki/core@REL1_33] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537545

Change 537550 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_32] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537550

Change 537551 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_31] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537551

Reedy closed this task as Resolved.Sep 17 2019, 9:20 PM
Reedy assigned this task to Tchanders.

Aha. Good. I'll add it to my review queue

I think this task is done, and we don't actually need to re-instate https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ as the behaviour is now as expected?

I've backported it to the supported branches as the doc improvements won't go a miss there either

I think this task is done, and we don't actually need to re-instate https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ as the behaviour is now as expected?

Unless there was an intention to force the password change?

Thanks for backporting.

Change 537551 merged by jenkins-bot:
[mediawiki/core@REL1_31] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537551

Change 537550 merged by jenkins-bot:
[mediawiki/core@REL1_32] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537550

Reedy added a comment.Sep 17 2019, 9:29 PM

I think this task is done, and we don't actually need to re-instate https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ as the behaviour is now as expected?

Unless there was an intention to force the password change?

I should probably have a look at the policy and what we were planning

Maybe we do it more staged, forcing reset first, and then eventually bumping it to completely prevent login of those old passwords. However, if we are going to do that, I definitely think we should get T214215 fixed (or more specifically, get tgr's patch merged. help reviewing welcome :)) before we do that

Change 537555 had a related patch set uploaded (by Jforrester; owner: Reedy):
[operations/mediawiki-config@master] Add comment about MinimumPasswordLengthToLogin

https://gerrit.wikimedia.org/r/537555

Change 537555 merged by jenkins-bot:
[operations/mediawiki-config@master] Add comment about MinimumPasswordLengthToLogin

https://gerrit.wikimedia.org/r/537555

Anomie added a subscriber: Anomie.

@Tchanders: Maybe this summary will help:

  1. 'MinimalPasswordLength' => [ 'value' => 10 ]
    • Prevents setting a password shorter than 10.
    • Existing passwords shorter than 10 can be used for login, and give no notification during the login process.
  2. 'MinimalPasswordLength' => [ 'value' => 10, 'suggestChangeOnLogin' => true ]
    • Prevents setting a password shorter than 10.
    • Existing passwords shorter than 10 can be used for login. The user will be prompted to change their password during login, but may choose to keep their existing password.
  3. 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ]
    • Prevents setting a password shorter than 10.
    • Existing passwords shorter than 10 can be used for login, but the user will be required to change their password before they can complete the login process.
  4. 'MinimalPasswordLengthToLogin' => [ 'value' => 10 ]
    • Prevents setting a password shorter than 10.
    • Existing passwords shorter than 10 cannot be used. The user will have to log in via some other method in order to change their password, or have a sysadmin reset their password or something like that.

It sounds like 92dbafe was intended to do #3, but actually did #4.

@Anomie That's correct - thank you for summarising so clearly for future readers of this task.

If you think the documentation of this behaviour needs updating beyond what I've already added, please feel free to add to it.

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM