Add hashed password support. #1011

Merged
merged 11 commits into from Nov 19, 2011

Projects

None yet

5 participants

@stefanv
stefanv commented Nov 18, 2011

Add hashing of passwords to notebook configuration [written with Mateusz Paprocki].

@minrk minrk and 1 other commented on an outdated diff Nov 18, 2011
IPython/lib/security.py
+ ----------
+ passphrase : str
+ Password to hash.
+
+ Returns
+ -------
+ hashed_passphrase : str
+ Hashed password, in the format 'hash_algorithm:salt:passphrase_hash'.
+
+ Examples
+ --------
+ In [1]: passwd('mypassword')
+ Out[1]: 'sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12'
+
+ """
+ algorithm = 'sha1'
@minrk
minrk Nov 18, 2011 IPython member

if algorithm is an option, why isn't it an arg to passwd?

@stefanv
stefanv Nov 18, 2011

Because, for now, only sha1 is supported. We could add it though; would that be preferred?

@minrk
minrk Nov 18, 2011 IPython member

If the design is that multiple algorithms are to be supported, it should probably be an arg, even if the supported list is only length one at this point.

In fact, if you simply removed the length-check dict, and just let the hash go ahead, you would already have support for more than just sha1.

@minrk minrk commented on an outdated diff Nov 18, 2011
IPython/lib/security.py
+ In [3]: passwd_check('sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12',
+ ...: 'anotherpassword')
+ Out[3]: False
+
+ """
+ # Algorithm and hash length
+ supported_algorithms = {'sha1': 40}
+
+ try:
+ algorithm, salt, pw_digest = hashed_passphrase.split(':', 2)
+ except (ValueError, TypeError):
+ return False
+
+ if not (algorithm in supported_algorithms and \
+ len(pw_digest) == supported_algorithms[algorithm] and \
+ len(salt) == 4):
@minrk
minrk Nov 18, 2011 IPython member

The way you generate the salt above, it is not guaranteed to be 4 bytes because the random values can be < 16^3, which will be 3 (or fewer) characters, so this will actually fail once in every ~16 runs.

It also seems unnecessary to check the length of the salt, since it shouldn't actually matter.

@minrk
minrk Nov 18, 2011 IPython member

Why not just skip this step, and wrap the next hashlib.new();h.update() bit in try/except: return False?

That's obviously more expensive in the rare cases where input is invalid, but it means we would support pretty much everything in hashlib.algorithms without having to maintain a supported_algorithms dict. I cannot think of a situation where this extra cost would be significant, though.

In this way, less code actually supports more use cases.

@stefanv stefanv referenced this pull request Nov 19, 2011
Merged

Add logout button. #1012

@fperez fperez commented on an outdated diff Nov 19, 2011
IPython/lib/security.py
+
+import hashlib
+import random
+
+def passwd(passphrase, algorithm='sha1'):
+ """Generate hashed password and salt for use in notebook configuration.
+
+ In the notebook configuration, set `c.NotebookApp.password` to
+ the generated string.
+
+ Parameters
+ ----------
+ passphrase : str
+ Password to hash.
+ algorithm : str
+ Hashing algorithm to use.
@fperez
fperez Nov 19, 2011 IPython member

Should indicate here that this string should be any that's a valid input to hashlib.new, so users know what to look for.

@fperez fperez commented on an outdated diff Nov 19, 2011
IPython/lib/security.py
+ algorithm : str
+ Hashing algorithm to use.
+
+ Returns
+ -------
+ hashed_passphrase : str
+ Hashed password, in the format 'hash_algorithm:salt:passphrase_hash'.
+
+ Examples
+ --------
+ In [1]: passwd('mypassword')
+ Out[1]: 'sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12'
+
+ """
+ h = hashlib.new(algorithm)
+ salt = '%04x' % random.getrandbits(16)
@fperez
fperez Nov 19, 2011 IPython member

I'm not an expert on salting, but from a quick read on wikipedia it seems that the recommended salt length is 48 to 128 bits. Why not make it at least 64 bits?

@fperez fperez commented on an outdated diff Nov 19, 2011
IPython/lib/security.py
@@ -0,0 +1,81 @@
+"""
+Password generation for the IPython notebook.
+"""
+
+import hashlib
+import random
+
+def passwd(passphrase, algorithm='sha1'):
@fperez
fperez Nov 19, 2011 IPython member

This function should be callable without a passphrase (set to None). In that mode, it should use the getpass module to ask the user for the password interactively, doing it twice to verify correctness, and then return the encoded value. This would let users create passwords without echoing them to the screen.

@fperez
Member
fperez commented Nov 19, 2011

Thanks a ton for this, it's awesome. The fixes should be pretty easy and we can merge soon.

@fperez

Why not use None here? It's a clearer way to indicate that this parameter is in an invalid state...

@fperez

Since this is inherently interactive code, instead of a simple check with a ValueError at the end, it should instead do a 3-loop attempt at getting it right, and if on the third time it still doesn't work, then simply print to stderr a short error message and return.

@fperez

No, not an exception, just an error message to stderr: exceptions are useful for programmatic use, but they are not very user friendly. Alternatively, you can import from IPython.core.error import UsageError. That error is a real exception (in case this code is used by something else) but we special-case it and don't show a traceback, for situations like this.

@fperez fperez merged commit 1f72dda into ipython:master Nov 19, 2011
@minrk minrk commented on the diff Nov 19, 2011
IPython/lib/security.py
+ In [3]: passwd_check('sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12',
+ ...: 'anotherpassword')
+ Out[3]: False
+
+ """
+ try:
+ algorithm, salt, pw_digest = hashed_passphrase.split(':', 2)
+ except (ValueError, TypeError):
+ return False
+
+ try:
+ h = hashlib.new(algorithm)
+ except ValueError:
+ return False
+
+ if len(pw_digest) == 0 or len(salt) != salt_len:
@minrk
minrk Nov 19, 2011 IPython member

Checking the salt length seems entirely unnecessary here. It has no valuable effect, because shorter or longer salts would still be perfectly valid.

@fperez
fperez Nov 19, 2011 IPython member

Good point. I won't revert it quite yet, let's see if @stefanv had a specific reason to check for that. But I can't think of one and unless Stefan has a good reason for wanting it there, we can remove the length check.

@stefanv
stefanv Nov 19, 2011

You can take this out. We put in a number of tests to make sure the hash is
in the right format, but as all negative results return False it doesn't
matter.
On Nov 18, 2011 9:02 PM, "Fernando Perez" <
reply@reply.github.com>
wrote:

  • In [3]:
    passwd_check('sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12',
  •   ...:              'anotherpassword')
    
  • Out[3]: False
  • """
  • try:
  •    algorithm, salt, pw_digest = hashed_passphrase.split(':', 2)
    
  • except (ValueError, TypeError):
  •    return False
    
  • try:
  •    h = hashlib.new(algorithm)
    
  • except ValueError:
  •    return False
    
  • if len(pw_digest) == 0 or len(salt) != salt_len:

Good point. I won't revert it quite yet, let's see if @stefanv had a
specific reason to check for that. But I can't think of one and unless
Stefan has a good reason for wanting it there, we can remove the length
check.


Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1011/files#r240467

@fperez
fperez Nov 19, 2011 IPython member

Fixed in 4cd1067

@fperez fperez commented on the diff Nov 19, 2011
IPython/lib/security.py
+ p1 = getpass.getpass('Verify password: ')
+ if p0 == p1:
+ passphrase = p0
+ break
+ else:
+ print('Passwords do not match.')
+ else:
+ raise UsageError('No matching passwords found. Giving up.')
+
+ h = hashlib.new(algorithm)
+ salt = ('%0' + str(salt_len) + 'x') % random.getrandbits(4 * salt_len)
+ h.update(passphrase + salt)
+
+ return ':'.join((algorithm, salt, h.hexdigest()))
+
+def passwd_check(hashed_passphrase, passphrase):
@fperez
fperez Nov 19, 2011 IPython member

BTW, it just occurred to me that we should probably add to this function a delay option, defaulting to 0.25 seconds at least... That will prevent DOS attacks and make brute-force rapid-fire guessing impossible. What do you guys think?

@satra
satra commented Nov 19, 2011

is there anyway to do this, so it can be done on the command line when starting the notebook? i tried to add a function to the config.py file that asks for the password, but it seems the config file is parsed too many times during a session and hence i kept getting asked for a password.

@takluyver takluyver commented on the diff Nov 19, 2011
IPython/lib/security.py
+ """
+ if passphrase is None:
+ for i in range(3):
+ p0 = getpass.getpass('Enter password: ')
+ p1 = getpass.getpass('Verify password: ')
+ if p0 == p1:
+ passphrase = p0
+ break
+ else:
+ print('Passwords do not match.')
+ else:
+ raise UsageError('No matching passwords found. Giving up.')
+
+ h = hashlib.new(algorithm)
+ salt = ('%0' + str(salt_len) + 'x') % random.getrandbits(4 * salt_len)
+ h.update(passphrase + salt)
@takluyver
takluyver Nov 19, 2011 IPython member

It's just occurred to me that this is not going to be portable to Python 3, because you can't directly hash unicode. For that matter, it will fail on non-ascii unicode strings in Python 2 as well.

I think the best thing is to do is: py3compat.cast_bytes((passphrase + salt), 'utf-8').

I realise this has been merged - I'll try to get round to doing another PR, unless someone else beats me to it.

@fperez
fperez Nov 19, 2011 IPython member

Sorry about that! We haven't gotten into a good swing of keeping py3 in mind, my fault...

BTW, if you see anything similarly problematic on #1012, let me know. It doesn't have similar low-level pieces so it shouldn't be much of a problem, but still, pitch in if you see anything amiss regarding py3.

@takluyver
takluyver Nov 19, 2011 IPython member

I'll have a check.

@stefanv
stefanv Nov 20, 2011

Mateusz just reminded me that allowing unicode for a password is probably a bad idea, so I think we'll try to detect that situation.

@takluyver
takluyver Nov 20, 2011 IPython member

My feeling is that if people want to use non-ascii characters in their password, that's up to them.

@stefanv
stefanv Nov 20, 2011

So how about just doing .encode() on the string? That should work in both 2.7 and 3.

@takluyver
takluyver Nov 20, 2011 IPython member

I've done essentially that in PR #1016.

@fperez
Member
fperez commented Nov 19, 2011

@satra, that's a good point. There's a quick and easy hack (until we have a chance to disentangle why the init file is being parsed more than once): write a small wrapper function in your init file (or a locally loaded utility) that simply tracks its own state and uses passwd:

def password():
  from IPython.lib import passwd
  if password.pwd is None:
    pwd = passwd()
    password.pwd = pwd
    return pwd
  else:
    return password.pwd

password.pwd = None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment