From 7b2272f40998dc8eab5e591592b8cdc3dd1d69af Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Mon, 11 Jun 2018 15:00:57 -0700 Subject: [PATCH 01/12] Warn about insecure config permissions on the config directory and the config file --- st2client/st2client/base.py | 15 ++- st2client/st2client/commands/auth.py | 2 + st2client/st2client/config_parser.py | 37 +++++++ st2client/tests/unit/test_auth.py | 27 ++++- st2client/tests/unit/test_config_parser.py | 122 +++++++++++++++++++++ st2client/tests/unit/test_shell.py | 14 ++- 6 files changed, 203 insertions(+), 14 deletions(-) diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index 5ed24db477..c1aedc1d89 100644 --- a/st2client/st2client/base.py +++ b/st2client/st2client/base.py @@ -225,7 +225,10 @@ def _get_cached_auth_token(self, client, username, password): :rtype: ``str`` """ if not os.path.isdir(ST2_CONFIG_DIRECTORY): - os.makedirs(ST2_CONFIG_DIRECTORY) + os.makedirs(ST2_CONFIG_DIRECTORY, mode=0o2770) + # os.makedirs straight up ignores the setgid bit, so we have to set + # it manually + os.chmod(ST2_CONFIG_DIRECTORY, 0o2770) cached_token_path = self._get_cached_token_path_for_user(username=username) @@ -253,7 +256,7 @@ def _get_cached_auth_token(self, client, username, password): file_st_mode = oct(os.stat(cached_token_path).st_mode & 0o777) others_st_mode = int(file_st_mode[-1]) - if others_st_mode >= 4: + if others_st_mode >= 2: # Every user has access to this file which is dangerous message = ('Permissions (%s) for cached token file "%s" are to permissive. Please ' 'restrict the permissions and make sure only your own user can read ' @@ -290,7 +293,10 @@ def _cache_auth_token(self, token_obj): :type token_obj: ``object`` """ if not os.path.isdir(ST2_CONFIG_DIRECTORY): - os.makedirs(ST2_CONFIG_DIRECTORY) + os.makedirs(ST2_CONFIG_DIRECTORY, mode=0o2770) + # os.makedirs straight up ignores the setgid bit, so we have to set + # it manually + os.chmod(ST2_CONFIG_DIRECTORY, 0o2770) username = token_obj.user cached_token_path = self._get_cached_token_path_for_user(username=username) @@ -326,9 +332,10 @@ def _cache_auth_token(self, token_obj): # open + chmod are two operations which means that during a short time frame (between # open and chmod) when file can potentially be read by other users if the default # permissions used during create allow that. - fd = os.open(cached_token_path, os.O_WRONLY | os.O_CREAT, 0o600) + fd = os.open(cached_token_path, os.O_WRONLY | os.O_CREAT, 0o660) with os.fdopen(fd, 'w') as fp: fp.write(data) + os.chmod(cached_token_path, 0o660) self.LOG.debug('Token has been cached in "%s"' % (cached_token_path)) return True diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index a9ed7541ce..5f95a36bca 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -18,6 +18,7 @@ import getpass import json import logging +import os import requests from six.moves.configparser import ConfigParser @@ -140,6 +141,7 @@ def run(self, args, **kwargs): with open(config_file, 'w') as cfg_file_out: config.write(cfg_file_out) + os.chmod(config_file, 0o660) return manager diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index a4a1aa1577..b0df04232f 100644 --- a/st2client/st2client/config_parser.py +++ b/st2client/st2client/config_parser.py @@ -19,6 +19,7 @@ from __future__ import absolute_import +import logging import os from collections import defaultdict @@ -120,6 +121,8 @@ class CLIConfigParser(object): + LOG = logging.getLogger(__name__) # logger instance to use + def __init__(self, config_file_path, validate_config_exists=True): if validate_config_exists and not os.path.isfile(config_file_path): raise ValueError('Config file "%s" doesn\'t exist') @@ -138,6 +141,40 @@ def parse(self): # Config doesn't exist, return the default values return CONFIG_DEFAULT_VALUES + config_dir_path = os.path.dirname(self.config_file_path) + + # Make sure the directory permissions == 0o770 + if bool(os.stat(config_dir_path).st_mode & 0o777 ^ 0o770): + self.LOG.warn( + # TODO: Perfect place for an f-string + "The StackStorm configuration directory permissions are " + "insecure (too permissive)." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod 770 {config_dir}".format(config_dir=config_dir_path)) + + # Make sure the setgid bit is set on the directory + if not bool(os.stat(config_dir_path).st_mode & 0o2000): + self.LOG.info( + # TODO: Perfect place for an f-string + "The SGID bit is not set on the StackStorm configuration " + "directory." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod g+s {config_dir}".format(config_dir=config_dir_path)) + + # Make sure the file permissions == 0o660 + if bool(os.stat(self.config_file_path).st_mode & 0o777 ^ 0o660): + self.LOG.warn( + # TODO: Another perfect place for an f-string + "The StackStorm configuration file permissions are insecure." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod 660 {config_file}".format(config_file=self.config_file_path)) + config = ConfigParser() with io.open(self.config_file_path, 'r', encoding='utf8') as fp: config.readfp(fp) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 1a2b667ed7..d2e49c100f 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -60,7 +60,9 @@ class TestLoginBase(base.BaseCLITestCase): """ DOTST2_PATH = os.path.expanduser('~/.st2/') - CONFIG_FILE = tempfile.mkstemp(suffix='st2.conf') + CONFIG_FILE_NAME = 'st2.conf' + PARENT_DIR = 'testconfig' + TMP_DIR = tempfile.mkdtemp() CONFIG_CONTENTS = """ [credentials] username = olduser @@ -78,9 +80,16 @@ def __init__(self, *args, **kwargs): self.parser.add_argument('--api-key', dest='api_key') self.shell = shell.Shell() + self.CONFIG_DIR = os.path.join(self.TMP_DIR, self.PARENT_DIR) + self.CONFIG_FILE = os.path.join(self.CONFIG_DIR, self.CONFIG_FILE_NAME) + def setUp(self): super(TestLoginBase, self).setUp() + if not os.path.isdir(self.CONFIG_DIR): + os.makedirs(self.CONFIG_DIR) + os.chmod(self.CONFIG_DIR, 0o2770) + # Remove any existing config file if os.path.isfile(self.CONFIG_FILE): os.remove(self.CONFIG_FILE) @@ -89,6 +98,8 @@ def setUp(self): for line in self.CONFIG_CONTENTS.split('\n'): cfg.write('%s\n' % line.strip()) + os.chmod(self.CONFIG_FILE, 0o660) + def tearDown(self): super(TestLoginBase, self).tearDown() @@ -99,10 +110,13 @@ def tearDown(self): for file in [f for f in os.listdir(self.DOTST2_PATH) if 'token-' in f]: os.remove(self.DOTST2_PATH + file) + # Clean up config directory + os.rmdir(self.CONFIG_DIR) + class TestLoginPasswordAndConfig(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', @@ -141,7 +155,7 @@ def runTest(self): class TestLoginIntPwdAndConfig(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', @@ -175,6 +189,9 @@ def runTest(self, mock_gp): } requests.post.assert_called_with('http://127.0.0.1:9100/tokens', '{}', **expected_kwargs) + # Check file permissions + self.assertEqual(os.stat(self.CONFIG_FILE).st_mode & 0o777, 0o660) + with open(self.CONFIG_FILE, 'r') as config_file: for line in config_file.readlines(): # Make sure certain values are not present @@ -203,7 +220,7 @@ def runTest(self, mock_gp): class TestLoginWritePwdOkay(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', @@ -244,7 +261,7 @@ def runTest(self, mock_gp): class TestLoginUncaughtException(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', diff --git a/st2client/tests/unit/test_config_parser.py b/st2client/tests/unit/test_config_parser.py index 6b77f3c14a..bb1c575ac6 100644 --- a/st2client/tests/unit/test_config_parser.py +++ b/st2client/tests/unit/test_config_parser.py @@ -16,7 +16,9 @@ from __future__ import absolute_import import os +import shutil +import mock import six import unittest2 @@ -37,6 +39,126 @@ def test_constructor(self): self.assertRaises(ValueError, CLIConfigParser, config_file_path='doestnotexist', validate_config_exists=True) + def test_correct_permissions_emit_no_warnings(self): + TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') + TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) + + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) + + try: + # Setup the config directory + os.makedirs(TEMP_CONFIG_DIR) + os.chmod(TEMP_CONFIG_DIR, 0o2770) + + self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + + # Setup the config file + shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) + os.chmod(TEMP_FILE_PATH, 0o660) + + self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o660) + + parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(TEMP_FILE_PATH)) + self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o660) + + self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + finally: + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + def test_warn_on_bad_config_permissions(self): + TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') + TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) + + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) + + try: + # Setup the config directory + os.makedirs(TEMP_CONFIG_DIR) + os.chmod(TEMP_CONFIG_DIR, 0o0755) + + self.assertNotEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) + + # Setup the config file + shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) + os.chmod(TEMP_FILE_PATH, 0o664) + + self.assertNotEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o770) + + parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.info.call_count, 1) + + self.assertEqual( + "The SGID bit is not set on the StackStorm configuration directory." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod g+s {config_dir}".format(config_dir=TEMP_CONFIG_DIR), + parser.LOG.info.call_args_list[0][0][0]) + + self.assertEqual(parser.LOG.warn.call_count, 2) + self.assertEqual( + "The StackStorm configuration directory permissions are insecure " + "(too permissive)." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod 770 {config_dir}".format(config_dir=TEMP_CONFIG_DIR), + parser.LOG.warn.call_args_list[0][0][0]) + + self.assertEqual( + "The StackStorm configuration file permissions are insecure." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod 660 {config_file}".format(config_file=TEMP_FILE_PATH), + parser.LOG.warn.call_args_list[1][0][0]) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(TEMP_FILE_PATH)) + self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o664) + + self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755) + finally: + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + def test_parse(self): # File doesn't exist parser = CLIConfigParser(config_file_path='doesnotexist', validate_config_exists=False) diff --git a/st2client/tests/unit/test_shell.py b/st2client/tests/unit/test_shell.py index f6788ded3f..2b9d48d6e2 100644 --- a/st2client/tests/unit/test_shell.py +++ b/st2client/tests/unit/test_shell.py @@ -465,8 +465,12 @@ def _write_mock_package_metadata_file(self): class CLITokenCachingTestCase(unittest2.TestCase): def setUp(self): super(CLITokenCachingTestCase, self).setUp() - self._mock_config_directory_path = tempfile.mkdtemp() + self._mock_temp_dir_path = tempfile.mkdtemp() + self._mock_config_directory_path = os.path.join(self._mock_temp_dir_path, 'testconfig') self._mock_config_path = os.path.join(self._mock_config_directory_path, 'config') + + os.makedirs(self._mock_config_directory_path) + self._p1 = mock.patch('st2client.base.ST2_CONFIG_DIRECTORY', self._mock_config_directory_path) self._p2 = mock.patch('st2client.base.ST2_CONFIG_PATH', @@ -508,7 +512,7 @@ def test_get_cached_auth_token_invalid_permissions(self): fp.write(json.dumps(data)) # 1. Current user doesn't have read access to the config directory - os.chmod(self._mock_config_directory_path, 0000) + os.chmod(self._mock_config_directory_path, 0o000) shell.LOG = mock.Mock() result = shell._get_cached_auth_token(client=client, username=username, @@ -524,7 +528,7 @@ def test_get_cached_auth_token_invalid_permissions(self): # 2. Read access on the directory, but not on the cached token file os.chmod(self._mock_config_directory_path, 0o777) # nosec - os.chmod(cached_token_path, 0000) + os.chmod(cached_token_path, 0o000) shell.LOG = mock.Mock() result = shell._get_cached_auth_token(client=client, username=username, @@ -570,7 +574,7 @@ def test_cache_auth_token_invalid_permissions(self): fp.write(json.dumps(data)) # 1. Current user has no write access to the parent directory - os.chmod(self._mock_config_directory_path, 0000) + os.chmod(self._mock_config_directory_path, 0o000) shell.LOG = mock.Mock() shell._cache_auth_token(token_obj=token_db) @@ -584,7 +588,7 @@ def test_cache_auth_token_invalid_permissions(self): # 2. Current user has no write access to the cached token file os.chmod(self._mock_config_directory_path, 0o777) # nosec - os.chmod(cached_token_path, 0000) + os.chmod(cached_token_path, 0o000) shell.LOG = mock.Mock() shell._cache_auth_token(token_obj=token_db) From e83f2dc61e154409ba857366b4325c431e01c152 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Thu, 21 Jun 2018 11:18:24 -0700 Subject: [PATCH 02/12] Update changelog --- CHANGELOG.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f0e1b92bdd..eada4e79be 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -40,6 +40,8 @@ Added Changed ~~~~~~~ +* Update st2 CLI to create the configuration directory and file, and authentication tokens with + secure permissions (eg: readable only to owner) * Refactor the callback module for the post run in runner to be more generic. (improvement) * Update various Python dependencies to the latest stable versions (gunicorn, gitpython, python-gnupg, tooz, flex). #4110 From 6d2af292d922ec05f47fb84c0ea9f3d27831aa17 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Thu, 21 Jun 2018 12:10:29 -0700 Subject: [PATCH 03/12] Don't try to chmod the config every time it's updated --- st2client/st2client/commands/auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 5f95a36bca..5eb54b9190 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -141,7 +141,6 @@ def run(self, args, **kwargs): with open(config_file, 'w') as cfg_file_out: config.write(cfg_file_out) - os.chmod(config_file, 0o660) return manager From 49b0933cd8f5350cd98ed52a9bf73d85ee207773 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Thu, 21 Jun 2018 13:10:24 -0700 Subject: [PATCH 04/12] Lint auth.py --- st2client/st2client/commands/auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 5eb54b9190..a9ed7541ce 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -18,7 +18,6 @@ import getpass import json import logging -import os import requests from six.moves.configparser import ConfigParser From bc1eee8e20c8879af8c0bfbe148eeb88da8b9205 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Thu, 21 Jun 2018 15:37:12 -0700 Subject: [PATCH 05/12] Use more complicated logic before adjusting file permissions --- st2client/st2client/commands/auth.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index a9ed7541ce..7c4489e545 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -18,6 +18,7 @@ import getpass import json import logging +import os import requests from six.moves.configparser import ConfigParser @@ -138,8 +139,12 @@ def run(self, args, **kwargs): # Remove any existing password from config config.remove_option('credentials', 'password') + config_existed = os.path.exists(config_file) with open(config_file, 'w') as cfg_file_out: config.write(cfg_file_out) + # If we created the config file, correct the permissions + if not config_existed: + os.chmod(config_file, 0o660) return manager From fd5d3cfcfe99b4d01b4adc7d28cea84681204381 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Fri, 22 Jun 2018 09:33:05 -0700 Subject: [PATCH 06/12] Tweak log configuration logic to silence messages --- st2client/st2client/base.py | 24 ++++--- st2client/st2client/config_parser.py | 74 ++++++++++++---------- st2client/st2client/shell.py | 2 +- st2client/tests/unit/test_config_parser.py | 56 +++++++++++++++- st2client/tests/unit/test_shell.py | 32 +++++++++- 5 files changed, 141 insertions(+), 47 deletions(-) diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index c1aedc1d89..c5e47a0d8b 100644 --- a/st2client/st2client/base.py +++ b/st2client/st2client/base.py @@ -153,24 +153,28 @@ def get_client(self, args, debug=False): return client - def _get_config_file_options(self, args): + def _get_config_file_options(self, args, validate_config_permissions=True): """ Parse the config and return kwargs which can be passed to the Client constructor. :rtype: ``dict`` """ - rc_options = self._parse_config_file(args=args) + rc_options = self._parse_config_file( + args=args, validate_config_permissions=validate_config_permissions) result = {} for kwarg_name, (section, option) in six.iteritems(CONFIG_OPTION_TO_CLIENT_KWARGS_MAP): result[kwarg_name] = rc_options.get(section, {}).get(option, None) return result - def _parse_config_file(self, args): + def _parse_config_file(self, args, validate_config_permissions=True): config_file_path = self._get_config_file_path(args=args) - parser = CLIConfigParser(config_file_path=config_file_path, validate_config_exists=False) + parser = CLIConfigParser(config_file_path=config_file_path, + validate_config_exists=False, + validate_config_permissions=validate_config_permissions, + log=self.LOG) result = parser.parse() return result @@ -258,10 +262,14 @@ def _get_cached_auth_token(self, client, username, password): if others_st_mode >= 2: # Every user has access to this file which is dangerous - message = ('Permissions (%s) for cached token file "%s" are to permissive. Please ' + message = ('Permissions (%s) for cached token file "%s" are too permissive. Please ' 'restrict the permissions and make sure only your own user can read ' - 'from the file' % (file_st_mode, cached_token_path)) - self.LOG.warn(message) + 'from or write to the file.' + '\n\n' + 'You can fix this by running:' + '\n\n' + ' chmod o-rw %s') + self.LOG.warn(message % (file_st_mode, cached_token_path, cached_token_path)) with open(cached_token_path) as fp: data = fp.read() @@ -356,7 +364,7 @@ def _get_cached_token_path_for_user(self, username): return result def _print_config(self, args): - config = self._parse_config_file(args=args) + config = self._parse_config_file(args=args, validate_config_permissions=False) for section, options in six.iteritems(config): print('[%s]' % (section)) diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index b0df04232f..d5fe7c3a07 100644 --- a/st2client/st2client/config_parser.py +++ b/st2client/st2client/config_parser.py @@ -121,13 +121,18 @@ class CLIConfigParser(object): - LOG = logging.getLogger(__name__) # logger instance to use - - def __init__(self, config_file_path, validate_config_exists=True): + def __init__(self, config_file_path, validate_config_exists=True, + validate_config_permissions=True, log=None): if validate_config_exists and not os.path.isfile(config_file_path): raise ValueError('Config file "%s" doesn\'t exist') + if log is None: + log = logging.getLogger(__name__) + logging.basicConfig() + self.config_file_path = config_file_path + self.validate_config_permissions = validate_config_permissions + self.LOG = log def parse(self): """ @@ -143,37 +148,38 @@ def parse(self): config_dir_path = os.path.dirname(self.config_file_path) - # Make sure the directory permissions == 0o770 - if bool(os.stat(config_dir_path).st_mode & 0o777 ^ 0o770): - self.LOG.warn( - # TODO: Perfect place for an f-string - "The StackStorm configuration directory permissions are " - "insecure (too permissive)." - "\n\n" - "You can fix this by running:" - "\n\n" - "chmod 770 {config_dir}".format(config_dir=config_dir_path)) - - # Make sure the setgid bit is set on the directory - if not bool(os.stat(config_dir_path).st_mode & 0o2000): - self.LOG.info( - # TODO: Perfect place for an f-string - "The SGID bit is not set on the StackStorm configuration " - "directory." - "\n\n" - "You can fix this by running:" - "\n\n" - "chmod g+s {config_dir}".format(config_dir=config_dir_path)) - - # Make sure the file permissions == 0o660 - if bool(os.stat(self.config_file_path).st_mode & 0o777 ^ 0o660): - self.LOG.warn( - # TODO: Another perfect place for an f-string - "The StackStorm configuration file permissions are insecure." - "\n\n" - "You can fix this by running:" - "\n\n" - "chmod 660 {config_file}".format(config_file=self.config_file_path)) + if self.validate_config_permissions: + # Make sure the directory permissions == 0o770 + if bool(os.stat(config_dir_path).st_mode & 0o777 ^ 0o770): + self.LOG.warn( + # TODO: Perfect place for an f-string + "The StackStorm configuration directory permissions are " + "insecure (too permissive)." + "\n\n" + "You can fix this by running:" + "\n\n" + " chmod 770 {config_dir}\n".format(config_dir=config_dir_path)) + + # Make sure the setgid bit is set on the directory + if not bool(os.stat(config_dir_path).st_mode & 0o2000): + self.LOG.info( + # TODO: Perfect place for an f-string + "The SGID bit is not set on the StackStorm configuration " + "directory." + "\n\n" + "You can fix this by running:" + "\n\n" + " chmod g+s {config_dir}\n".format(config_dir=config_dir_path)) + + # Make sure the file permissions == 0o660 + if bool(os.stat(self.config_file_path).st_mode & 0o777 ^ 0o660): + self.LOG.warn( + # TODO: Another perfect place for an f-string + "The StackStorm configuration file permissions are insecure." + "\n\n" + "You can fix this by running:" + "\n\n" + " chmod 660 {config_file}\n".format(config_file=self.config_file_path)) config = ConfigParser() with io.open(self.config_file_path, 'r', encoding='utf8') as fp: diff --git a/st2client/st2client/shell.py b/st2client/st2client/shell.py index ef021e2308..b5a2f23623 100755 --- a/st2client/st2client/shell.py +++ b/st2client/st2client/shell.py @@ -365,7 +365,7 @@ def run(self, argv): return 3 # Parse config and store it in the config module - config = self._parse_config_file(args=args) + config = self._parse_config_file(args=args, validate_config_permissions=False) set_config(config=config) self._check_locale_and_print_warning() diff --git a/st2client/tests/unit/test_config_parser.py b/st2client/tests/unit/test_config_parser.py index bb1c575ac6..a3d99de4c8 100644 --- a/st2client/tests/unit/test_config_parser.py +++ b/st2client/tests/unit/test_config_parser.py @@ -123,7 +123,7 @@ def test_warn_on_bad_config_permissions(self): "\n\n" "You can fix this by running:" "\n\n" - "chmod g+s {config_dir}".format(config_dir=TEMP_CONFIG_DIR), + " chmod g+s {config_dir}\n".format(config_dir=TEMP_CONFIG_DIR), parser.LOG.info.call_args_list[0][0][0]) self.assertEqual(parser.LOG.warn.call_count, 2) @@ -133,7 +133,7 @@ def test_warn_on_bad_config_permissions(self): "\n\n" "You can fix this by running:" "\n\n" - "chmod 770 {config_dir}".format(config_dir=TEMP_CONFIG_DIR), + " chmod 770 {config_dir}\n".format(config_dir=TEMP_CONFIG_DIR), parser.LOG.warn.call_args_list[0][0][0]) self.assertEqual( @@ -141,7 +141,7 @@ def test_warn_on_bad_config_permissions(self): "\n\n" "You can fix this by running:" "\n\n" - "chmod 660 {config_file}".format(config_file=TEMP_FILE_PATH), + " chmod 660 {config_file}\n".format(config_file=TEMP_FILE_PATH), parser.LOG.warn.call_args_list[1][0][0]) # Make sure we left the file alone @@ -159,6 +159,56 @@ def test_warn_on_bad_config_permissions(self): os.removedirs(TEMP_CONFIG_DIR) self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + def test_disable_permissions_warnings(self): + TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') + TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) + + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) + + try: + # Setup the config directory + os.makedirs(TEMP_CONFIG_DIR) + os.chmod(TEMP_CONFIG_DIR, 0o0755) + + self.assertNotEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) + + # Setup the config file + shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) + os.chmod(TEMP_FILE_PATH, 0o664) + + self.assertNotEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o770) + + parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, + validate_config_exists=True, + validate_config_permissions=False) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.info.call_count, 0) + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(TEMP_FILE_PATH)) + self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o664) + + self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755) + finally: + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + + if os.path.exists(TEMP_CONFIG_DIR): + os.removedirs(TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(TEMP_FILE_PATH)) + def test_parse(self): # File doesn't exist parser = CLIConfigParser(config_file_path='doesnotexist', validate_config_exists=False) diff --git a/st2client/tests/unit/test_shell.py b/st2client/tests/unit/test_shell.py index 2b9d48d6e2..a2d5ed6133 100644 --- a/st2client/tests/unit/test_shell.py +++ b/st2client/tests/unit/test_shell.py @@ -20,8 +20,10 @@ import datetime import json import logging +import shutil import tempfile +import requests import six import mock import unittest2 @@ -461,6 +463,34 @@ def _write_mock_package_metadata_file(self): return package_metadata_path + @mock.patch.object( + requests, 'get', + mock.MagicMock(return_value=base.FakeResponse("{}", 200, 'OK'))) + def test_dont_warn_multiple_times(self): + mock_temp_dir_path = tempfile.mkdtemp() + mock_config_dir_path = os.path.join(mock_temp_dir_path, 'testconfig') + mock_config_path = os.path.join(mock_config_dir_path, 'config') + + # Make the temporary config directory + os.makedirs(mock_config_dir_path) + + old_perms = os.stat(mock_config_dir_path).st_mode + new_perms = old_perms | 0o7 + os.chmod(mock_config_dir_path, new_perms) + + # Make the temporary config file + shutil.copyfile(CONFIG_FILE_PATH_FULL, mock_config_path) + os.chmod(mock_config_path, 0o777) # nosec + + shell = Shell() + shell.LOG = mock.Mock() + + # Test without token. + shell.run(['--config-file', mock_config_path, 'action', 'list']) + + self.assertEqual(shell.LOG.warn.call_count, 2) + self.assertEqual(shell.LOG.info.call_count, 1) + class CLITokenCachingTestCase(unittest2.TestCase): def setUp(self): @@ -553,7 +583,7 @@ def test_get_cached_auth_token_invalid_permissions(self): self.assertEqual(shell.LOG.warn.call_count, 1) log_message = shell.LOG.warn.call_args[0][0] - expected_msg = ('Permissions .*? for cached token file .*? are to permissive') + expected_msg = ('Permissions .*? for cached token file .*? are too permissive.*') self.assertRegexpMatches(log_message, expected_msg) def test_cache_auth_token_invalid_permissions(self): From a88528ed6e483ecacab593413dd31fd233d18ef4 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Tue, 26 Jun 2018 03:04:52 -0700 Subject: [PATCH 07/12] Test new log configuration logic --- st2client/tests/unit/test_shell.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/st2client/tests/unit/test_shell.py b/st2client/tests/unit/test_shell.py index a2d5ed6133..b97e57b7b0 100644 --- a/st2client/tests/unit/test_shell.py +++ b/st2client/tests/unit/test_shell.py @@ -489,7 +489,19 @@ def test_dont_warn_multiple_times(self): shell.run(['--config-file', mock_config_path, 'action', 'list']) self.assertEqual(shell.LOG.warn.call_count, 2) - self.assertEqual(shell.LOG.info.call_count, 1) + self.assertEqual( + shell.LOG.warn.call_args_list[0][0][0][:63], + 'The StackStorm configuration directory permissions are insecure') + self.assertEqual( + shell.LOG.warn.call_args_list[1][0][0][:59], + 'The StackStorm configuration file permissions are insecure.') + + self.assertEqual(shell.LOG.info.call_count, 2) + self.assertEqual( + shell.LOG.info.call_args_list[0][0][0][:19], "The SGID bit is not") + + self.assertEqual( + shell.LOG.info.call_args_list[1][0][0], 'Skipping parsing CLI config') class CLITokenCachingTestCase(unittest2.TestCase): From 9b54e09f656e7fcdeaaf58bcc7bc21416ea9e3bc Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Tue, 26 Jun 2018 12:04:46 -0700 Subject: [PATCH 08/12] Remove help text from log messages --- st2client/st2client/config_parser.py | 24 +++++----------------- st2client/tests/unit/test_config_parser.py | 18 +++------------- st2client/tests/unit/test_shell.py | 7 ++++--- 3 files changed, 12 insertions(+), 37 deletions(-) diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index d5fe7c3a07..dc4c6891a6 100644 --- a/st2client/st2client/config_parser.py +++ b/st2client/st2client/config_parser.py @@ -152,34 +152,20 @@ def parse(self): # Make sure the directory permissions == 0o770 if bool(os.stat(config_dir_path).st_mode & 0o777 ^ 0o770): self.LOG.warn( - # TODO: Perfect place for an f-string "The StackStorm configuration directory permissions are " - "insecure (too permissive)." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod 770 {config_dir}\n".format(config_dir=config_dir_path)) + "insecure (too permissive): others have access.") # Make sure the setgid bit is set on the directory if not bool(os.stat(config_dir_path).st_mode & 0o2000): self.LOG.info( - # TODO: Perfect place for an f-string "The SGID bit is not set on the StackStorm configuration " - "directory." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod g+s {config_dir}\n".format(config_dir=config_dir_path)) + "directory.") # Make sure the file permissions == 0o660 - if bool(os.stat(self.config_file_path).st_mode & 0o777 ^ 0o660): + if bool(os.stat(self.config_file_path).st_mode & 0o667 ^ 0o660): self.LOG.warn( - # TODO: Another perfect place for an f-string - "The StackStorm configuration file permissions are insecure." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod 660 {config_file}\n".format(config_file=self.config_file_path)) + "The StackStorm configuration file permissions are " + "insecure: others have access.") config = ConfigParser() with io.open(self.config_file_path, 'r', encoding='utf8') as fp: diff --git a/st2client/tests/unit/test_config_parser.py b/st2client/tests/unit/test_config_parser.py index a3d99de4c8..7d155bb667 100644 --- a/st2client/tests/unit/test_config_parser.py +++ b/st2client/tests/unit/test_config_parser.py @@ -119,29 +119,17 @@ def test_warn_on_bad_config_permissions(self): self.assertEqual(parser.LOG.info.call_count, 1) self.assertEqual( - "The SGID bit is not set on the StackStorm configuration directory." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod g+s {config_dir}\n".format(config_dir=TEMP_CONFIG_DIR), + "The SGID bit is not set on the StackStorm configuration directory.", parser.LOG.info.call_args_list[0][0][0]) self.assertEqual(parser.LOG.warn.call_count, 2) self.assertEqual( "The StackStorm configuration directory permissions are insecure " - "(too permissive)." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod 770 {config_dir}\n".format(config_dir=TEMP_CONFIG_DIR), + "(too permissive): others have access.", parser.LOG.warn.call_args_list[0][0][0]) self.assertEqual( - "The StackStorm configuration file permissions are insecure." - "\n\n" - "You can fix this by running:" - "\n\n" - " chmod 660 {config_file}\n".format(config_file=TEMP_FILE_PATH), + "The StackStorm configuration file permissions are insecure: others have access.", parser.LOG.warn.call_args_list[1][0][0]) # Make sure we left the file alone diff --git a/st2client/tests/unit/test_shell.py b/st2client/tests/unit/test_shell.py index b97e57b7b0..aca0a0acc2 100644 --- a/st2client/tests/unit/test_shell.py +++ b/st2client/tests/unit/test_shell.py @@ -493,12 +493,13 @@ def test_dont_warn_multiple_times(self): shell.LOG.warn.call_args_list[0][0][0][:63], 'The StackStorm configuration directory permissions are insecure') self.assertEqual( - shell.LOG.warn.call_args_list[1][0][0][:59], - 'The StackStorm configuration file permissions are insecure.') + shell.LOG.warn.call_args_list[1][0][0][:58], + 'The StackStorm configuration file permissions are insecure') self.assertEqual(shell.LOG.info.call_count, 2) self.assertEqual( - shell.LOG.info.call_args_list[0][0][0][:19], "The SGID bit is not") + shell.LOG.info.call_args_list[0][0][0], "The SGID bit is not " + "set on the StackStorm configuration directory.") self.assertEqual( shell.LOG.info.call_args_list[1][0][0], 'Skipping parsing CLI config') From d20def7f47203f4648aefdd2f01d16507891aeb9 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Tue, 26 Jun 2018 12:18:02 -0700 Subject: [PATCH 09/12] Add PR number to changelog --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b2e7bd0c08..7d711b1b1c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,7 +46,7 @@ Changed ~~~~~~~ * Update st2 CLI to create the configuration directory and file, and authentication tokens with - secure permissions (eg: readable only to owner) + secure permissions (eg: readable only to owner) #4173 * Refactor the callback module for the post run in runner to be more generic. (improvement) * Update various Python dependencies to the latest stable versions (gunicorn, gitpython, python-gnupg, tooz, flex). #4110 From 1ccd64973194371b86a4c8eb12e9e3d31db708b7 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Wed, 27 Jun 2018 11:41:23 -0700 Subject: [PATCH 10/12] Remove one last help text from log messages --- st2client/st2client/base.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index c5e47a0d8b..6ee204210a 100644 --- a/st2client/st2client/base.py +++ b/st2client/st2client/base.py @@ -264,12 +264,8 @@ def _get_cached_auth_token(self, client, username, password): # Every user has access to this file which is dangerous message = ('Permissions (%s) for cached token file "%s" are too permissive. Please ' 'restrict the permissions and make sure only your own user can read ' - 'from or write to the file.' - '\n\n' - 'You can fix this by running:' - '\n\n' - ' chmod o-rw %s') - self.LOG.warn(message % (file_st_mode, cached_token_path, cached_token_path)) + 'from or write to the file.' % (file_st_mode, cached_token_path)) + self.LOG.warn(message) with open(cached_token_path) as fp: data = fp.read() From 2680dcec83700e5ca4473745194b3d6dc2249836 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Wed, 27 Jun 2018 12:38:25 -0700 Subject: [PATCH 11/12] Tweak to not warn on weird permissions --- st2client/st2client/config_parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index dc4c6891a6..c627c68a9a 100644 --- a/st2client/st2client/config_parser.py +++ b/st2client/st2client/config_parser.py @@ -150,7 +150,7 @@ def parse(self): if self.validate_config_permissions: # Make sure the directory permissions == 0o770 - if bool(os.stat(config_dir_path).st_mode & 0o777 ^ 0o770): + if bool(os.stat(config_dir_path).st_mode & 0o7): self.LOG.warn( "The StackStorm configuration directory permissions are " "insecure (too permissive): others have access.") @@ -162,7 +162,7 @@ def parse(self): "directory.") # Make sure the file permissions == 0o660 - if bool(os.stat(self.config_file_path).st_mode & 0o667 ^ 0o660): + if bool(os.stat(self.config_file_path).st_mode & 0o7): self.LOG.warn( "The StackStorm configuration file permissions are " "insecure: others have access.") From 1c76e6b5c4c57b1d54b91a5a6b8e795f62da01d3 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Wed, 27 Jun 2018 12:38:57 -0700 Subject: [PATCH 12/12] Add tests for weird permissions and refactor tests --- st2client/tests/unit/test_config_parser.py | 318 +++++++++++---------- 1 file changed, 160 insertions(+), 158 deletions(-) diff --git a/st2client/tests/unit/test_config_parser.py b/st2client/tests/unit/test_config_parser.py index 7d155bb667..3d11c6b5f8 100644 --- a/st2client/tests/unit/test_config_parser.py +++ b/st2client/tests/unit/test_config_parser.py @@ -39,164 +39,6 @@ def test_constructor(self): self.assertRaises(ValueError, CLIConfigParser, config_file_path='doestnotexist', validate_config_exists=True) - def test_correct_permissions_emit_no_warnings(self): - TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') - TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) - - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) - - try: - # Setup the config directory - os.makedirs(TEMP_CONFIG_DIR) - os.chmod(TEMP_CONFIG_DIR, 0o2770) - - self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) - - # Setup the config file - shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) - os.chmod(TEMP_FILE_PATH, 0o660) - - self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o660) - - parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, validate_config_exists=True) - parser.LOG = mock.Mock() - - result = parser.parse() # noqa F841 - - self.assertEqual(parser.LOG.warn.call_count, 0) - - # Make sure we left the file alone - self.assertTrue(os.path.exists(TEMP_FILE_PATH)) - self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o660) - - self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) - self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) - finally: - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - def test_warn_on_bad_config_permissions(self): - TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') - TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) - - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) - - try: - # Setup the config directory - os.makedirs(TEMP_CONFIG_DIR) - os.chmod(TEMP_CONFIG_DIR, 0o0755) - - self.assertNotEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) - - # Setup the config file - shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) - os.chmod(TEMP_FILE_PATH, 0o664) - - self.assertNotEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o770) - - parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, validate_config_exists=True) - parser.LOG = mock.Mock() - - result = parser.parse() # noqa F841 - - self.assertEqual(parser.LOG.info.call_count, 1) - - self.assertEqual( - "The SGID bit is not set on the StackStorm configuration directory.", - parser.LOG.info.call_args_list[0][0][0]) - - self.assertEqual(parser.LOG.warn.call_count, 2) - self.assertEqual( - "The StackStorm configuration directory permissions are insecure " - "(too permissive): others have access.", - parser.LOG.warn.call_args_list[0][0][0]) - - self.assertEqual( - "The StackStorm configuration file permissions are insecure: others have access.", - parser.LOG.warn.call_args_list[1][0][0]) - - # Make sure we left the file alone - self.assertTrue(os.path.exists(TEMP_FILE_PATH)) - self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o664) - - self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) - self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755) - finally: - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - def test_disable_permissions_warnings(self): - TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') - TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) - - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) - - try: - # Setup the config directory - os.makedirs(TEMP_CONFIG_DIR) - os.chmod(TEMP_CONFIG_DIR, 0o0755) - - self.assertNotEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) - - # Setup the config file - shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) - os.chmod(TEMP_FILE_PATH, 0o664) - - self.assertNotEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o770) - - parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, - validate_config_exists=True, - validate_config_permissions=False) - parser.LOG = mock.Mock() - - result = parser.parse() # noqa F841 - - self.assertEqual(parser.LOG.info.call_count, 0) - self.assertEqual(parser.LOG.warn.call_count, 0) - - # Make sure we left the file alone - self.assertTrue(os.path.exists(TEMP_FILE_PATH)) - self.assertEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o664) - - self.assertTrue(os.path.exists(TEMP_CONFIG_DIR)) - self.assertEqual(os.stat(TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755) - finally: - if os.path.exists(TEMP_FILE_PATH): - os.remove(TEMP_FILE_PATH) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - - if os.path.exists(TEMP_CONFIG_DIR): - os.removedirs(TEMP_CONFIG_DIR) - self.assertFalse(os.path.exists(TEMP_FILE_PATH)) - def test_parse(self): # File doesn't exist parser = CLIConfigParser(config_file_path='doesnotexist', validate_config_exists=False) @@ -252,3 +94,163 @@ def test_get_config_for_unicode_char(self): self.assertEqual(config['credentials']['password'], '密码') else: self.assertEqual(config['credentials']['password'], u'\u5bc6\u7801') + + +class CLIConfigPermissionsTestCase(unittest2.TestCase): + def setUp(self): + self.TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') + self.TEMP_CONFIG_DIR = os.path.dirname(self.TEMP_FILE_PATH) + + if os.path.exists(self.TEMP_FILE_PATH): + os.remove(self.TEMP_FILE_PATH) + self.assertFalse(os.path.exists(self.TEMP_FILE_PATH)) + + if os.path.exists(self.TEMP_CONFIG_DIR): + os.removedirs(self.TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(self.TEMP_CONFIG_DIR)) + + # Setup the config directory + os.makedirs(self.TEMP_CONFIG_DIR) + + # Copy the config file + shutil.copyfile(CONFIG_FILE_PATH_FULL, self.TEMP_FILE_PATH) + + def tearDown(self): + if os.path.exists(self.TEMP_FILE_PATH): + os.remove(self.TEMP_FILE_PATH) + self.assertFalse(os.path.exists(self.TEMP_FILE_PATH)) + + if os.path.exists(self.TEMP_CONFIG_DIR): + os.removedirs(self.TEMP_CONFIG_DIR) + self.assertFalse(os.path.exists(self.TEMP_CONFIG_DIR)) + + def test_correct_permissions_emit_no_warnings(self): + os.chmod(self.TEMP_CONFIG_DIR, 0o2770) + + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + + # Setup the config file + os.chmod(self.TEMP_FILE_PATH, 0o660) + + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o660) + + parser = CLIConfigParser(config_file_path=self.TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(self.TEMP_FILE_PATH)) + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o660) + + self.assertTrue(os.path.exists(self.TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + + def test_weird_but_correct_permissions_emit_no_warnings(self): + os.chmod(self.TEMP_CONFIG_DIR, 0o2770) + + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + + # 1. Config file: 0o640 + os.chmod(self.TEMP_FILE_PATH, 0o640) + + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o640) + + parser = CLIConfigParser(config_file_path=self.TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(self.TEMP_FILE_PATH)) + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o640) + + # 2. Config file: 0o600 + os.chmod(self.TEMP_FILE_PATH, 0o600) + + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o600) + + parser = CLIConfigParser(config_file_path=self.TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(self.TEMP_FILE_PATH)) + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o600) + + self.assertTrue(os.path.exists(self.TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o2770) + + def test_warn_on_bad_config_permissions(self): + # Setup the config directory + os.chmod(self.TEMP_CONFIG_DIR, 0o0755) + + self.assertNotEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) + + # Setup the config file + os.chmod(self.TEMP_FILE_PATH, 0o664) + + self.assertNotEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o770) + + parser = CLIConfigParser(config_file_path=self.TEMP_FILE_PATH, validate_config_exists=True) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.info.call_count, 1) + + self.assertEqual( + "The SGID bit is not set on the StackStorm configuration directory.", + parser.LOG.info.call_args_list[0][0][0]) + + self.assertEqual(parser.LOG.warn.call_count, 2) + self.assertEqual( + "The StackStorm configuration directory permissions are insecure " + "(too permissive): others have access.", + parser.LOG.warn.call_args_list[0][0][0]) + + self.assertEqual( + "The StackStorm configuration file permissions are insecure: others have access.", + parser.LOG.warn.call_args_list[1][0][0]) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(self.TEMP_FILE_PATH)) + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o664) + + self.assertTrue(os.path.exists(self.TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755) + + def test_disable_permissions_warnings(self): + # Setup the config directory + os.chmod(self.TEMP_CONFIG_DIR, 0o0755) + + self.assertNotEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0770) + + # Setup the config file + os.chmod(self.TEMP_FILE_PATH, 0o664) + + self.assertNotEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o770) + + parser = CLIConfigParser(config_file_path=self.TEMP_FILE_PATH, + validate_config_exists=True, + validate_config_permissions=False) + parser.LOG = mock.Mock() + + result = parser.parse() # noqa F841 + + self.assertEqual(parser.LOG.info.call_count, 0) + self.assertEqual(parser.LOG.warn.call_count, 0) + + # Make sure we left the file alone + self.assertTrue(os.path.exists(self.TEMP_FILE_PATH)) + self.assertEqual(os.stat(self.TEMP_FILE_PATH).st_mode & 0o777, 0o664) + + self.assertTrue(os.path.exists(self.TEMP_CONFIG_DIR)) + self.assertEqual(os.stat(self.TEMP_CONFIG_DIR).st_mode & 0o7777, 0o0755)