From 92e8f5ad7dc9ea04292333cc29ebec578270fa04 Mon Sep 17 00:00:00 2001 From: Drew Hubl Date: Mon, 11 Jun 2018 15:00:57 -0700 Subject: [PATCH] Fix config permissions on the config directory and the config file --- st2client/st2client/base.py | 9 ++--- st2client/st2client/commands/auth.py | 1 + st2client/st2client/config_parser.py | 15 ++++++++ st2client/tests/unit/test_auth.py | 12 ++++--- st2client/tests/unit/test_config_parser.py | 40 ++++++++++++++++++++++ 5 files changed, 69 insertions(+), 8 deletions(-) diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index 5ed24db4772..0b77e816c02 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 a9ed7541cee..1b865f65793 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -140,6 +140,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..71ef51f7a25 100644 --- a/st2client/st2client/config_parser.py +++ b/st2client/st2client/config_parser.py @@ -20,6 +20,7 @@ from __future__ import absolute_import import os +import warnings from collections import defaultdict @@ -138,6 +139,20 @@ 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 bool(os.stat(config_dir_path).st_mode ^ 0o770): + warnings.warn('Setting StackStorm config directory permissions ' + '(%s) to 0770' % config_dir_path) + os.chmod(config_dir_path, 0o770) + if not bool(os.stat(config_dir_path).st_mode ^ 0o770): + raise ValueError('Unable to set config directory permssions') + + if bool(os.stat(self.config_file_path).st_mode ^ 0o660): + warnings.warn('Setting StackStorm config file permissions (%s) ' + 'to 0660' % self.config_file_path) + os.chmod(self.config_file_path, 0o660) + 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..59f741a78fc 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -61,6 +61,7 @@ class TestLoginBase(base.BaseCLITestCase): DOTST2_PATH = os.path.expanduser('~/.st2/') CONFIG_FILE = tempfile.mkstemp(suffix='st2.conf') + CONFIG_DIR = tempfile.mkdtemp() CONFIG_CONTENTS = """ [credentials] username = olduser @@ -102,7 +103,7 @@ def tearDown(self): class TestLoginPasswordAndConfig(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE = os.path.join(TestLoginBase.CONFIG_DIR, 'logintest.cfg') TOKEN = { 'user': 'st2admin', @@ -141,7 +142,7 @@ def runTest(self): class TestLoginIntPwdAndConfig(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE = os.path.join(TestLoginBase.CONFIG_DIR, 'logintest.cfg') TOKEN = { 'user': 'st2admin', @@ -175,6 +176,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 +207,7 @@ def runTest(self, mock_gp): class TestLoginWritePwdOkay(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE = os.path.join(TestLoginBase.CONFIG_DIR, 'logintest.cfg') TOKEN = { 'user': 'st2admin', @@ -244,7 +248,7 @@ def runTest(self, mock_gp): class TestLoginUncaughtException(TestLoginBase): - CONFIG_FILE = '/tmp/logintest.cfg' + CONFIG_FILE = os.path.join(TestLoginBase.CONFIG_DIR, '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..b1a0ddc1e77 100644 --- a/st2client/tests/unit/test_config_parser.py +++ b/st2client/tests/unit/test_config_parser.py @@ -16,6 +16,8 @@ from __future__ import absolute_import import os +import shutil +import warnings import six import unittest2 @@ -37,6 +39,44 @@ def test_constructor(self): self.assertRaises(ValueError, CLIConfigParser, config_file_path='doestnotexist', validate_config_exists=True) + def test_fix_permssions_on_existing_config(self): + TEMP_FILE_PATH = os.path.join('st2config', '.st2', 'config') + TEMP_CONFIG_DIR = os.path.dirname(TEMP_FILE_PATH) + + self.assertFalse(os.path.exists(TEMP_CONFIG_DIR)) + + try: + os.makedirs(TEMP_CONFIG_DIR) + + shutil.copyfile(CONFIG_FILE_PATH_FULL, TEMP_FILE_PATH) + + self.assertNotEqual(os.stat(TEMP_FILE_PATH).st_mode & 0o777, 0o770) + + parser = CLIConfigParser(config_file_path=TEMP_FILE_PATH, validate_config_exists=True) + + with warnings.catch_warnings(record=True) as warnings_list: + result = parser.parse() + + self.assertEquals( + "Setting StackStorm config directory permissions ({}) to 0770".format(TEMP_CONFIG_DIR), + str(warnings_list[0].message)) + + self.assertEqual( + "Setting StackStorm config file permissions ({}) to 0660".format(TEMP_FILE_PATH), + str(warnings_list[1].message)) + + 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 & 0o777, 0o770) + finally: + if os.path.exists(TEMP_FILE_PATH): + os.remove(TEMP_FILE_PATH) + 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)