Skip to content
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 hashed password support. #1011

Merged
merged 11 commits into from Nov 19, 2011
Merged

Add hashed password support. #1011

merged 11 commits into from Nov 19, 2011

Conversation

stefanv
Copy link
Contributor

@stefanv stefanv commented Nov 18, 2011

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

Out[1]: 'sha1:7cf3:b7d6da294ea9592a9480c8f52e63cd42cfb9dd12'

"""
algorithm = 'sha1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@stefanv stefanv mentioned this pull request Nov 19, 2011
@stefanv
Copy link
Contributor Author

stefanv commented Nov 19, 2011

@mattpap @fperez

passphrase : str
Password to hash.
algorithm : str
Hashing algorithm to use.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
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 added a commit that referenced this pull request Nov 19, 2011
Add hashing of passwords to notebook configuration.  

From now on, we do NOT support plain text passwords in the notebook configuration file, only hashed ones.

To create a properly hashed password, you can use `IPython.lib.security.passwd()`.

Written with Mateusz Paprocki (@mattpap at github).
@fperez fperez merged commit 1f72dda into ipython:master Nov 19, 2011
except ValueError:
return False

if len(pw_digest) == 0 or len(salt) != salt_len:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4cd1067

@satra
Copy link
Contributor

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.


h = hashlib.new(algorithm)
salt = ('%0' + str(salt_len) + 'x') % random.getrandbits(4 * salt_len)
h.update(passphrase + salt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done essentially that in PR #1016.

@fperez
Copy link
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

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Add hashing of passwords to notebook configuration.  

From now on, we do NOT support plain text passwords in the notebook configuration file, only hashed ones.

To create a properly hashed password, you can use `IPython.lib.security.passwd()`.

Written with Mateusz Paprocki (@mattpap at github).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants