From 4899210334a030c541e13aa994b25b3996bb3cc9 Mon Sep 17 00:00:00 2001 From: termie Date: Tue, 24 Jan 2012 00:56:53 -0800 Subject: [PATCH] bcrypt the passwords --- keystone/common/utils.py | 25 +++++++++++++++++++++++++ keystone/config.py | 12 ++++++++++++ keystone/identity/backends/kvs.py | 27 +++++++++++++++++++-------- keystone/identity/backends/sql.py | 13 ++++++++++++- keystone/identity/core.py | 7 ++----- keystone/test.py | 10 ++++++++-- tests/test_backend.py | 5 +++++ tests/test_keystoneclient.py | 10 ++-------- tests/test_keystoneclient_sql.py | 2 +- tests/test_overrides.conf | 3 +++ tools/pip-requires | 1 + 11 files changed, 90 insertions(+), 25 deletions(-) diff --git a/keystone/common/utils.py b/keystone/common/utils.py index c3f19037b1..3ee9489441 100644 --- a/keystone/common/utils.py +++ b/keystone/common/utils.py @@ -25,9 +25,16 @@ import subprocess import sys import urllib +import bcrypt + +from keystone import config from keystone.common import logging +CONF = config.CONF +config.register_int('bcrypt_strength', default=12) + + def import_class(import_str): """Returns a class from a string including module and class.""" mod_str, _sep, class_str = import_str.rpartition('.') @@ -134,6 +141,24 @@ class Ec2Signer(object): return b64 +def hash_password(password): + """Hash a password. Hard.""" + salt = bcrypt.gensalt(CONF.bcrypt_strength) + return bcrypt.hashpw(password, salt) + + +def check_password(password, hashed): + """Check that a plaintext password matches hashed. + + Due to the way bcrypt works, hashing a password with the hashed + version of that password as salt will return the hashed version + of that password (mostly). Neat! + + """ + check = bcrypt.hashpw(password, hashed[:29]) + return check == hashed + + # From python 2.7 def check_output(*popenargs, **kwargs): r"""Run command with arguments and return its output as a byte string. diff --git a/keystone/config.py b/keystone/config.py index 11db8b53aa..2d69d7a644 100644 --- a/keystone/config.py +++ b/keystone/config.py @@ -114,6 +114,18 @@ def register_cli_bool(*args, **kw): return conf.register_cli_opt(cfg.BoolOpt(*args, **kw), group=group) +def register_int(*args, **kw): + conf = kw.pop('conf', CONF) + group = _ensure_group(kw, conf) + return conf.register_opt(cfg.IntOpt(*args, **kw), group=group) + + +def register_cli_int(*args, **kw): + conf = kw.pop('conf', CONF) + group = _ensure_group(kw, conf) + return conf.register_cli_opt(cfg.IntOpt(*args, **kw), group=group) + + def _ensure_group(kw, conf): group = kw.pop('group', None) if group: diff --git a/keystone/identity/backends/kvs.py b/keystone/identity/backends/kvs.py index 26209355e9..8215054b49 100644 --- a/keystone/identity/backends/kvs.py +++ b/keystone/identity/backends/kvs.py @@ -2,6 +2,7 @@ from keystone import identity from keystone.common import kvs +from keystone.common import utils def _filter_user(user_ref): @@ -12,6 +13,13 @@ def _filter_user(user_ref): return user_ref +def _ensure_hashed_password(user_ref): + pw = user_ref.get('password', None) + if pw is not None: + user_ref['password'] = utils.hash_password(pw) + return user_ref + + class Identity(kvs.Base, identity.Driver): # Public interface def authenticate(self, user_id=None, tenant_id=None, password=None): @@ -24,7 +32,8 @@ class Identity(kvs.Base, identity.Driver): user_ref = self._get_user(user_id) tenant_ref = None metadata_ref = None - if not user_ref or user_ref.get('password') != password: + if (not user_ref + or not utils.check_password(password, user_ref.get('password'))): raise AssertionError('Invalid user / password') if tenant_id and tenant_id not in user_ref['tenants']: raise AssertionError('Invalid tenant') @@ -78,15 +87,13 @@ class Identity(kvs.Base, identity.Driver): user_ref = self._get_user(user_id) tenants = set(user_ref.get('tenants', [])) tenants.add(tenant_id) - user_ref['tenants'] = list(tenants) - self.update_user(user_id, user_ref) + self.update_user(user_id, {'tenants': list(tenants)}) def remove_user_from_tenant(self, tenant_id, user_id): user_ref = self._get_user(user_id) tenants = set(user_ref.get('tenants', [])) tenants.remove(tenant_id) - user_ref['tenants'] = list(tenants) - self.update_user(user_id, user_ref) + self.update_user(user_id, {'tenants': list(tenants)}) def get_tenants_for_user(self, user_id): user_ref = self._get_user(user_id) @@ -118,6 +125,7 @@ class Identity(kvs.Base, identity.Driver): # CRUD def create_user(self, user_id, user): + user = _ensure_hashed_password(user) self.db.set('user-%s' % user_id, user) self.db.set('user_name-%s' % user['name'], user) user_list = set(self.db.get('user_list', [])) @@ -128,10 +136,13 @@ class Identity(kvs.Base, identity.Driver): def update_user(self, user_id, user): # get the old name and delete it too old_user = self.db.get('user-%s' % user_id) + new_user = old_user.copy() + user = _ensure_hashed_password(user) + new_user.update(user) self.db.delete('user_name-%s' % old_user['name']) - self.db.set('user-%s' % user_id, user) - self.db.set('user_name-%s' % user['name'], user) - return user + self.db.set('user-%s' % user_id, new_user) + self.db.set('user_name-%s' % new_user['name'], new_user) + return new_user def delete_user(self, user_id): old_user = self.db.get('user-%s' % user_id) diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index a614ebf945..8a3db42248 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -2,6 +2,7 @@ from keystone import identity from keystone.common import sql +from keystone.common import utils from keystone.common.sql import migration @@ -11,6 +12,13 @@ def _filter_user(user_ref): return user_ref +def _ensure_hashed_password(user_ref): + pw = user_ref.get('password', None) + if pw is not None: + user_ref['password'] = utils.hash_password(pw) + return user_ref + + class User(sql.ModelBase, sql.DictBase): __tablename__ = 'user' id = sql.Column(sql.String(64), primary_key=True) @@ -106,7 +114,8 @@ class Identity(sql.Base, identity.Driver): user_ref = self._get_user(user_id) tenant_ref = None metadata_ref = None - if not user_ref or user_ref.get('password') != password: + if (not user_ref + or not utils.check_password(password, user_ref.get('password'))): raise AssertionError('Invalid user / password') tenants = self.get_tenants_for_user(user_id) @@ -246,6 +255,7 @@ class Identity(sql.Base, identity.Driver): # CRUD def create_user(self, user_id, user): + user = _ensure_hashed_password(user) session = self.get_session() with session.begin(): user_ref = User.from_dict(user) @@ -258,6 +268,7 @@ class Identity(sql.Base, identity.Driver): with session.begin(): user_ref = session.query(User).filter_by(id=user_id).first() old_user_dict = user_ref.to_dict() + user = _ensure_hashed_password(user) for k in user: old_user_dict[k] = user[k] new_user = User.from_dict(old_user_dict) diff --git a/keystone/identity/core.py b/keystone/identity/core.py index 484faf50ea..e841898545 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -334,13 +334,10 @@ class UserController(wsgi.Application): self.identity_api.add_user_to_tenant(tenant_id, user_id) return {'user': new_user_ref} - # NOTE(termie): this is really more of a patch than a put def update_user(self, context, user_id, user): + # NOTE(termie): this is really more of a patch than a put self.assert_admin(context) - user_ref = self.identity_api._get_user(context, user_id) - del user['id'] - user_ref.update(user) - self.identity_api.update_user(context, user_id, user_ref) + user_ref = self.identity_api.update_user(context, user_id, user) return {'user': user_ref} def delete_user(self, context, user_id): diff --git a/keystone/test.py b/keystone/test.py index 82d2c77df4..9e1137a7a5 100644 --- a/keystone/test.py +++ b/keystone/test.py @@ -107,6 +107,11 @@ class TestCase(unittest.TestCase): def setUp(self): super(TestCase, self).setUp() + self.config() + + def config(self): + CONF(config_files=[etcdir('keystone.conf'), + testsdir('test_overrides.conf')]) def tearDown(self): for path in self._paths: @@ -137,10 +142,11 @@ class TestCase(unittest.TestCase): for user in fixtures.USERS: user_copy = user.copy() tenants = user_copy.pop('tenants') - rv = self.identity_api.create_user(user['id'], user_copy) + rv = self.identity_api.create_user(user['id'], user_copy.copy()) for tenant_id in tenants: self.identity_api.add_user_to_tenant(tenant_id, user['id']) - setattr(self, 'user_%s' % user['id'], rv) + setattr(self, 'user_%s' % user['id'], user_copy) + print user_copy for role in fixtures.ROLES: rv = self.identity_api.create_role(role['id'], role) diff --git a/tests/test_backend.py b/tests/test_backend.py index 30ff3edad3..151700aa23 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -45,6 +45,11 @@ class IdentityTests(object): self.assertDictEquals(tenant_ref, self.tenant_bar) self.assertDictEquals(metadata_ref, self.metadata_foobar) + def test_password_hashed(self): + user_ref = self.identity_api._get_user(self.user_foo['id']) + self.assertNotEqual(user_ref['password'], self.user_foo['password']) + + def test_get_tenant_bad_tenant(self): tenant_ref = self.identity_api.get_tenant( tenant_id=self.tenant_bar['id'] + 'WRONG') diff --git a/tests/test_keystoneclient.py b/tests/test_keystoneclient.py index 32a8cbc26e..d38313d303 100644 --- a/tests/test_keystoneclient.py +++ b/tests/test_keystoneclient.py @@ -45,7 +45,6 @@ class KcMasterTestCase(CompatTestCase): from keystoneclient.v2_0 import client as ks_client reload(ks_client) - self._config() self.public_app = self.loadapp('keystone', name='main') self.admin_app = self.loadapp('keystone', name='admin') @@ -63,10 +62,6 @@ class KcMasterTestCase(CompatTestCase): self.user_foo['id'], self.tenant_bar['id'], dict(roles=['keystone_admin'], is_admin='1')) - def _config(self): - CONF(config_files=[test.etcdir('keystone.conf'), - test.testsdir('test_overrides.conf')]) - def get_client(self, user_ref=None, tenant_ref=None): if user_ref is None: user_ref = self.user_foo @@ -87,10 +82,9 @@ class KcMasterTestCase(CompatTestCase): self.assertEquals(tenants[0].id, self.tenant_bar['id']) def test_authenticate_tenant_id_and_tenants(self): - client = self._client(username='FOO', - password='foo2', + client = self._client(username=self.user_foo['name'], + password=self.user_foo['password'], tenant_id='bar') - tenants = client.tenants.list() self.assertEquals(tenants[0].id, self.tenant_bar['id']) diff --git a/tests/test_keystoneclient_sql.py b/tests/test_keystoneclient_sql.py index ce8ce01151..e365f83c5e 100644 --- a/tests/test_keystoneclient_sql.py +++ b/tests/test_keystoneclient_sql.py @@ -11,7 +11,7 @@ CONF = config.CONF class KcMasterSqlTestCase(test_keystoneclient.KcMasterTestCase): - def _config(self): + def config(self): CONF(config_files=[test.etcdir('keystone.conf'), test.testsdir('test_overrides.conf'), test.testsdir('backend_sql.conf')]) diff --git a/tests/test_overrides.conf b/tests/test_overrides.conf index fdeb6e4b2d..399110d7e8 100644 --- a/tests/test_overrides.conf +++ b/tests/test_overrides.conf @@ -1,2 +1,5 @@ +[DEFAULT] +bcrypt_strength = 2 + [catalog] template_file = default_catalog.templates diff --git a/tools/pip-requires b/tools/pip-requires index 062e94ee5d..d4157f8ef1 100644 --- a/tools/pip-requires +++ b/tools/pip-requires @@ -7,5 +7,6 @@ routes pycli sqlalchemy sqlalchemy-migrate +py-bcrypt -e git+https://github.com/openstack/python-keystoneclient.git#egg=python-keystoneclient