From f986a631a25c8fa547d07d2fae4bd2b4ac1c2b9a Mon Sep 17 00:00:00 2001 From: Gabriel Hurley Date: Thu, 24 May 2012 15:25:35 -0700 Subject: [PATCH] Make sure Horizon is treating passwords securely. * Applies the sensitive_post_parameters and sensitive_variables decorators to functions that handle sensitive data. * Defines a custom Exception Filter class to provide some added security. * Adds notes on logging to the docs. Fixes bug 1004114 for Horizon. Change-Id: I13ac91d91e0ed2322cc61633b02455cfed39fdcd --- docs/source/topics/deployment.rst | 28 +++++++++++++ horizon/dashboards/syspanel/users/forms.py | 9 +++- horizon/dashboards/syspanel/users/views.py | 12 ++++++ horizon/exceptions.py | 48 ++++++++++++++++++++++ horizon/views/auth.py | 7 ++++ horizon/views/auth_forms.py | 3 ++ openstack_dashboard/settings.py | 2 + openstack_dashboard/views.py | 6 +-- 8 files changed, 110 insertions(+), 5 deletions(-) diff --git a/docs/source/topics/deployment.rst b/docs/source/topics/deployment.rst index e60797b6d..16f4e1057 100644 --- a/docs/source/topics/deployment.rst +++ b/docs/source/topics/deployment.rst @@ -5,6 +5,34 @@ Deploying Horizon This guide aims to cover some common questions, concerns and pitfalls you may encounter when deploying Horizon in a production environment. +Logging +======= + +Logging is an important concern for prouction deployments, and the intricacies +of good logging configuration go far beyond what can be covered here. However +there are a few points worth noting about the logging included with Horizon, +how to customize it, and where other components may take over: + +* Horizon's logging uses Django's logging configuration mechanism, which + can be customized in your ``local_settings.py`` file through the + ``LOGGING`` dictionary. +* Horizon's default logging example sets the log level to ``"INFO"``, which is + a reasonable choice for production deployments. For development, however, + you may want to change the log level to ``"DEBUG"``. +* Horizon also uses a number of 3rd-party clients which log separately. The + log level for these can still be controlled through Horizon's ``LOGGING`` + config, however behaviors may vary beyond Horizon's control. + +.. warning:: + + At this time there is `a known bug in python-keystoneclient`_ where it will + log the complete request body of any request sent to Keystone through it + (including logging passwords in plain text) when the log level is set to + ``"DEBUG"``. If this behavior is not desired, make sure your log level is + ``"INFO"`` or higher. + +.. _a known bug in python-keystoneclient: https://bugs.launchpad.net/keystone/+bug/1004114 + Session Storage =============== diff --git a/horizon/dashboards/syspanel/users/forms.py b/horizon/dashboards/syspanel/users/forms.py index 71de09426..d4bcacde1 100644 --- a/horizon/dashboards/syspanel/users/forms.py +++ b/horizon/dashboards/syspanel/users/forms.py @@ -22,8 +22,9 @@ import logging from django import shortcuts from django.contrib import messages -from django.utils.translation import force_unicode, ugettext_lazy as _ from django.forms import ValidationError +from django.utils.translation import force_unicode, ugettext_lazy as _ +from django.views.decorators.debug import sensitive_variables from horizon import api from horizon import exceptions @@ -72,6 +73,9 @@ class CreateUserForm(BaseUserForm): widget=forms.PasswordInput(render_value=False)) tenant_id = forms.ChoiceField(label=_("Primary Project")) + # We have to protect the entire "data" dict because it contains the + # password and confirm_password strings. + @sensitive_variables('data') def handle(self, request, data): try: LOG.info('Creating user with name "%s"' % data['name']) @@ -123,6 +127,9 @@ class UpdateUserForm(BaseUserForm): for field in ('name', 'email', 'password', 'confirm_password'): self.fields.pop(field) + # We have to protect the entire "data" dict because it contains the + # password and confirm_password strings. + @sensitive_variables('data', 'password') def handle(self, request, data): failed, succeeded = [], [] user_is_editable = api.keystone_can_edit_user() diff --git a/horizon/dashboards/syspanel/users/views.py b/horizon/dashboards/syspanel/users/views.py index b3a46154a..9255b96c1 100644 --- a/horizon/dashboards/syspanel/users/views.py +++ b/horizon/dashboards/syspanel/users/views.py @@ -21,7 +21,9 @@ import logging from django.core.urlresolvers import reverse +from django.utils.decorators import method_decorator from django.utils.translation import ugettext_lazy as _ +from django.views.decorators.debug import sensitive_post_parameters from horizon import api from horizon import exceptions @@ -53,6 +55,11 @@ class UpdateView(forms.ModalFormView): template_name = 'syspanel/users/update.html' context_object_name = 'user' + @method_decorator(sensitive_post_parameters('password', + 'confirm_password')) + def dispatch(self, *args, **kwargs): + return super(UpdateView, self).dispatch(*args, **kwargs) + def get_object(self, *args, **kwargs): user_id = kwargs['user_id'] try: @@ -73,3 +80,8 @@ class UpdateView(forms.ModalFormView): class CreateView(forms.ModalFormView): form_class = CreateUserForm template_name = 'syspanel/users/create.html' + + @method_decorator(sensitive_post_parameters('password', + 'confirm_password')) + def dispatch(self, *args, **kwargs): + return super(CreateView, self).dispatch(*args, **kwargs) diff --git a/horizon/exceptions.py b/horizon/exceptions.py index dd7ee4658..78364bf59 100644 --- a/horizon/exceptions.py +++ b/horizon/exceptions.py @@ -24,8 +24,11 @@ import sys from django.conf import settings from django.contrib import messages +from django.http import HttpRequest from django.utils import termcolors from django.utils.translation import ugettext as _ +from django.views.debug import SafeExceptionReporterFilter, CLEANSED_SUBSTITUTE + from cloudfiles import errors as swiftclient from glanceclient.common import exceptions as glanceclient from keystoneclient import exceptions as keystoneclient @@ -36,6 +39,51 @@ LOG = logging.getLogger(__name__) PALETTE = termcolors.PALETTES[termcolors.DEFAULT_PALETTE] +class HorizonReporterFilter(SafeExceptionReporterFilter): + """ Error report filter that's always active, even in DEBUG mode. """ + def is_active(self, request): + return True + + # TODO(gabriel): When Django bug #18379 is fixed, this whole method + # can be removed: https://code.djangoproject.com/ticket/18379 + def get_traceback_frame_variables(self, request, tb_frame): + """ + Replaces the values of variables marked as sensitive with + stars (*********). + """ + func_name = tb_frame.f_code.co_name + func = tb_frame.f_globals.get(func_name) + # Methods won't be in the global namespace, func could be None here... + if func is None and "self" in tb_frame.f_locals: + func = getattr(tb_frame.f_locals.get('self'), func_name, None) + sensitive_variables = getattr(func, 'sensitive_variables', []) + cleansed = [] + if self.is_active(request) and sensitive_variables: + if sensitive_variables == '__ALL__': + # Cleanse all variables + for name, value in tb_frame.f_locals.items(): + cleansed.append((name, CLEANSED_SUBSTITUTE)) + return cleansed + else: + # Cleanse specified variables + for name, value in tb_frame.f_locals.items(): + if name in sensitive_variables: + value = CLEANSED_SUBSTITUTE + elif isinstance(value, HttpRequest): + # Cleanse the request's POST parameters. + value = self.get_request_repr(value) + cleansed.append((name, value)) + return cleansed + else: + # Cleanse only the request if it's one of the frame variables. + for name, value in tb_frame.f_locals.items(): + if isinstance(value, HttpRequest): + # Cleanse the request's POST parameters. + value = self.get_request_repr(value) + cleansed.append((name, value)) + return cleansed + + class HorizonException(Exception): """ Base exception class for distinguishing our own exception classes. """ pass diff --git a/horizon/views/auth.py b/horizon/views/auth.py index 5120eed76..3af37f936 100644 --- a/horizon/views/auth.py +++ b/horizon/views/auth.py @@ -23,7 +23,9 @@ import logging from django import shortcuts from django.conf import settings from django.contrib.auth import REDIRECT_FIELD_NAME +from django.utils.decorators import method_decorator from django.utils.translation import ugettext as _ +from django.views.decorators.debug import sensitive_post_parameters import horizon from horizon import api @@ -50,6 +52,10 @@ class LoginView(forms.ModalFormView): form_class = Login template_name = "horizon/auth/login.html" + @method_decorator(sensitive_post_parameters('password')) + def dispatch(self, *args, **kwargs): + return super(LoginView, self).dispatch(*args, **kwargs) + def get_context_data(self, **kwargs): context = super(LoginView, self).get_context_data(**kwargs) redirect_to = self.request.REQUEST.get(REDIRECT_FIELD_NAME, "") @@ -67,6 +73,7 @@ class LoginView(forms.ModalFormView): return initial +@sensitive_post_parameters("password") def switch_tenants(request, tenant_id): """ Swaps a user from one tenant to another using the unscoped token from diff --git a/horizon/views/auth_forms.py b/horizon/views/auth_forms.py index 2ebecfcda..ec1e9a011 100644 --- a/horizon/views/auth_forms.py +++ b/horizon/views/auth_forms.py @@ -29,6 +29,8 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth import REDIRECT_FIELD_NAME from django.utils.translation import ugettext as _ +from django.views.decorators.debug import sensitive_variables + from keystoneclient import exceptions as keystone_exceptions from horizon import api @@ -76,6 +78,7 @@ class Login(forms.SelfHandlingForm): self.fields['region'].initial = default_region[0] self.fields['region'].widget = forms.widgets.HiddenInput() + @sensitive_variables("data") def handle(self, request, data): if 'user_name' in request.session: if request.session['user_name'] != data['username']: diff --git a/openstack_dashboard/settings.py b/openstack_dashboard/settings.py index 1aa184740..344a75141 100644 --- a/openstack_dashboard/settings.py +++ b/openstack_dashboard/settings.py @@ -127,6 +127,8 @@ USE_I18N = True OPENSTACK_KEYSTONE_DEFAULT_ROLE = 'Member' +DEFAULT_EXCEPTION_REPORTER_FILTER = 'horizon.exceptions.HorizonReporterFilter' + try: from local.local_settings import * except ImportError: diff --git a/openstack_dashboard/views.py b/openstack_dashboard/views.py index c41baf52c..e643acbc2 100644 --- a/openstack_dashboard/views.py +++ b/openstack_dashboard/views.py @@ -25,7 +25,7 @@ from django import shortcuts from django.views.decorators import vary import horizon -from horizon.views import auth as auth_views +from horizon.views import auth_forms def qunit_tests(request): @@ -42,8 +42,6 @@ def user_home(user): def splash(request): if request.user.is_authenticated(): return shortcuts.redirect(user_home(request.user)) - form, handled = auth_views.Login.maybe_handle(request) - if handled: - return handled + form = auth_forms.Login() request.session.clear() return shortcuts.render(request, 'splash.html', {'form': form})