From 6dc670c851d31b12ffa0f07f418b74705e3b5902 Mon Sep 17 00:00:00 2001 From: Ignatious Johnson Christopher Date: Wed, 9 Nov 2016 16:08:03 -0800 Subject: [PATCH] Issue: Password is displayed in the log files of the config daemon, during uncaught exceptions. Fix: cgitb sets sys.excepthook to format uncaught exceptions. Deriving the cgitb Hook and modifying the handle method to mask password along with formatting. Change-Id: I5b4251f2ebe0205465b15430a9ef38ef04b3a634 Closes-Bug: 1626317 --- src/config/api-server/db_manage.py | 8 +- src/config/api-server/tests/test_askip.py | 5 +- .../api-server/tests/test_crud_basic.py | 5 +- src/config/api-server/tests/test_ip_alloc.py | 5 +- .../api-server/tests/test_logical_router.py | 4 +- src/config/api-server/tests/test_perms.py | 5 +- src/config/api-server/tests/test_perms2.py | 5 +- src/config/api-server/tests/test_rbac.py | 5 +- .../api-server/tests/test_subnet_ip_count.py | 4 +- src/config/api-server/vnc_cfg_api_server.py | 5 +- src/config/common/SConscript | 1 + src/config/common/tests/test_common.py | 10 +- src/config/common/tests/test_discovery.py | 11 +- src/config/common/utils.py | 63 +--------- src/config/common/vnc_cgitb.py | 116 ++++++++++++++++++ .../device_manager/device_manager.py | 4 +- src/config/schema-transformer/to_bgp.py | 4 +- .../svc-monitor/svc_monitor/svc_monitor.py | 4 +- src/discovery/disc_server.py | 4 +- src/discovery/disc_server_zk.py | 4 +- 20 files changed, 162 insertions(+), 110 deletions(-) create mode 100644 src/config/common/vnc_cgitb.py diff --git a/src/config/api-server/db_manage.py b/src/config/api-server/db_manage.py index 648d206abd8..b02a021917a 100644 --- a/src/config/api-server/db_manage.py +++ b/src/config/api-server/db_manage.py @@ -8,11 +8,11 @@ from netaddr import IPAddress, IPNetwork import argparse from cStringIO import StringIO -import cgitb import kazoo.client import kazoo.exceptions import cfgm_common +from cfgm_common import vnc_cgitb from cfgm_common.utils import cgitb_hook from cfgm_common.ifmap.client import client from cfgm_common.ifmap.request import NewSessionRequest @@ -1452,7 +1452,7 @@ def heal_security_groups_id(self): # end class DatabaseCleaner def db_check(args_str): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') db_checker = DatabaseChecker(args_str) # Mode and node count check across all nodes @@ -1469,7 +1469,7 @@ def db_check(args_str): # end db_check def db_clean(args_str): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') db_cleaner = DatabaseCleaner(args_str) db_cleaner.clean_obj_missing_mandatory_fields() @@ -1484,7 +1484,7 @@ def db_clean(args_str): # end db_clean def db_heal(args_str): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') db_healer = DatabaseHealer(args_str) db_healer.heal_fq_name_index() diff --git a/src/config/api-server/tests/test_askip.py b/src/config/api-server/tests/test_askip.py index 00259d5b2f7..6200b8c6168 100644 --- a/src/config/api-server/tests/test_askip.py +++ b/src/config/api-server/tests/test_askip.py @@ -10,9 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') - import testtools from testtools.matchers import Equals, MismatchError, Not, Contains from testtools import content, content_type, ExpectedException @@ -29,6 +26,8 @@ import vnc_api.gen.vnc_api_test_gen from vnc_api.gen.resource_test import * import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') from test_utils import * diff --git a/src/config/api-server/tests/test_crud_basic.py b/src/config/api-server/tests/test_crud_basic.py index 7e48718b9fc..a761d2ee7dd 100644 --- a/src/config/api-server/tests/test_crud_basic.py +++ b/src/config/api-server/tests/test_crud_basic.py @@ -13,9 +13,6 @@ import netaddr import tempfile -import cgitb -cgitb.enable(format='text') - import fixtures import testtools from testtools.matchers import Equals, MismatchError, Not, Contains, LessThan @@ -41,6 +38,8 @@ import cfgm_common from cfgm_common import vnc_plugin_base from cfgm_common import imid +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') from test_utils import * diff --git a/src/config/api-server/tests/test_ip_alloc.py b/src/config/api-server/tests/test_ip_alloc.py index 0015301770a..3d54c51c28e 100644 --- a/src/config/api-server/tests/test_ip_alloc.py +++ b/src/config/api-server/tests/test_ip_alloc.py @@ -10,9 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') - import testtools from testtools.matchers import Equals, MismatchError, Not, Contains from testtools import content, content_type, ExpectedException @@ -29,6 +26,8 @@ import vnc_api.gen.vnc_api_test_gen from vnc_api.gen.resource_test import * import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') from test_utils import * diff --git a/src/config/api-server/tests/test_logical_router.py b/src/config/api-server/tests/test_logical_router.py index 7706b39d312..87b0c152f16 100644 --- a/src/config/api-server/tests/test_logical_router.py +++ b/src/config/api-server/tests/test_logical_router.py @@ -10,8 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') import testtools from testtools.matchers import Equals, MismatchError, Not, Contains @@ -32,6 +30,8 @@ from netaddr import IPNetwork, IPAddress import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') from test_utils import * diff --git a/src/config/api-server/tests/test_perms.py b/src/config/api-server/tests/test_perms.py index acdd7ee510b..c96b9862f38 100644 --- a/src/config/api-server/tests/test_perms.py +++ b/src/config/api-server/tests/test_perms.py @@ -11,9 +11,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') - import fixtures import testtools from testtools.matchers import Equals, MismatchError, Not, Contains @@ -29,6 +26,8 @@ from vnc_api.vnc_api import * import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') import test_utils diff --git a/src/config/api-server/tests/test_perms2.py b/src/config/api-server/tests/test_perms2.py index 0c3e012dede..b5ce2044407 100644 --- a/src/config/api-server/tests/test_perms2.py +++ b/src/config/api-server/tests/test_perms2.py @@ -10,9 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') - import fixtures import testtools from testtools.matchers import Equals, MismatchError, Not, Contains @@ -34,6 +31,8 @@ from cfgm_common import rest, utils from cfgm_common.rbaclib import * import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') import test_utils diff --git a/src/config/api-server/tests/test_rbac.py b/src/config/api-server/tests/test_rbac.py index 4a6a000f663..9a5989fdb63 100644 --- a/src/config/api-server/tests/test_rbac.py +++ b/src/config/api-server/tests/test_rbac.py @@ -10,9 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') - import fixtures import testtools from testtools.matchers import Equals, MismatchError, Not, Contains @@ -32,6 +29,8 @@ from keystonemiddleware import auth_token from cfgm_common import rest, utils import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') import test_utils diff --git a/src/config/api-server/tests/test_subnet_ip_count.py b/src/config/api-server/tests/test_subnet_ip_count.py index 4bb1632aeb3..229495a02c0 100644 --- a/src/config/api-server/tests/test_subnet_ip_count.py +++ b/src/config/api-server/tests/test_subnet_ip_count.py @@ -10,8 +10,6 @@ import logging import coverage -import cgitb -cgitb.enable(format='text') import testtools from testtools.matchers import Equals, MismatchError, Not, Contains @@ -29,6 +27,8 @@ import vnc_api.gen.vnc_api_test_gen from vnc_api.gen.resource_test import * import cfgm_common +from cfgm_common import vnc_cgitb +vnc_cgitb.enable(format='text') sys.path.append('../common/tests') from test_utils import * diff --git a/src/config/api-server/vnc_cfg_api_server.py b/src/config/api-server/vnc_cfg_api_server.py index 249b0b736b0..ed8ae74fd30 100644 --- a/src/config/api-server/vnc_cfg_api_server.py +++ b/src/config/api-server/vnc_cfg_api_server.py @@ -34,6 +34,8 @@ from lxml import etree # import GreenletProfiler +from cfgm_common import vnc_cgitb + logger = logging.getLogger(__name__) """ @@ -3553,8 +3555,7 @@ def main(args_str=None): # end main def server_main(args_str=None): - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() #server_main diff --git a/src/config/common/SConscript b/src/config/common/SConscript index b961928724a..30ce74652e5 100644 --- a/src/config/common/SConscript +++ b/src/config/common/SConscript @@ -40,6 +40,7 @@ local_sources = [ 'dependency_tracker.py', 'vnc_api_stats.py', 'ssl_adapter.py', + 'vnc_cgitb.py', ] local_sources_rules = [] for file in local_sources: diff --git a/src/config/common/tests/test_common.py b/src/config/common/tests/test_common.py index f3b99d1184b..af880123511 100644 --- a/src/config/common/tests/test_common.py +++ b/src/config/common/tests/test_common.py @@ -24,6 +24,7 @@ import cfgm_common.zkclient from cfgm_common.uve.vnc_api.ttypes import VncApiConfigLog from cfgm_common import imid +from cfgm_common import vnc_cgitb from cfgm_common.utils import cgitb_hook from test_utils import * @@ -148,8 +149,7 @@ def launch_disc_server(test_id, listen_ip, listen_port, http_server_port, conf_s args_str = args_str + "--log_local " args_str = args_str + "--log_file discovery_server_%s.log " % test_id - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') with tempfile.NamedTemporaryFile() as conf, tempfile.NamedTemporaryFile() as logconf: cfg_parser = generate_conf_file_contents(conf_sections) @@ -219,8 +219,7 @@ def launch_api_server(test_id, listen_ip, listen_port, http_server_port, args_str = args_str + "--log_local " args_str = args_str + "--log_file api_server_%s.log " %(test_id) - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') with tempfile.NamedTemporaryFile() as conf, tempfile.NamedTemporaryFile() as logconf: cfg_parser = generate_conf_file_contents(conf_sections) @@ -427,8 +426,7 @@ def __init__(self, *args, **kwargs): self.addOnException(self._add_detailed_traceback) def _add_detailed_traceback(self, exc_info): - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') from cStringIO import StringIO tmp_file = StringIO() diff --git a/src/config/common/tests/test_discovery.py b/src/config/common/tests/test_discovery.py index 5d2db3f8595..bcbcff9f21e 100644 --- a/src/config/common/tests/test_discovery.py +++ b/src/config/common/tests/test_discovery.py @@ -27,6 +27,9 @@ import gevent.wsgi import uuid +from cfgm_common import vnc_cgitb + + def lineno(): """Returns the current line number in our program.""" return inspect.currentframe().f_back.f_lineno @@ -103,8 +106,7 @@ def launch_disc_server(listen_ip, listen_port, http_server_port, conf_sections): args_str = args_str + "--log_local " args_str = args_str + "--log_file discovery_server_sandesh.log " - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') with tempfile.NamedTemporaryFile() as conf, tempfile.NamedTemporaryFile() as logconf: cfg_parser = generate_conf_file_contents(conf_sections) @@ -163,12 +165,11 @@ def __init__(self, *args, **kwargs): } def _add_detailed_traceback(self, exc_info): - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') from cStringIO import StringIO tmp_file = StringIO() - cgitb.Hook(format="text", file=tmp_file).handle(exc_info) + vnc_cgitb.Hook(format="text", file=tmp_file).handle(exc_info) tb_str = tmp_file.getvalue() tmp_file.close() self.addDetail('detailed-traceback', content.text_content(tb_str)) diff --git a/src/config/common/utils.py b/src/config/common/utils.py index 9fb45c1fe16..e1382ccb582 100644 --- a/src/config/common/utils.py +++ b/src/config/common/utils.py @@ -22,76 +22,17 @@ import os -import re import urllib from collections import OrderedDict import sys -import cgitb import cStringIO import logging - -# Masking of password from openstack/common/log.py -_SANITIZE_KEYS = ['adminPass', 'admin_pass', 'password', 'admin_password'] - -# NOTE(ldbragst): Let's build a list of regex objects using the list of -# _SANITIZE_KEYS we already have. This way, we only have to add the new key -# to the list of _SANITIZE_KEYS and we can generate regular expressions -# for XML and JSON automatically. -_SANITIZE_PATTERNS = [] -_FORMAT_PATTERNS = [r'(%(key)s\s*[=]\s*[\"\']).*?([\"\'])', - r'(<%(key)s>).*?()', - r'([\"\']%(key)s[\"\']\s*:\s*[\"\']).*?([\"\'])', - r'([\'"].*?%(key)s[\'"]\s*:\s*u?[\'"]).*?([\'"])'] - -for key in _SANITIZE_KEYS: - for pattern in _FORMAT_PATTERNS: - reg_ex = re.compile(pattern % {'key': key}, re.DOTALL) - _SANITIZE_PATTERNS.append(reg_ex) - - -def mask_password(message, secret="***"): - """Replace password with 'secret' in message. - :param message: The string which includes security information. - :param secret: value with which to replace passwords. - :returns: The unicode value of message with the password fields masked. - - For example: - - >>> mask_password("'adminPass' : 'aaaaa'") - "'adminPass' : '***'" - >>> mask_password("'admin_pass' : 'aaaaa'") - "'admin_pass' : '***'" - >>> mask_password('"password" : "aaaaa"') - '"password" : "***"' - >>> mask_password("'original_password' : 'aaaaa'") - "'original_password' : '***'" - >>> mask_password("u'original_password' : u'aaaaa'") - "u'original_password' : u'***'" - """ - if not any(key in message for key in _SANITIZE_KEYS): - return message - - secret = r'\g<1>' + secret + r'\g<2>' - for pattern in _SANITIZE_PATTERNS: - message = re.sub(pattern, secret, message) - return message -# end mask_password +from cfgm_common import vnc_cgitb def cgitb_hook(info=None, **kwargs): - buf = sys.stdout - if 'file' in kwargs: - buf = kwargs['file'] - - local_buf = cStringIO.StringIO() - kwargs['file'] = local_buf - cgitb.Hook(**kwargs).handle(info or sys.exc_info()) - - doc = local_buf.getvalue() - local_buf.close() - buf.write(mask_password(doc)) - buf.flush() + vnc_cgitb.Hook(**kwargs).handle(info or sys.exc_info()) # end cgitb_hook diff --git a/src/config/common/vnc_cgitb.py b/src/config/common/vnc_cgitb.py new file mode 100644 index 00000000000..d14e469792a --- /dev/null +++ b/src/config/common/vnc_cgitb.py @@ -0,0 +1,116 @@ +"""Traceback formatting with masked password for vnc config daemons. + +To enable this module, do: + + import vnc_cgitb; vnc_cgitb.enable() + +at the top of your script. The optional arguments to enable() are: + + display - if true, tracebacks are displayed in the web browser + logdir - if set, tracebacks are written to files in this directory + context - number of lines of source code to show for each stack frame + format - 'text' or 'html' controls the output format + +By default, tracebacks are displayed but not saved, the context is 5 lines +and the output format is 'html' (for backwards compatibility with the +original use of this module) + +If you have caught an exception and want vnc_cgitb to display it +after masking passwords, call vn_cgitb.handler(). The optional argument to +handler() is a 3-item tuple (etype, evalue, etb) just like the value of +sys.exc_info(). The default handler displays output as text. + +Alternatively, if you have caught an exception and want to collect the +formatted/masked traceback as string and use it , +>>> string_buf = StringIO() +>>> vnc_cgitb.Hook(file=string_buf, format="text").handle(sys.exc_info()) +>>> formatted_tb = string_buf.get_value() + +""" +import re +import sys +import cgitb +import cStringIO + + +# Masking of password from openstack/common/log.py +_SANITIZE_KEYS = ['adminPass', 'admin_pass', 'password', 'admin_password'] + +# NOTE(ldbragst): Let's build a list of regex objects using the list of +# _SANITIZE_KEYS we already have. This way, we only have to add the new key +# to the list of _SANITIZE_KEYS and we can generate regular expressions +# for XML and JSON automatically. +_SANITIZE_PATTERNS = [] +_FORMAT_PATTERNS = [r'(%(key)s\s*[=]\s*[\"\']).*?([\"\'])', + r'(<%(key)s>).*?()', + r'([\"\']%(key)s[\"\']\s*:\s*[\"\']).*?([\"\'])', + r'([\'"].*?%(key)s[\'"]\s*:\s*u?[\'"]).*?([\'"])'] + +for key in _SANITIZE_KEYS: + for pattern in _FORMAT_PATTERNS: + reg_ex = re.compile(pattern % {'key': key}, re.DOTALL) + _SANITIZE_PATTERNS.append(reg_ex) + + +def mask_password(message, secret="***"): + """Replace password with 'secret' in message. + :param message: The string which includes security information. + :param secret: value with which to replace passwords. + :returns: The unicode value of message with the password fields masked. + + For example: + + >>> mask_password("'adminPass' : 'aaaaa'") + "'adminPass' : '***'" + >>> mask_password("'admin_pass' : 'aaaaa'") + "'admin_pass' : '***'" + >>> mask_password('"password" : "aaaaa"') + '"password" : "***"' + >>> mask_password("'original_password' : 'aaaaa'") + "'original_password' : '***'" + >>> mask_password("u'original_password' : u'aaaaa'") + "u'original_password' : u'***'" + """ + if not any(key in message for key in _SANITIZE_KEYS): + return message + + secret = r'\g<1>' + secret + r'\g<2>' + for pattern in _SANITIZE_PATTERNS: + message = re.sub(pattern, secret, message) + return message + + +class Hook(cgitb.Hook): + """A hook to replace sys.excepthook that masks password from tracebacks. + Derived from the standard cgitb Hook class. + """ + + def handle(self, info=None): + # Format the traceback and store it in StringIO buffer + local_buf = cStringIO.StringIO() + kwargs = {'display': self.display, + 'logdir': self.logdir, + 'context': self.context, + 'file': local_buf, + 'format': self.format, + } + cgitb.Hook(**kwargs).handle(info) + # Retrive the formatted traceback from StringIO buffer, + # mask the passwords and write to the IO + doc = local_buf.getvalue() + local_buf.close() + self.file.write(mask_password(doc)) + self.file.flush() + + +handler = Hook(format='text').handle + + +def enable(display=1, logdir=None, context=5, format="html"): + """Install an exception handler that formats tracebacks as HTML. + + The optional argument 'display' can be set to 0 to suppress sending the + traceback to the browser, and 'logdir' can be set to a directory to cause + tracebacks to be written to files there.""" + sys.excepthook = Hook(display=display, logdir=logdir, + context=context, format=format) diff --git a/src/config/device-manager/device_manager/device_manager.py b/src/config/device-manager/device_manager/device_manager.py index cc709813b07..a817fb12d0d 100644 --- a/src/config/device-manager/device_manager/device_manager.py +++ b/src/config/device-manager/device_manager/device_manager.py @@ -11,7 +11,6 @@ from gevent import monkey monkey.patch_all() from cfgm_common.vnc_kombu import VncKombuClient -import cgitb import sys import argparse import requests @@ -42,6 +41,7 @@ GlobalVRouterConfigDM, FloatingIpDM, InstanceIpDM, DMCassandraDB, PortTupleDM from physical_router_config import PushConfigState from cfgm_common.dependency_tracker import DependencyTracker +from cfgm_common import vnc_cgitb from cfgm_common.utils import cgitb_hook @@ -606,7 +606,7 @@ def run_device_manager(args): def server_main(): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() # end server_main diff --git a/src/config/schema-transformer/to_bgp.py b/src/config/schema-transformer/to_bgp.py index 573ca8f007b..0ac0c57f6da 100644 --- a/src/config/schema-transformer/to_bgp.py +++ b/src/config/schema-transformer/to_bgp.py @@ -18,13 +18,13 @@ sys.setdefaultencoding('UTF8') import requests import ConfigParser -import cgitb import argparse import socket from cfgm_common import vnc_cpu_info +from cfgm_common import vnc_cgitb from cfgm_common.exceptions import * from config_db import * @@ -857,7 +857,7 @@ def main(args_str=None): def server_main(): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() # end server_main diff --git a/src/config/svc-monitor/svc_monitor/svc_monitor.py b/src/config/svc-monitor/svc_monitor/svc_monitor.py index 84444a6b901..d670fc560f1 100644 --- a/src/config/svc-monitor/svc_monitor/svc_monitor.py +++ b/src/config/svc-monitor/svc_monitor/svc_monitor.py @@ -16,7 +16,6 @@ from cfgm_common.zkclient import ZookeeperClient import requests import ConfigParser -import cgitb import cStringIO import argparse @@ -28,6 +27,7 @@ from cfgm_common.imid import * from cfgm_common import importutils from cfgm_common import svc_info +from cfgm_common import vnc_cgitb from cfgm_common.utils import cgitb_hook from config_db import * @@ -887,7 +887,7 @@ def main(args_str=None): def server_main(): - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() # end server_main diff --git a/src/discovery/disc_server.py b/src/discovery/disc_server.py index 15b666b52c0..1f31cd0b035 100644 --- a/src/discovery/disc_server.py +++ b/src/discovery/disc_server.py @@ -52,6 +52,7 @@ from gevent.lock import BoundedSemaphore from cfgm_common.rest import LinkObject +from cfgm_common import vnc_cgitb import disc_auth_keystone @@ -1505,8 +1506,7 @@ def main(args_str=None): # end main def server_main(): - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() # end server_main diff --git a/src/discovery/disc_server_zk.py b/src/discovery/disc_server_zk.py index e71f9f47ef5..5b2445e6056 100644 --- a/src/discovery/disc_server_zk.py +++ b/src/discovery/disc_server_zk.py @@ -45,6 +45,7 @@ from gevent.lock import BoundedSemaphore from cfgm_common.rest import LinkObject +from cfgm_common import vnc_cgitb def obj_to_json(obj): @@ -1134,8 +1135,7 @@ def main(args_str=None): # end main def server_main(): - import cgitb - cgitb.enable(format='text') + vnc_cgitb.enable(format='text') main() # end server_main