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

Add password strength option #31

Merged
merged 14 commits into from Jan 23, 2019

Conversation

Projects
None yet
2 participants
@leportella
Copy link
Collaborator

leportella commented Jan 15, 2019

Closes #22

@yuvipanda
Copy link
Member

yuvipanda left a comment

Password complexity requirements around mixing cases has generally proven to lead to less secure passwords and not more - the general suggestion today is to not do that. Instead, what we should do is to check password against a common list of passwords - such as https://github.com/danielmiessler/SecLists/blob/master/Passwords/Common-Credentials/10-million-password-list-top-10000.txt - and disallow that.

See https://auth0.com/blog/dont-pass-on-the-new-nist-password-guidelines/ for more information, and a link to the NIST guidelines themselves.

@leportella

This comment has been minimized.

Copy link
Collaborator Author

leportella commented Jan 16, 2019

Nice article @yuvipanda ! Thanks. I changed it check for size and that if it is too common

@leportella leportella force-pushed the add-password-strength-option branch from e3fb956 to 7dfbefb Jan 16, 2019

@leportella leportella force-pushed the add-password-strength-option branch from 662b27a to 6e1027f Jan 16, 2019

@yuvipanda
Copy link
Member

yuvipanda left a comment

Great!

I left a few suggested changes. Can you also add docs or a script on where the list of common passwords comes from and how to update it?


result_message = 'Your information have been sent to the admin'
if not user:
result_message = """Something went wrong. Be sure your password

This comment has been minimized.

@yuvipanda

yuvipanda Jan 16, 2019

Member

This length should be configurable

This comment has been minimized.

@leportella

leportella Jan 17, 2019

Author Collaborator

Done!


from .common_credentials import COMMON_CREDENTIALS

This comment has been minimized.

@yuvipanda

yuvipanda Jan 16, 2019

Member

Can you make this a .txt file instead of a .py file?

This comment has been minimized.

@leportella

leportella Jan 17, 2019

Author Collaborator

I thought about this but I would have to everytime open a file, create a list and then search in the list. I thought that if I already had a list, it would be more efficient. What do you think?

This comment has been minimized.

@yuvipanda

yuvipanda Jan 17, 2019

Member

Hmm, can we read it once - on first use and cache it? As a static method maybe? This also means it isn't loaded into memory unless it is needed.

I try to avoid Python files where possible for pure data, since any mistakes there - especially in massive files - can execute arbitrary code.

This comment has been minimized.

@leportella

leportella Jan 17, 2019

Author Collaborator

Not sure how to do this using cache. Could you give me an example?

This comment has been minimized.

@yuvipanda

yuvipanda Jan 17, 2019

Member

Sure! something like

common_passwords.py

COMMON_PASSWORDS = None

def is_password_common(password):
    global COMMON_PASSWORDS
    if COMMON_PASSWORDS == None:
        with open("path-to-file") as f:
            COMMON_PASSWORDS = f.readlines()
    return password in COMMON_PASSWORDS

How does that sound?

This comment has been minimized.

@leportella

leportella Jan 17, 2019

Author Collaborator

Oh! Ok! Great!

@leportella

This comment has been minimized.

Copy link
Collaborator Author

leportella commented Jan 17, 2019

@yuvipanda I just added a new pull request for docs only

@leportella leportella force-pushed the add-password-strength-option branch from c9d64b7 to ea07fdf Jan 17, 2019

@yuvipanda
Copy link
Member

yuvipanda left a comment

Some more comments but can be merged after this :)

if not self.COMMON_PASSWORDS:
with open(common_credentials_file) as f:
self.COMMON_PASSWORDS = f.read().splitlines()
return password.lower() in self.COMMON_PASSWORDS

This comment has been minimized.

@yuvipanda

yuvipanda Jan 17, 2019

Member

Does COMMON_PASSWORDS only contain lowercase characters?

This comment has been minimized.

@leportella

leportella Jan 18, 2019

Author Collaborator

Indeed, it doesn't. I removed this lower() method :)

return password.lower() in self.COMMON_PASSWORDS

def is_password_strong(self, password):
checks = [

This comment has been minimized.

@yuvipanda

yuvipanda Jan 17, 2019

Member

I think these should be two config options:

  1. disallow_common_passwords - Checks if password used is common. Default to True
  2. minimum_password_length - Only allow passwords longer than this. Can be set to 0 to disable length checking. Defaults to 8.

Also, when checking a constant number of variables like this, it's clearer to just use a boolean expression directly len(password) >= self.minimum_password_length and not self.is_common_password(password) than using all

result_message = 'Your information have been sent to the admin'
if not user:
result_message = """Something went wrong. Be sure your password
has at least 8 characters and is not

This comment has been minimized.

@yuvipanda

yuvipanda Jan 18, 2019

Member

This should refer to the actual configured length rather than be hardcoded to 8

@leportella leportella force-pushed the add-password-strength-option branch from 5a56f14 to c59ef08 Jan 21, 2019

@yuvipanda
Copy link
Member

yuvipanda left a comment

<3 LGTM!

@yuvipanda yuvipanda merged commit 449e08d into master Jan 23, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment