Skip to content

Set a weak password threshold for "forbidden substrings"

Nick Malcolm requested to merge 384336-adjust-weak-password-threshold into master

What does this MR do and why?

Set a weak password threshold for "forbidden substrings"

Security::WeakPasswords flags a password as weak when it contains a user's username, name, email, or a "substring" of one of those. E.g. for a user with a full name "Nick Malcolm" if either "Nick", "Malcolm" or "Nick Malcolm" appeared in the password, it would be weak.

However in long passwords it was possible that those forbidden substrings would legitimately occur in a truly random password. This in fact did occur in the randomized GitLab specs (See #384336 (closed)). This MR, therefore, skips the "weak substring" checks for passwords 64 characters or longer.

Passwords are still checked against the static weak password list regardless of length.

::User.random_password is 128 chars, so this will address any potential test flakiness as the "weak substring" checks will be skipped for passwords of that length.

Closes WeakPassword returns true even for long, random... (#384336 - closed) and fixes a flaky-testrandom input which caused a ~"master-broken:flaky-test"

Alternatives

The approach in this MR is a pretty blunt and arbitrary solution to the issue. In a future iteration, perhaps we could figure out a mapping of "substring size" to "password size", but that seems like a complex approach for a marginal security benefit.

We could also increase Security::WeakPasswords::MINIMUM_SUBSTRING_SIZE from 4 to 5 to make a collision less likely, but they'd still possible. We'd also lose the security benefit of flagging four-character substrings. It means we still prevent alex1password from being a valid password for Alex Fischer, but if Alex uses a random password generator that creates a long strong password that just happens to contain aLeX they'll still be fine.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #384336 (closed)

Edited by Nick Malcolm

Merge request reports