# HG changeset patch # User Andrew Shadura # Date 2015-05-17 02:07:18 # Node ID f629e9a0c3761bca8ed5c952521a7279c79ebf80 # Parent 2577ddeab3318cb82d1ea019ff4c3f68ff3fe3af auth: secure password reset implementation This is a better implementation of password reset function, which doesn't involve sending a new password to the user's email address in clear text, and at the same time is stateless. The old implementation generated a new password and sent it in clear text to whatever email assigned to the user currently, so that any user, possibly unauthenticated, could request a reset for any username or email. Apart from potential insecurity, this made it possible for anyone to disrupt users' workflow by repeatedly resetting their passwords. The idea behind this implementation is to generate an authentication token which is dependent on the user state at the time before the password change takes place, so the token is one-time and can't be reused, and also to bind the token to the browser session. The token is calculated as SHA1 hash of the following: * user's identifier (number, not a name) * timestamp * hashed user's password * session identifier * per-application secret We use numeric user's identifier, as it's fixed and doesn't change, so renaming users doesn't affect the mechanism. Timestamp is added to make it possible to limit the token's validness (currently hard coded to 24h), and we don't want users to be able to fake that field easily. Hashed user's password is needed to prevent using the token again once the password has been changed. Session identifier is an additional security measure to ensure someone else stealing the token can't use it. Finally, per-application secret is just another way to make it harder for an attacker to guess all values in an attempt to generate a valid token. When the token is generated, an anonymous user is directed to a confirmation page where the timestamp and the usernames are already preloaded, so the user needs to specify the token. User can either click the link in the email if it's really them reading it, or to type the token manually. Using the right token in the same session as it was requested directs the user to a password change form, where the user is supposed to specify a new password (twice, of course). Upon completing the form (which is POSTed) the password change happens and a notification mail is sent. The test is updated to test the basic functionality with a bad and a good token, but it doesn't (yet) cover all code paths. The original work from Andrew has been thorougly reviewed and heavily modified by Søren Løvborg. diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py --- a/kallithea/controllers/login.py +++ b/kallithea/controllers/login.py @@ -31,7 +31,7 @@ import formencode import urlparse from formencode import htmlfill -from webob.exc import HTTPFound +from webob.exc import HTTPFound, HTTPBadRequest from pylons.i18n.translation import _ from pylons.controllers.util import redirect from pylons import request, session, tmpl_context as c, url @@ -42,7 +42,8 @@ from kallithea.lib.base import BaseContr from kallithea.lib.exceptions import UserCreationError from kallithea.lib.utils2 import safe_str from kallithea.model.db import User, Setting -from kallithea.model.forms import LoginForm, RegisterForm, PasswordResetForm +from kallithea.model.forms import \ + LoginForm, RegisterForm, PasswordResetRequestForm, PasswordResetConfirmationForm from kallithea.model.user import UserModel from kallithea.model.meta import Session @@ -182,7 +183,7 @@ class LoginController(BaseController): c.captcha_public_key = settings.get('captcha_public_key') if request.POST: - password_reset_form = PasswordResetForm()() + password_reset_form = PasswordResetRequestForm()() try: form_result = password_reset_form.to_python(dict(request.POST)) if c.captcha_active: @@ -197,10 +198,10 @@ class LoginController(BaseController): error_dict = {'recaptcha_field': _msg} raise formencode.Invalid(_msg, _value, None, error_dict=error_dict) - UserModel().reset_password_link(form_result) - h.flash(_('Your password reset link was sent'), + redirect_link = UserModel().send_reset_password_email(form_result) + h.flash(_('A password reset confirmation code has been sent'), category='success') - return redirect(url('login_home')) + return redirect(redirect_link) except formencode.Invalid as errors: return htmlfill.render( @@ -214,18 +215,45 @@ class LoginController(BaseController): return render('/password_reset.html') def password_reset_confirmation(self): - if request.GET and request.GET.get('key'): - try: - user = User.get_by_api_key(request.GET.get('key')) - data = dict(email=user.email) - UserModel().reset_password(data) - h.flash(_('Your password reset was successful, ' - 'new password has been sent to your email'), - category='success') - except Exception as e: - log.error(e) - return redirect(url('reset_password')) + # This controller handles both GET and POST requests, though we + # only ever perform the actual password change on POST (since + # GET requests are not allowed to have side effects, and do not + # receive automatic CSRF protection). + + # The template needs the email address outside of the form. + c.email = request.params.get('email') + + if not request.POST: + return htmlfill.render( + render('/password_reset_confirmation.html'), + defaults=dict(request.params), + encoding='UTF-8') + form = PasswordResetConfirmationForm()() + try: + form_result = form.to_python(dict(request.POST)) + except formencode.Invalid as errors: + return htmlfill.render( + render('/password_reset_confirmation.html'), + defaults=errors.value, + errors=errors.error_dict or {}, + prefix_error=False, + encoding='UTF-8') + + if not UserModel().verify_reset_password_token( + form_result['email'], + form_result['timestamp'], + form_result['token'], + ): + return htmlfill.render( + render('/password_reset_confirmation.html'), + defaults=form_result, + errors={'token': _('Invalid password reset token')}, + prefix_error=False, + encoding='UTF-8') + + UserModel().reset_password(form_result['email'], form_result['password']) + h.flash(_('Successfully updated password'), category='success') return redirect(url('login_home')) def logout(self): diff --git a/kallithea/model/forms.py b/kallithea/model/forms.py --- a/kallithea/model/forms.py +++ b/kallithea/model/forms.py @@ -205,13 +205,27 @@ def RegisterForm(edit=False, old_data={} return _RegisterForm -def PasswordResetForm(): - class _PasswordResetForm(formencode.Schema): +def PasswordResetRequestForm(): + class _PasswordResetRequestForm(formencode.Schema): allow_extra_fields = True filter_extra_fields = True email = v.Email(not_empty=True) - return _PasswordResetForm + return _PasswordResetRequestForm + +def PasswordResetConfirmationForm(): + class _PasswordResetConfirmationForm(formencode.Schema): + allow_extra_fields = True + filter_extra_fields = True + email = v.UnicodeString(strip=True, not_empty=True) + timestamp = v.Number(strip=True, not_empty=True) + token = v.UnicodeString(strip=True, not_empty=True) + password = All(v.ValidPassword(), v.UnicodeString(strip=False, min=6)) + password_confirm = All(v.ValidPassword(), v.UnicodeString(strip=False, min=6)) + + chained_validators = [v.ValidPasswordsMatch('password', + 'password_confirm')] + return _PasswordResetConfirmationForm def RepoForm(edit=False, old_data={}, supported_backends=BACKENDS.keys(), repo_groups=[], landing_revs=[]): diff --git a/kallithea/model/user.py b/kallithea/model/user.py --- a/kallithea/model/user.py +++ b/kallithea/model/user.py @@ -26,8 +26,12 @@ Original author and date, and relevant c """ +import hashlib import logging +import time import traceback + +from pylons import config from pylons.i18n.translation import _ from sqlalchemy.exc import DatabaseError @@ -47,6 +51,8 @@ log = logging.getLogger(__name__) class UserModel(BaseModel): + password_reset_token_lifetime = 86400 # 24 hours + cls = User def get(self, user_id, cache=False): @@ -271,25 +277,73 @@ class UserModel(BaseModel): from kallithea.lib.hooks import log_delete_user log_delete_user(user.get_dict(), cur_user) - def reset_password_link(self, data): + def get_reset_password_token(self, user, timestamp, session_id): + """ + The token is calculated as SHA1 hash of the following: + + * user's identifier (number, not a name) + * timestamp + * hashed user's password + * session identifier + * per-application secret + + We use numeric user's identifier, as it's fixed and doesn't change, + so renaming users doesn't affect the mechanism. Timestamp is added + to make it possible to limit the token's validness (currently hard + coded to 24h), and we don't want users to be able to fake that field + easily. Hashed user's password is needed to prevent using the token + again once the password has been changed. Session identifier is + an additional security measure to ensure someone else stealing the + token can't use it. Finally, per-application secret is just another + way to make it harder for an attacker to guess all values in an + attempt to generate a valid token. + """ + return hashlib.sha1('\0'.join([ + str(user.user_id), + str(timestamp), + user.password, + session_id, + config.get('app_instance_uuid'), + ])).hexdigest() + + def send_reset_password_email(self, data): + """ + Sends email with a password reset token and link to the password + reset confirmation page with all information (including the token) + pre-filled. Also returns URL of that page, only without the token, + allowing users to copy-paste or manually enter the token from the + email. + """ from kallithea.lib.celerylib import tasks, run_task from kallithea.model.notification import EmailNotificationModel import kallithea.lib.helpers as h user_email = data['email'] user = User.get_by_email(user_email) + timestamp = int(time.time()) if user is not None: - log.debug('password reset user found %s', user) - link = h.canonical_url('reset_password_confirmation', - key=user.api_key) + log.debug('password reset user %s found', user) + token = self.get_reset_password_token(user, + timestamp, + h.authentication_token()) + # URL must be fully qualified; but since the token is locked to + # the current browser session, we must provide a URL with the + # current scheme and hostname, rather than the canonical_url. + link = h.url('reset_password_confirmation', qualified=True, + email=user_email, + timestamp=timestamp, + token=token) + reg_type = EmailNotificationModel.TYPE_PASSWORD_RESET body = EmailNotificationModel().get_email_tmpl( reg_type, 'txt', user=user.short_contact, + reset_token=token, reset_url=link) html_body = EmailNotificationModel().get_email_tmpl( reg_type, 'html', user=user.short_contact, + reset_token=token, reset_url=link) log.debug('sending email') run_task(tasks.send_email, [user_email], @@ -298,27 +352,52 @@ class UserModel(BaseModel): else: log.debug("password reset email %s not found", user_email) - return True + return h.url('reset_password_confirmation', + email=user_email, + timestamp=timestamp) - def reset_password(self, data): + def verify_reset_password_token(self, email, timestamp, token): from kallithea.lib.celerylib import tasks, run_task from kallithea.lib import auth - user_email = data['email'] + import kallithea.lib.helpers as h + user = User.get_by_email(email) + if user is None: + log.debug("user with email %s not found", email) + return False + + token_age = int(time.time()) - int(timestamp) + + if token_age < 0: + log.debug('timestamp is from the future') + return False + + if token_age > UserModel.password_reset_token_lifetime: + log.debug('password reset token expired') + return False + + expected_token = self.get_reset_password_token(user, + timestamp, + h.authentication_token()) + log.debug('computed password reset token: %s', expected_token) + log.debug('received password reset token: %s', token) + return expected_token == token + + def reset_password(self, user_email, new_passwd): + from kallithea.lib.celerylib import tasks, run_task + from kallithea.lib import auth user = User.get_by_email(user_email) - new_passwd = auth.PasswordGenerator().gen_password( - 8, auth.PasswordGenerator.ALPHABETS_BIG_SMALL) if user is not None: user.password = auth.get_crypt_password(new_passwd) Session().add(user) Session().commit() log.info('change password for %s', user_email) if new_passwd is None: - raise Exception('unable to generate new password') + raise Exception('unable to set new password') run_task(tasks.send_email, [user_email], - _('Your new password'), - _('Your new Kallithea password:%s') % (new_passwd,)) - log.info('send new password mail to %s', user_email) + _('Password reset notification'), + _('The password to your account %s has been changed using password reset form.') % (user.username,)) + log.info('send password reset mail to %s', user_email) return True diff --git a/kallithea/templates/email_templates/password_reset.html b/kallithea/templates/email_templates/password_reset.html --- a/kallithea/templates/email_templates/password_reset.html +++ b/kallithea/templates/email_templates/password_reset.html @@ -3,8 +3,10 @@

${_('Hello %s') % user}

-

${_('We received a request to create a new password for your account.')}

-

${_('You can generate it by clicking following URL')}:

+

${_('We have received a request to reset the password for your account.')}

+

${_('To set a new password, click the following link')}:

${reset_url}

-

${_("Please ignore this email if you did not request a new password .")}

+

${_("Should you not be able to use the link above, please type the following code into the password reset form")}: ${reset_token}

+ +

${_("If it weren't you who requested the password reset, just disregard this message.")}

diff --git a/kallithea/templates/email_templates/password_reset.txt b/kallithea/templates/email_templates/password_reset.txt --- a/kallithea/templates/email_templates/password_reset.txt +++ b/kallithea/templates/email_templates/password_reset.txt @@ -3,9 +3,11 @@ ${_('Hello %s') % user|n,unicode} -${_('We received a request to create a new password for your account.')|n,unicode} -${_('You can generate it by clicking following URL')|n,unicode}: +${_('We have received a request to reset the password for your account..')|n,unicode} +${_('To set a new password, click the following link')|n,unicode}: ${reset_url|n,unicode} -${_("Please ignore this email if you did not request a new password .")|n,unicode} +${_("Should you not be able to use the link above, please type the following code into the password reset form")|n,unicode}: ${reset_token|n,unicode} + +${_("If it weren't you who requested the password reset, just disregard this message.")|n,unicode} diff --git a/kallithea/templates/password_reset.html b/kallithea/templates/password_reset.html --- a/kallithea/templates/password_reset.html +++ b/kallithea/templates/password_reset.html @@ -44,7 +44,7 @@
${h.submit('send',_('Send Password Reset Email'),class_="btn")} -
${_('Password reset link will be sent to the email address matching your username.')}
+
${_('A password reset link will be sent to the specified email address if it is registered in the system.')}
diff --git a/kallithea/templates/password_reset_confirmation.html b/kallithea/templates/password_reset_confirmation.html --- a/kallithea/templates/password_reset_confirmation.html +++ b/kallithea/templates/password_reset_confirmation.html @@ -0,0 +1,63 @@ +## -*- coding: utf-8 -*- +<%inherit file="base/root.html"/> + +<%block name="title"> + ${_('Reset Your Password')} + + +
+ <%include file="/base/flash_msg.html"/> + +
+ ${h.form(h.url('reset_password_confirmation'), method='post')} +

${_('You are about to set a new password for the email address %s.') % c.email}

+

${_('Note that you must use the same browser session for this as the one used to request the password reset.')}

+
+ ${h.hidden('email')} + ${h.hidden('timestamp')} +
+
+ +
+
+
+ +
+
+ ${h.text('token', class_='focus')} +
+
+ +
+
+ +
+
+ ${h.password('password',class_='focus')} +
+
+ +
+
+ +
+
+ ${h.password('password_confirm',class_='focus')} +
+
+
+
+ ${h.submit('send',_('Confirm'),class_="btn")} +
+
+
+
+ ${h.end_form()} +
+
diff --git a/kallithea/tests/functional/test_login.py b/kallithea/tests/functional/test_login.py --- a/kallithea/tests/functional/test_login.py +++ b/kallithea/tests/functional/test_login.py @@ -1,8 +1,10 @@ # -*- coding: utf-8 -*- from __future__ import with_statement import re +import time import mock + from kallithea.tests import * from kallithea.tests.fixture import Fixture from kallithea.lib.utils2 import generate_api_key @@ -12,6 +14,7 @@ from kallithea.model.api_key import ApiK from kallithea.model import validators from kallithea.model.db import User, Notification from kallithea.model.meta import Session +from kallithea.model.user import UserModel fixture = Fixture() @@ -326,6 +329,10 @@ class TestLoginController(TestController self.assertNotEqual(ret.api_key, None) self.assertEqual(ret.admin, False) + #========================================================================== + # PASSWORD RESET + #========================================================================== + def test_forgot_password_wrong_mail(self): bad_email = 'username%wrongmail.org' response = self.app.post( @@ -345,6 +352,7 @@ class TestLoginController(TestController email = 'username@python-works.com' name = 'passwd' lastname = 'reset' + timestamp = int(time.time()) new = User() new.username = username @@ -360,34 +368,58 @@ class TestLoginController(TestController action='password_reset'), {'email': email, }) - self.checkSessionFlash(response, 'Your password reset link was sent') + self.checkSessionFlash(response, 'A password reset confirmation code has been sent') response = response.follow() - # BAD KEY + # BAD TOKEN + + token = "bad" - key = "bad" + response = self.app.post(url(controller='login', + action='password_reset_confirmation'), + {'email': email, + 'timestamp': timestamp, + 'password': "p@ssw0rd", + 'password_confirm': "p@ssw0rd", + 'token': token, + }) + self.assertEqual(response.status, '200 OK') + response.mustcontain('Invalid password reset token') + + # GOOD TOKEN + + # TODO: The token should ideally be taken from the mail sent + # above, instead of being recalculated. + + token = UserModel().get_reset_password_token( + User.get_by_username(username), timestamp, self.authentication_token()) + response = self.app.get(url(controller='login', action='password_reset_confirmation', - key=key)) - self.assertEqual(response.status, '302 Found') - self.assertTrue(response.location.endswith(url('reset_password'))) - - # GOOD KEY + email=email, + timestamp=timestamp, + token=token)) + self.assertEqual(response.status, '200 OK') + response.mustcontain("You are about to set a new password for the email address %s" % email) - key = User.get_by_username(username).api_key - response = self.app.get(url(controller='login', - action='password_reset_confirmation', - key=key)) + response = self.app.post(url(controller='login', + action='password_reset_confirmation'), + {'email': email, + 'timestamp': timestamp, + 'password': "p@ssw0rd", + 'password_confirm': "p@ssw0rd", + 'token': token, + }) self.assertEqual(response.status, '302 Found') - self.assertTrue(response.location.endswith(url('login_home'))) - - self.checkSessionFlash(response, - ('Your password reset was successful, ' - 'new password has been sent to your email')) + self.checkSessionFlash(response, 'Successfully updated password') response = response.follow() + #========================================================================== + # API + #========================================================================== + def _get_api_whitelist(self, values=None): config = {'api_access_controllers_whitelist': values or []} return config