T6353: Add password strength check and basic validation #4338
Conversation
… password length check; Improve message format.
oniko94
requested review from
dmbaturin,
sarthurdev,
jestabro,
sever-sever,
c-po,
fett0,
nicolas-fort and
zdc
as code owners
|
❌ |
oniko94
changed the title
There was a problem hiding this comment.
There's a widely used tool for password strength checking — cracklib. Debian has Python bindings in the repos (python3-cracklib) and it includes a dictionary check in addition to entropy checks. The FascistCheck function from https://github.com/cracklib/cracklib/blob/main/src/python/cracklib.py may suffice (however distasteful its name is...).
The entropy function may still be useful because we may want to set a higher cutoff point than cracklib, but we certainly should also use something for a dictionary check to prevent passwords like 'Password999?' that technically have decent entropy but can be easily cracked with a dictionary attack.
The requirement for special characters have long been proven to be counterproductive and should be removed in any case. If cracklib likes a password but it's below our entropy threshold, we can tell the user to make it longer. Otherwise we can just use cracklib's error, since it gives it in the exception message:
>>> cracklib.FascistCheck('swordfish')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: it is based on a dictionary word
| # - 1 special character | ||
| # - be at least 8 characters long | ||
| PASSWORD_POLICY = { | ||
| "have uppercase characters": { |
There was a problem hiding this comment.
The requirement to have special characters and digits is still widespread but security studies found that it's misguided because it can only improve password entropy by making it less memorable. Stringing multiple words together can create passwords with very high entropy that are still easy to remember.
See the NIST recommendation and the XKCD#936 for an illustration.
If we are to go with a custom implementation, I'd remove all requirements except length, since they will prevent high-entropy memorable passwords like the-magic-words-are-sqeamish-ossifrage (entropy in the set of keyboard-typeable characters about 250).
There was a problem hiding this comment.
@dmbaturin Thank you for your feedback! I've taken my time to read the docs for cracklib2 and decided that indeed it would be better to use it in our case. However, this means I would need to have several system packages present apart from the Python bindings - namely, the libcrack2, cracklib-runtime and wordlist. I have made the according changes in the vyos-build repo, a pull request incoming as soon as I finish testing locally.
Until my upcoming PR to vyos-build is reviewed and approved, I think it's better not to push any changes here so that I won't clutter your CI/CD pipeline with builds that are to fail in any case (since they lack required libraries).
P.S. I have also encountered a build issue while building the Docker container locally, which required a tiny patch in the Dockerfile as well. More details in the upcoming PR.
Best regards,
Sasha
| result["strength"] = "Very Weak" | ||
| case e if e in range(36, 59): | ||
| result["strength"] = "Weak" | ||
| case e if e in range(36, 119): |
There was a problem hiding this comment.
The lower boundary is incorrect: should be 59 (mis-pasted from the previous condition?)
| @@ -83,6 +86,7 @@ | |||
| MSG_WARN_ROOT_SIZE_TOOSMALL: str = 'The size is too small. Try again' | |||
| MSG_WARN_IMAGE_NAME_WRONG: str = 'The suggested name is unsupported!\n'\ | |||
| 'It must be between 1 and 64 characters long and contains only the next characters: .+-_ a-z A-Z 0-9' | |||
| MSG_WARN_PASSWORD_TOO_SHORT: str = 'The password should be at least 4 characters long. Try again.' | |||
There was a problem hiding this comment.
Why does the message say '4' when the requirement is 8?
| confirm: str = ask_input(MSG_INPUT_PASSWORD_CONFIRM, no_echo=True, | ||
| non_empty=True) | ||
|
|
||
| passwd_weak = check_result['strength'] not in ['Strong', 'Very Strong'] |
There was a problem hiding this comment.
Using magic strings is a recipe for fragile code. Someone decides to change it to very strong (different case) and the check will break. The usual practice is to use constants, although here there's no need for constants for different strengths, either: just for the strength cutoff (if (strength < MIN_PASSWORD_ENTROPY) or similar).
|
CI integration ❌ failed! Details
|
|
vyos/vyos-build#903 @dmbaturin @sever-sever kindly review this PR - the changes added there block this one. Thanks in advance! |
Change summary
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
install imageChecklist: