20
\$\begingroup\$

I have made a password verification "program" which takes user input and checks whether the password is valid or not. Based on that, a reply is printed.

print("Create a password! Your password must have 8 to 12 digits. Numbers and lower as well as upper case letters must be a part of it!")
password = input("Enter your password:")
res = any(chr.isdigit() for chr in password)
res2 = any(chr.islower() for chr in password)
res3 = any(chr.isupper() for chr in password)
if len(password) >= 8 and len(password) <13 and res == True and res2 == True and res3 == True :
    print("Welcome!")
else:
    print("Your password is not valid!")

Is there a more elegant or better way? Maybe you also have some ideas on how to expand the project?

\$\endgroup\$
2
  • 8
    \$\begingroup\$ Don't worry, I was not planing on using this for anything. It was just a small project so that I have something specific to look into. Otherwise getting into programming seems pretty overwhelming \$\endgroup\$ – morloq Mar 23 at 18:28
  • \$\begingroup\$ I remember that overwhelming feeling (hell, I still have it sometimes), but don‘t let it discourage you. For me the key was to find projects that steadily increase in complexity. The closer a project is to a real use case, the better. \$\endgroup\$ – riskypenguin Mar 25 at 15:38
29
\$\begingroup\$

Two general points about your code:

  1. Variable naming: res, res2, res3 are not descriptive, so it's harder to understand at first glance what their purpose is. Instead I would recommend something similiar to contains_digit, contains_lower_char, contains_upper_char. Variable naming is always up to a bit of personal preference, but explicit names are generally preferable.

  2. Conditionals: The way the condition in your if-statement is set up is suboptimal. Firstly: len(password) >= 8 and len(password) <13 can be shortened to 8 <= len(password) <= 12 or len(password) in range(8, 13). This is more readable and depicts your intentions in a more concise way. Secondly: You almost never need == True clauses since True == True -> True and False == True -> False. So the second part of your condition can be shortened to res and res2 and res3. This is also where better variable names make the functionality of your program way clearer.

To avoid multiple concatenated and-statements you could probably use something like all([len(password) in range(8, 13), res, res2, res3]), but I find this usually decreases readbility.

To conclude I would suggest the following if-condition:

if 8 <= len(password) <= 12 and contains_digit and contains_lower and contains_upper:

On a side note: This is not a password generator, but a password validity checker. You might want to additionally check, that the password only includes ASCII-characters if that is relevant.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you! That's really helpful, I can see what you mean \$\endgroup\$ – morloq Mar 21 at 17:03
  • \$\begingroup\$ Glad to help. Please consider marking the question as answered (i.e. accept an answer) if you requirements are met, so other contributors know the question does not require further attention. \$\endgroup\$ – riskypenguin Mar 21 at 17:05
  • 12
    \$\begingroup\$ Please consider upvoting the question as well. If it was worth answering, it should've been worth a vote too. Otherwise, why did you answer it? ;-) \$\endgroup\$ – Mast Mar 21 at 17:17
  • \$\begingroup\$ @mast is that a thing? \$\endgroup\$ – theonlygusti Mar 24 at 16:33
13
\$\begingroup\$

Not much more to add here, but you'll quickly find that this routine is too basic for the need and regular expressions are the way to go.

This routine does not guarantee that the resulting password will be balanced, that is sufficiently "random" and hard to guess. For example AAAbbb123 or Abcdef123 will pass your test. These are not strong passwords and they may even be in some lists of common passwords (pwnlists), which means they are less likely to withstand brute force attempts. Or what about Ab1 followed by 5 or more whitespace characters ? The point is that really poor patterns will get through.

On the other hand, a really good password is hard to remember for humans but that's why we have password managers, especially that each password should be unique and not reused across sites.

@riskypenguin suggests that you may want to restrict input to ASCII characters only. I'm not sure I would do that. Using non-standard strings increases complexity. You may have foreign users who are accustomed to their native script, for example Japanese. They may want to use their keyboard in "native mode" without switching to ASCII.

Due to the complexity, many developers prefer to ignore the subject and stick to ASCII but see below if you want to learn more.

Nowadays websites usually use the UTF-8 character set and this usually holds true for database storage. Plus, you are not supposed to store the plaintext password but a hash, like a salted SHA-512 hash, which is plain ASCII.

Finally, limiting the length to 12 characters is a poor choice. Some users may want to use longer passwords, or a more memorable passphrase. The choice of 12 characters is arbitrary and puts a cap on complexity. The real limit should be the length of the password field in your HTML forms. Yeah, 50 characters should be reasonable maybe. But 12 ? This is so 1996 I think :)

References:

\$\endgroup\$
5
  • 9
    \$\begingroup\$ The main and super important point here is that the actual policy being implemented by OP is bad, and has been known to be bad for many years. I pity the users if this code ever makes it into production. \$\endgroup\$ – l0b0 Mar 21 at 23:36
  • 1
    \$\begingroup\$ A really good password is hard to remember for humans? Hardly, as you seem to allude to later. See security.stackexchange.com/questions/6095/… Of course, there are systems imposing unreasonable restrictions regarding password-length and contents to work in an insecure manner. \$\endgroup\$ – Deduplicator Mar 22 at 1:25
  • 9
    \$\begingroup\$ "Finally, limiting the length to 12 characters is a poor choice." +1 for that alone. \$\endgroup\$ – infinitezero Mar 22 at 8:46
  • 3
    \$\begingroup\$ store the plaintext password but a hash, like a salted SHA-512 hash no, you should not do that.SHA is a fast hash and you do not want that. You want it to be slow, independent on hardware etc. There are special hashes done just for that (PBKDF2, bcrypt, argon ...) \$\endgroup\$ – WoJ Mar 22 at 14:22
  • \$\begingroup\$ mandatory xkcd: Password Strength \$\endgroup\$ – 0xFEE1DEAD Mar 23 at 21:54
9
\$\begingroup\$

There are two separate questions here, and I'll try to tackle both. You're asking how to improve the code, which I'll separate into two halves: "How can I make this into better code that does the same thing?", and "How can I make this do better things?".

Making the code better while doing the same thing:

if len(password) >= 8 and len(password) <13 and res == True and res2 == True and res3 == True :

You're doing a "betweenness" check here, which Python actually makes really easy. Also, you don't have to check if something is equal to True - just check if it is. So, try this:

if 8 <= len(password) < 13 and res and res2 and res3:

Others have mentioned variable naming, so I'll just briefly say that I agree, and "has_lower" and so on would be better than "res2".

It's a very small UI adjustment, but I prefer a space after a colon prompt.

password = input("Enter your password:")
password = input("Enter your password: ")

So, on to doing things in better ways.

An easy one: Check out the getpass module for a better way to ask the user for a password. Where possible, it'll mask the password, or hide it from history, or whatever else is appropriate for the environment it's in.

Secondly: Don't ever limit the length of a password, other than to cut off really ridiculous things that wouldn't actually be passwords at all. For instance, a limit of 50 (as has been suggested) is probably not a problem, but I'd go all the way to 256 or more, unless you have something that actually can't work with passwords that long. There's definitely no reason to cap it at thirteen.

If you want to get really sophisticated, what you can consider is a hybrid of bank-style password requirements ("must have at least one upper, one lower, one digit, one symbol, and one character that can't be typed on a US English keyboard") and a much more secure length requirement. Permit XKCD 936 style passwords by simply allowing any password greater than some length requirement. Since this is an exercise in Python coding, here's what I'd recommend: Give a score to each category of character you've found (eg 26 points for a lower-case letter, 26 more for an upper-case, 10 for a digit, etc), add up all those scores, and raise it to the power of the length of the password. That's a decent way to evaluate a password's complexity, although it's never going to be perfect (XKCD 936 passwords are extremely difficult to judge fairly unless you know the original word list); you can then play around with it and see just how long a pure-lower-case-ASCII-letter password needs to be in order to score as well as a more bank-complex password. HINT: It's not nearly as long as many people's intuition says it is.

If you want to actually get into password generation, as opposed to validation, I'd recommend looking at the secrets module, which has a variety of tools that will help you.

Have fun with it!

\$\endgroup\$
8
  • \$\begingroup\$ unless you have something that actually can't work with passwords that long this usually means bad news. When a password length is capped, I am always extremely suspicious of the way it is handled. Having a 1MB big password should not be an issue. With 1GB you may run into some timeout and performance issues, but never security ones. (examples of 1 MB or 1GB passwords are of course exaggerated) \$\endgroup\$ – WoJ Mar 22 at 14:25
  • 1
    \$\begingroup\$ Yep. Depending on the protocol, sometimes it's worth setting a practical limit, just to prevent protocol failures, and there's always the possibility that there's something in the system that doesn't want a password more than (say) 65536 bytes. I'm not ruling out the possibility. But yeah, you definitely don't want to cap it at 13 characters. \$\endgroup\$ – rosuav Mar 23 at 15:03
  • \$\begingroup\$ there's always the possibility that there's something in the system that doesn't want a password more than (say) 65536 bytes → that should never be the case. The password is checked against a hash (so it does not matter if the password is 1 or 98098080809 chars long), and then there is a session token (or similar) that is being used for authorization (and relayed further on). If there is a limit because "something does not play well with the length" then it is a real security issue. \$\endgroup\$ – WoJ Mar 23 at 15:49
  • \$\begingroup\$ (cont'd) If there is capping for performance reasons then fine (but it will be very large anyway). A browser will set a (huge) limit itself, but having 256 (MS choice) or 10000 should not be an issue. \$\endgroup\$ – WoJ Mar 23 at 15:49
  • 1
    \$\begingroup\$ Right, that's what I'm saying - it would be a ridiculously high limit. Some protocols simply don't play well with stupid-large data; for instance, if you're sending information over UDP, you really want to keep it within one packet if you can. (Though if you're sending passwords in clear text over UDP, you have bigger problems.) Any length limit on the password needs to be significantly greater than the hash length, for a start. \$\endgroup\$ – rosuav Mar 24 at 6:57
3
\$\begingroup\$

I'm not going to retype what everyone already said, but I'll add a quick suggestion.

Use Pylint to help you write better code. It helped me tremendously when starting. As an example, Pylint would mark the following in your code.

On the first line: Line too long (141/100)
On each res == true (etc..): Comparison 'res == True' should be 'res is True' if checking for the singleton value True, or 'bool(res)' if testing for truthiness.

And just because I'm in the mood of giving suggestions, take a look at venv.

\$\endgroup\$
9
  • 1
    \$\begingroup\$ venv is irrelevant here, no point burdening a new(ish) programmer with too much hassle :) Incidentally, bool(res) is a poor choice too - just res is the best way here. Not sure why pylint recommended that. \$\endgroup\$ – rosuav Mar 24 at 6:58
  • \$\begingroup\$ PEP 8 says "Don’t compare boolean values to True or False using ==:". I would have expected Pylint to pick that up. What Pylint warnings have you disabled and what version of Pylint (pylint --version)? \$\endgroup\$ – Peter Mortensen Mar 24 at 15:15
  • \$\begingroup\$ I use Pylint 2.6.0 (Ubuntu MATE 20.04 (Focal Fossa)), and I get C0121: Comparison to True should be just 'expr' (singleton-comparison). \$\endgroup\$ – Peter Mortensen Mar 24 at 15:17
  • \$\begingroup\$ This confusing Pylint error message seems to have been fixed at the end of 2018. \$\endgroup\$ – Peter Mortensen Mar 24 at 15:50
  • \$\begingroup\$ On newer versions of Debian/Ubuntu, installing Pylint with pip install pylint will not work. Instead, use pip3 install pylint. \$\endgroup\$ – Peter Mortensen Mar 24 at 16:04
1
\$\begingroup\$

Welcome to the wonderful world of programming!

While writing code it is helpful to think of similar problems you can solve. Not sure if you are specifically looking for a password verification routine (which you should use an established library instead of rolling your own), or if you are just going through an academic exercise.

Exercises are good to help learn a language or explore a problem, even if they are never truly production worthy. Here is your password routine re-envisioned as a Product Number verifier. Same basic problem of testing contents of Strings, but removes the whole password discussion.

print( "Enter product number: normally 8 to 12 characters with numerics and upper and lower case letters..." )
allegedProduct = input( "Product: " )

# control checks
isValid = True # optimistic

if( not any( chr.isdigit() for chr in allegedProduct ) ) :
    isValid = False

if( not any( chr.islower() for chr in allegedProduct ) ) :
    isValid = False

if( not any( chr.isupper() for chr in allegedProduct ) ) :
    isValid = False

if( not ( 8 <= len( allegedProduct ) <= 12 ) ) :
    isValid = False

if( isValid ) :
    print( "Product Number format confirmed." )
else:
    print( "Incorrect Product Number format." )

Of course, the If statements can be combined as follows...

print( "Enter product number: normally 8 to 12 characters with numerics and upper and lower case letters..." )
allegedProduct = input( "Product: " )

# control checks
if( ( not any( chr.isdigit() for chr in allegedProduct ) ) or
    ( not any( chr.islower() for chr in allegedProduct ) ) or
    ( not any( chr.isupper() for chr in allegedProduct ) ) or
    ( not ( 8 <= len( allegedProduct ) <= 12 ) ) ) :
    print( "Incorrect Product Number format." )
else:
    print( "Product Number format confirmed." )

Keep testing. Keep iterating. And keep playing. Have fun!

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to the Code Review Community. The point of the code review community is to help the original poster improve their coding skills so answers should make meaningful observations about the code in the question. This answer doesn't seem to address the code in the question at all. \$\endgroup\$ – pacmaninbw Mar 24 at 11:54
0
\$\begingroup\$

I don't have any Python implementation feedback, but I do have a little bit of security design feedback.

When you ask the user to specify an initial password, you should not echo the output to the screen (in case an interloper is watching them).

When you ask the user to specify an initial password, you should ask them to enter it twice, and then check additionally that the two passwords are the same. This helps the user avoid mistyping when the output is not echoed.

It is not a good design to provide a password length upper limit (You specified an arbitrary upper limit of 13). Because "George Bush speaks Spanish." is quite a complex password that is easy to remember and type correctly. The technique is known as a "pass phrase".

The terminology for the character classes you asked for are "character groups". Systems designed to be highly secure often ask for three or four of the following four character groups:

  • lowercase ASCII letters

  • uppercase ASCII letters

  • digits

  • punctuation

BUT: some of the best and most recent advice from the USA National Institutes of Standards and Technology specify that users should just be able to choose their password, not to impose any composition rules (i.e. character group requirements) on them, and instead to check their proposal against a list of known bad or previously breached passwords. Such a list is available from the haveibeenpwned.com service.

In addition to ASCII characters, you might have a thought about whether users might like to use characters not found in ASCII. As long as you think the user would find it useful, I feel that any typeable and printable character could be allowed in a password. The limit is the range of systems a user might have to use, and the limitations of those input methods. That is a justification for restricting to ASCII.

Your code does not address storage of the password. Generally, you should use a cryprographic hash function like bcrypt and store the hash, never storing the password and even wiping it from heap memory of the Python interpreter if possible. You should apply the bcrypt work factor so that your computer needs to work a threshold amount of time to generate the hash. For example, you could set the target to process for 500ms. The work factor can be adjusted upwards every few years when computers process the hash faster. Each password should be salted with a random unique salt of at least 8 bytes, and you can store that salt in the table with the password hash in plain text. When the user needs to log in, you should request the password in plain text over a secure transport like TLS 1.3. You take their entered password and the salt, and use bcrypt to generate a hash. If the hash is the same as the one you previously stored, then the password matched.

When a user tries to log in, be careful not to disclose who the existing user accounts are, unless you decide to as part of your design. The existence of a user account is something you should keep secret, since that knowledge creates a target for an attack. The best way to keep this information private is to 1) generate a hash for every attempt, even for usernames that do not exist; and 2) to always give a generic feedback message, never indicating the specific reason the user cannot log in. In this way, an attacker cannot try lots of usernames and use the different error responses to determine which users exist, do not exist, or are locked out. Also the attacker will not be able to use the time delay of the reply to determine whether the username/password was correct or incorrect.

Final topic! When a user sets a password, they are not really "authenticated". All you know is the user provided you a password (and if it is the same password as when they registered, then you may assume it is the same person). BUT: you do not actually know that the person who registered is the person they say they are. Example, a user could enter their name Barack Obama, and specify the password "George Bush speaks Spanish." But that does not prove to you that the user is really Barack Obama. So, depending on what information or abilities this user has on your system, you will want to authenticate them to a certain standard.

  • One (low) standard of authentication is Email Address Verification. This simply means the person who registered also has control of the email address they have provided to you. You can do a similar thing with a mobile phone message.

  • Another (high) standard of authentication may be to ask them to log in and then provide access to a video call, where you can check the user's government identification.

  • An in-between level of authentication might be to use the OpenID Connect protocol to accept a user's identity that they choose to share with you through, for instance, Google or Twitter. After OIDC is configured, the user would log into that web service and choose to share their identity with your service. Another alternative would be to charge the user a credit card transaction and note their cardholder identity details.

If you read this far, then you will understand that handling passwords is actually an extremely difficult part of software design. Any time you can design a user interaction without passwords, that is superior. I am going to write a book on this someday. The main reason I gave such a detailed reply is that example code like yours sometimes does get copied/pasted into real implementations and these lead to real world security defects. I didn't cover everything, but I covered the most accessible and important 99%. And I hope that you and the other readers do not consider my answer too far off-topic. Best wishes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.