diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index 5ed24db477..0b77e816c0 100644 --- a/st2client/st2client/base.py +++ b/st2client/st2client/base.py @@ -225,7 +225,7 @@ 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=0o770) cached_token_path = self._get_cached_token_path_for_user(username=username) @@ -253,7 +253,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 +290,7 @@ 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=0o770) username = token_obj.user cached_token_path = self._get_cached_token_path_for_user(username=username) @@ -326,9 +326,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..9440eb4c3b 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 a4a1aa1577..bb8592c84c 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 + if not bool(os.stat(config_dir_path).st_mode & 0o2000): + self.LOG.warn( + # TODO: Perfect place for an f-string + "The StackStorm configuration directory permissions are " + "insecure (SGID bit not set)." + "\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..b96286e08e 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -60,7 +60,10 @@ class TestLoginBase(base.BaseCLITestCase): """ DOTST2_PATH = os.path.expanduser('~/.st2/') - CONFIG_FILE = tempfile.mkstemp(suffix='st2.conf') + CONFIG_FILE_NAME = 'st2.conf' + # CONFIG_FILE = tempfile.mkstemp(suffix='st2.conf') + PARENT_DIR = 'testconfig' + TMP_DIR = tempfile.mkdtemp() CONFIG_CONTENTS = """ [credentials] username = olduser @@ -78,9 +81,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 +99,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 +111,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 +156,7 @@ def runTest(self): class TestLoginIntPwdAndConfig(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', @@ -175,6 +190,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 +221,7 @@ def runTest(self, mock_gp): class TestLoginWritePwdOkay(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE_NAME = 'logintest.cfg' TOKEN = { 'user': 'st2admin', @@ -244,7 +262,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..566cd8d56b 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,125 @@ 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.warn.call_count, 3) + 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 directory permissions are insecure " + "(SGID bit not set)." + "\n\n" + "You can fix this by running:" + "\n\n" + "chmod g+s {config_dir}".format(config_dir=TEMP_CONFIG_DIR), + parser.LOG.warn.call_args_list[1][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[2][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)