diff --git a/CHANGELOG.rst b/CHANGELOG.rst index faec64b0a2..7d711b1b1c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -45,6 +45,8 @@ Added Changed ~~~~~~~ +* Update st2 CLI to create the configuration directory and file, and authentication tokens with + 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 diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index 5ed24db477..6ee204210a 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 @@ -225,7 +229,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,11 +260,11 @@ 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 ' + 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)) + 'from or write to the file.' % (file_st_mode, cached_token_path)) self.LOG.warn(message) with open(cached_token_path) as fp: @@ -290,7 +297,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 +336,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 @@ -349,7 +360,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/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 diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index a4a1aa1577..c627c68a9a 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,11 +121,18 @@ class CLIConfigParser(object): - 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): """ @@ -138,6 +146,27 @@ 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) + + if self.validate_config_permissions: + # Make sure the directory permissions == 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.") + + # Make sure the setgid bit is set on the directory + if not bool(os.stat(config_dir_path).st_mode & 0o2000): + self.LOG.info( + "The SGID bit is not set on the StackStorm configuration " + "directory.") + + # Make sure the file permissions == 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.") + config = ConfigParser() with io.open(self.config_file_path, 'r', encoding='utf8') as fp: config.readfp(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_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..3d11c6b5f8 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 @@ -92,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) diff --git a/st2client/tests/unit/test_shell.py b/st2client/tests/unit/test_shell.py index f6788ded3f..aca0a0acc2 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,12 +463,57 @@ 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.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][: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], "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') + 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 +555,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 +571,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, @@ -549,7 +596,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): @@ -570,7 +617,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 +631,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)