Fix config permissions on the config directory and the config file

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

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))

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
 

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)

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()

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