[PATCH] secure password reset implementation
Mads Kiilerich
mads at kiilerich.com
Mon Jul 20 14:57:20 UTC 2015
Nice - lot of improvements since the first iteration.
test_forgot_password is still failing?
Should some of the nice commit message perhaps be moved into the
documentation?
One thought: Before, the automaticly assigned password would be random
and very "secure". This makes it more easy for users to change their
password to something insecure ... whatever that means. There is thus
perhaps an increased need for a way to configure password security
policy instead of just the existing hardcoded defaults in the templates.
Inline comments:
On 07/19/2015 03:35 PM, Andrew Shadura wrote:
> # HG changeset patch
> # User Andrew Shadura <andrew at shadura.me>
> # Date 1431821238 -7200
> # Sun May 17 02:07:18 2015 +0200
> # Node ID 98cb64feddfb89f106f66763462061fd2ca3f412
> # Parent f103b1a2383bc4fba5d28f9732ba832025e3bf00
> secure password reset implementation
Please use a first line in the usual format that helps with the grouping
when creating release notes.
> 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 doesn't require state storing.
>
> The old implementation generated a new password and sent it
> in clear text to whatever email assigned to the user currently,
> invalidating the old password the very moment a new one is requested.
> Apart from potential insecurity, this makes it possible for anyone
> to disrupt users' workflow by repeatedly resetting their passwords.
>
> The idea behind this proposed implementation is to generate
This will very soon no longer just be a proposal! ;-)
> 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 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 (note: email template needs to be improved).
Is that still a TODO? Should it be done in this patch or later?
> 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.
>
> diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py
> --- a/kallithea/controllers/login.py
> +++ b/kallithea/controllers/login.py
> @@ -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, PasswordResetForm, PasswordResetConfirmationForm
> from kallithea.model.user import UserModel
> from kallithea.model.meta import Session
>
> @@ -197,12 +198,12 @@ class LoginController(BaseController):
> error_dict = {'recaptcha_field': _msg}
> raise formencode.Invalid(_msg, _value, None,
> error_dict=error_dict)
> - UserModel().reset_password_link(form_result)
> + redirect_link = UserModel().reset_password_link(form_result)
> h.flash(_('Your password reset link was sent'),
> category='success')
> - return redirect(url('login_home'))
> + return redirect(redirect_link)
>
> - except formencode.Invalid, errors:
> + except formencode.Invalid as errors:
This is probably a good idea to consistently do everywhere ... in a
separate changeset.
> return htmlfill.render(
> render('/password_reset.html'),
> defaults=errors.value,
> @@ -214,17 +215,45 @@ class LoginController(BaseController):
> return render('/password_reset.html')
>
> def password_reset_confirmation(self):
> - if request.GET and request.GET.get('key'):
> + if request.GET:
> + c.data = dict(
> + email = request.GET.get('email'),
> + timestamp = request.GET.get('timestamp'),
> + token = request.GET.get('token')
> + )
Please add a trailing comma to the last item (when each item has its own
line and the ending parens is on the next line) - that makes it a
one-liner diff to add a new line and it won't be forgotten. (And the
same below.)
> + if c.data['token']:
> + log.debug("data = %s" % c.data)
Perhaps add a bit extra text here to make the debug log entry more
useful for the admin debugging something?
Also, please use
log.debug("data = %s", c.data)
to avoid unnecessary string expansion when debugging is disabled.
> + if UserModel().reset_password_confirm(c.data):
> + return render('/password_reset_confirmation.html')
> + else:
> + h.flash(_('Invalid password reset token'),
> + category='error')
> + return redirect(url('reset_password'))
> + else:
> + return render('/password_reset_confirmation.html')
> + elif request.POST:
> + password_reset_confirmation_form = PasswordResetConfirmationForm()()
> + c.data = dict(
> + email = request.POST.get('email'),
> + password = request.POST.get('password'),
> + password_confirm = request.POST.get('password_confirm'),
> + timestamp = request.POST.get('timestamp'),
> + token = request.POST.get('token')
> + )
> 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, e:
> - log.error(e)
> - return redirect(url('reset_password'))
> + log.debug("data = %s" % c.data)
> + form_result = password_reset_confirmation_form.to_python(dict(request.POST))
> + UserModel().reset_password(c.data)
> + h.flash(_('Successfully updated password'),
> + category='success')
> + 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",
> + force_defaults=False)
>
> return redirect(url('login_home'))
>
> diff --git a/kallithea/lib/utils2.py b/kallithea/lib/utils2.py
> --- a/kallithea/lib/utils2.py
> +++ b/kallithea/lib/utils2.py
> @@ -34,6 +34,7 @@ import uuid
> import datetime
> import urllib
> import binascii
> +import hashlib
>
> import webob
> import urlobject
> @@ -170,6 +171,9 @@ def generate_api_key():
> return binascii.hexlify(os.urandom(20))
>
>
> +def hash_values(*args):
> + return hashlib.sha1('\0'.join([str(arg) for arg in args])).hexdigest()
> +
Docstring?
+1 for using a 0 delimiter (which however also put some constraints on
which data this function will work 'fully correctly' on).
There is no need for creating the list for join - just use the generator
directly as
join(str(arg) for arg in args)
Perhaps even assume that the caller is aware of the types and already
converted all arguments to strings?
> def safe_int(val, default=None):
> """
> Returns int() of val if val is not convertable to int use default
> diff --git a/kallithea/model/forms.py b/kallithea/model/forms.py
> --- a/kallithea/model/forms.py
> +++ b/kallithea/model/forms.py
> @@ -205,6 +205,17 @@ def PasswordResetForm():
> email = v.Email(not_empty=True)
> return _PasswordResetForm
>
> +def PasswordResetConfirmationForm():
> + class _PasswordResetConfirmationForm(formencode.Schema):
> + allow_extra_fields = True
> + filter_extra_fields = 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
> @@ -27,13 +27,16 @@ Original author and date, and relevant c
>
>
> import logging
> +import time
> import traceback
> +from pylons import config
> +from pylons import session
> from pylons.i18n.translation import _
>
> from sqlalchemy.exc import DatabaseError
>
> from kallithea import EXTERN_TYPE_INTERNAL
> -from kallithea.lib.utils2 import safe_unicode, generate_api_key, get_current_authuser
> +from kallithea.lib.utils2 import safe_unicode, generate_api_key, get_current_authuser, hash_values
> from kallithea.lib.caching_query import FromCache
> from kallithea.model import BaseModel
> from kallithea.model.db import User, UserToPerm, Notification, \
> @@ -280,6 +283,13 @@ class UserModel(BaseModel):
> from kallithea.lib.hooks import log_delete_user
> log_delete_user(user.get_dict(), cur_user)
>
> + def reset_password_token(self, user, timestamp):
> + return hash_values(user.user_id,
> + timestamp,
> + user.password,
> + session.id,
> + config.get('app_instance_uuid'))
> +
> def reset_password_link(self, data):
> from kallithea.lib.celerylib import tasks, run_task
> from kallithea.model.notification import EmailNotificationModel
> @@ -287,18 +297,24 @@ class UserModel(BaseModel):
>
> user_email = data['email']
> user = User.get_by_email(user_email)
> + timestamp = int(time.time())
> if user:
> log.debug('password reset user found %s' % user)
> + token = self.reset_password_token(user, timestamp)
> link = h.canonical_url('reset_password_confirmation',
> - key=user.api_key)
> + 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],
> @@ -307,27 +323,52 @@ class UserModel(BaseModel):
> else:
> log.debug("password reset email %s not found" % user_email)
>
> - return True
> + return h.canonical_url('reset_password_confirmation',
> + email=user_email,
> + timestamp=timestamp)
> +
> + def reset_password_confirm(self, data):
> + from kallithea.lib.celerylib import tasks, run_task
> + from kallithea.lib import auth
> + user = User.get_by_email(data['email'])
> + if not user:
> + log.debug("user with email %s not found" % data['email'])
> + return False
> +
> + now = int(time.time())
> + timestamp = int(data['timestamp'])
> +
> + if (timestamp is None) or (timestamp > now):
> + log.debug('timestamp is None or from the future')
> + return False
> +
> + # TODO for now, hardcode 24h, need to be a setting
> + if (now - timestamp) > 86400:
> + log.debug('token expired')
> + return False
> +
> + token = self.reset_password_token(user, data['timestamp'])
> + log.debug('computed token: %s' % token)
> + log.debug('received token: %s' % data['token'])
> + return token == data['token']
>
> def reset_password(self, data):
> from kallithea.lib.celerylib import tasks, run_task
> from kallithea.lib import auth
> - user_email = data['email']
> - user = User.get_by_email(user_email)
> - new_passwd = auth.PasswordGenerator().gen_password(
> - 8, auth.PasswordGenerator.ALPHABETS_BIG_SMALL)
> - if user:
> - user.password = auth.get_crypt_password(new_passwd)
> + user = User.get_by_email(data['email'])
> + if not user:
> + log.debug("user with email %s not found" % data['email'])
> +
> + if data['password']:
> + user.password = auth.get_crypt_password(data['password'])
> 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')
> + log.info('change password for %s' % user.username)
Perhaps make it more clear where in the process we are:
'changing password for %s'
>
> - 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)
> + run_task(tasks.send_email, [user.email],
> + _('Password change notification'),
> + _('Your Kallithea password has been changed'))
> + 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 @@
>
> <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 reset the password, click the following link')}:</p>
"reset" sounds a bit like the system is assigning a password. Perhaps
use wording like "To set a new password"? (But admittedly, setting a new
password is a way to reset it ...)
> <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>
> 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 reset the 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_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,73 @@
> +## -*- coding: utf-8 -*-
> +<%inherit file="base/root.html"/>
> +
> +<%block name="title">
> + ${_('Password Reset Confirmation')}
> +</%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">
> + %if c.data['email']:
> + <p>${_('You are about to reset password for the email address %s') % c.data['email']}</p>
Perhaps
... the password ...
> + ${h.form(h.canonical_url('reset_password_confirmation'), method='post' if c.data['token'] else 'get')}
> + <div style="display: none;">
> + ${h.hidden('email',value=c.data['email'])}
> + %if c.data['token']:
> + ${h.hidden('token',value=c.data['token'])}
> + %endif
> + %if c.data['timestamp']:
> + ${h.hidden('timestamp',value=c.data['timestamp'])}
> + %endif
> + </div>
> + %endif
> + <div class="form">
> + <!-- fields -->
> + <div class="fields">
> + %if c.data['token']:
> + <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>
> + %else:
> + <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>
> +
> + %endif
> +
> + <div class="buttons">
> + <div class="nohighlight">
> + ${h.submit('send',_('Confirm'),class_="btn")}
> + </div>
> + </div>
> + </div>
> + </div>
> + ${h.end_form()}
> + </div>
> + </div>
/Mads
More information about the kallithea-general
mailing list