Mercurial > kallithea
changeset 5457:f629e9a0c376
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.
| author | Andrew Shadura <andrew@shadura.me> |
|---|---|
| date | Sun, 17 May 2015 02:07:18 +0200 |
| parents | 2577ddeab331 |
| children | dd676aab3b4d |
| files | kallithea/controllers/login.py kallithea/model/forms.py kallithea/model/user.py kallithea/templates/email_templates/password_reset.html kallithea/templates/email_templates/password_reset.txt kallithea/templates/password_reset.html kallithea/templates/password_reset_confirmation.html kallithea/tests/functional/test_login.py |
| diffstat | 8 files changed, 277 insertions(+), 57 deletions(-) [+] |
line wrap: on
line diff
--- a/kallithea/controllers/login.py Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/controllers/login.py Sun May 17 02:07:18 2015 +0200 @@ -31,7 +31,7 @@ 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.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 @@ 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 @@ 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 @@ 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):
--- a/kallithea/model/forms.py Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/model/forms.py Sun May 17 02:07:18 2015 +0200 @@ -205,13 +205,27 @@ 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=[]):
--- a/kallithea/model/user.py Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/model/user.py Sun May 17 02:07:18 2015 +0200 @@ -26,8 +26,12 @@ """ +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 @@ class UserModel(BaseModel): + password_reset_token_lifetime = 86400 # 24 hours + cls = User def get(self, user_id, cache=False): @@ -271,25 +277,73 @@ 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 @@ 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
--- a/kallithea/templates/email_templates/password_reset.html Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/templates/email_templates/password_reset.html Sun May 17 02:07:18 2015 +0200 @@ -3,8 +3,10 @@ <h4>${_('Hello %s') % user}</h4> -<p>${_('We received a request to create a new password for your account.')}</p> -<p>${_('You can generate it by clicking following URL')}:</p> +<p>${_('We have received a request to reset the password for your account.')}</p> +<p>${_('To set a new password, click the following link')}:</p> <p><a href="${reset_url}">${reset_url}</a></p> -<p>${_("Please ignore this email if you did not request a new password .")}</p> +<p>${_("Should you not be able to use the link above, please type the following code into the password reset form")}: <code>${reset_token}</code></p> + +<p>${_("If it weren't you who requested the password reset, just disregard this message.")}</p>
--- a/kallithea/templates/email_templates/password_reset.txt Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/templates/email_templates/password_reset.txt Sun May 17 02:07:18 2015 +0200 @@ -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}
--- a/kallithea/templates/password_reset.html Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/templates/password_reset.html Sun May 17 02:07:18 2015 +0200 @@ -44,7 +44,7 @@ <div class="buttons"> <div class="nohighlight"> ${h.submit('send',_('Send Password Reset Email'),class_="btn")} - <div class="activation_msg">${_('Password reset link will be sent to the email address matching your username.')}</div> + <div class="activation_msg">${_('A password reset link will be sent to the specified email address if it is registered in the system.')}</div> </div> </div> </div>
--- a/kallithea/templates/password_reset_confirmation.html Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/templates/password_reset_confirmation.html Sun May 17 02:07:18 2015 +0200 @@ -0,0 +1,63 @@ +## -*- coding: utf-8 -*- +<%inherit file="base/root.html"/> + +<%block name="title"> + ${_('Reset Your Password')} +</%block> + +<div id="register"> + <%include file="/base/flash_msg.html"/> + <div class="title withlogo"> + %if c.site_name: + <h5>${_('Reset Your Password to %s') % c.site_name}</h5> + %else: + <h5>${_('Reset Your Password')}</h5> + %endif + </div> + <div class="inner"> + ${h.form(h.url('reset_password_confirmation'), method='post')} + <p>${_('You are about to set a new password for the email address %s.') % c.email}</p> + <p>${_('Note that you must use the same browser session for this as the one used to request the password reset.')}</p> + <div style="display: none;"> + ${h.hidden('email')} + ${h.hidden('timestamp')} + </div> + <div class="form"> + <!-- fields --> + <div class="fields"> + <div class="field"> + <div class="label"> + <label for="token">${_('Code you received in the email')}:</label> + </div> + <div class="input"> + ${h.text('token', class_='focus')} + </div> + </div> + + <div class="field"> + <div class="label"> + <label for="password">${_('New Password')}:</label> + </div> + <div class="input"> + ${h.password('password',class_='focus')} + </div> + </div> + + <div class="field"> + <div class="label"> + <label for="password_confirm">${_('Confirm New Password')}:</label> + </div> + <div class="input"> + ${h.password('password_confirm',class_='focus')} + </div> + </div> + <div class="buttons"> + <div class="nohighlight"> + ${h.submit('send',_('Confirm'),class_="btn")} + </div> + </div> + </div> + </div> + ${h.end_form()} + </div> + </div>
--- a/kallithea/tests/functional/test_login.py Thu Jul 09 22:17:26 2015 +0200 +++ b/kallithea/tests/functional/test_login.py Sun May 17 02:07:18 2015 +0200 @@ -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 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 @@ 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 @@ email = 'username@python-works.com' name = 'passwd' lastname = 'reset' + timestamp = int(time.time()) new = User() new.username = username @@ -360,34 +368,58 @@ 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