Warn about insecure config permissions on the config directory and th…

…e config file

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

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
 

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)

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',

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)