diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index 5ed24db4772..c1aedc1d899 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 a9ed7541cee..9440eb4c3bb 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, mode=0o660) return manager diff --git a/st2client/st2client/config_parser.py b/st2client/st2client/config_parser.py index a4a1aa15770..b0df04232fc 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 1a2b667ed78..d2e49c100f1 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 6b77f3c14a7..bb1c575ac6b 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 f6788ded3fe..2b9d48d6e23 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)