Merge lp:~jelmer/brz/authconfperm into lp:brz

[Jelmer Vernooij]

Use tight permissions on authentication.conf.

When creating authentication.conf, make sure only the user can read it.

When opening an existing authentication.conf, warn if others can read the file - allow this warning to be suppressed by setting ``suppress_warnings=insecure_permissions``.

[Martin Packman (gz)]

General concept seems fine, though we still don't really want users putting passwords in there. Have some nits about the implementation inline.

Yet another change that makes me want to get to saner trace/warnings soon.

review: Approve

[The Breezy Bot (the-breezy-bot)]

Running landing tests failed
http://10.242.247.184:8080/job/brz-dev/129/

=== modified file 'breezy/config.py'
--- breezy/config.py	2017-06-11 01:19:16 +0000
+++ breezy/config.py	2017-06-13 00:26:53 +0000
@@ -83,8 +83,10 @@
 from .lazy_import import lazy_import
 lazy_import(globals(), """
 import base64
+import errno
 import fnmatch
 import re
+import stat
 
 from breezy import (
     atomicfile,
@@ -1655,6 +1657,7 @@
         if _file is None:
             self._filename = authentication_config_filename()
             self._input = self._filename = authentication_config_filename()
+            self._check_permissions()
         else:
             # Tests can provide a string as _file
             self._filename = None
@@ -1677,12 +1680,29 @@
             raise errors.ConfigContentError(self._filename)
         return self._config
 
+    def _check_permissions(self):
+        """Check permission of auth file are user read/write able only."""
+        try:
+            st = os.stat(self._filename)
+        except OSError as e:
+            if e.errno != errno.ENOENT:
+                trace.mutter('Unable to stat %r: %r', self._filename, e)
+            return
+        mode = stat.S_IMODE(st.st_mode)
+        if ((stat.S_IXOTH | stat.S_IWOTH | stat.S_IROTH | stat.S_IXGRP |
+             stat.S_IWGRP | stat.S_IRGRP ) & mode):
+            if not GlobalConfig().suppress_warning('insecure_permissions'):
+                trace.warning("The file '%s' has insecure "
+                        "file permissions. Saved passwords may be accessible "
+                        "by other users.", self._filename)
+
     def _save(self):
         """Save the config file, only tests should use it for now."""
         conf_dir = os.path.dirname(self._filename)
         ensure_config_dir_exists(conf_dir)
-        f = file(self._filename, 'wb')
+        fd = os.open(self._filename, os.O_RDWR|os.O_CREAT, 0o600)
         try:
+            f = os.fdopen(fd, 'wb')
             self._get_config().write(f)
         finally:
             f.close()

=== modified file 'breezy/help_topics/en/configuration.txt'
--- breezy/help_topics/en/configuration.txt	2017-05-21 18:10:28 +0000
+++ breezy/help_topics/en/configuration.txt	2017-06-13 00:26:53 +0000
@@ -600,6 +600,10 @@
     whether the format deprecation warning is shown on repositories that are
     using deprecated formats.
 
+* ``insecure_permissions``:
+    whether a warning is shown if ``authentication.conf`` can be read
+    by other users.
+
 default_format
 ~~~~~~~~~~~~~~
 

=== modified file 'breezy/tests/test_config.py'
--- breezy/tests/test_config.py	2017-06-11 14:07:05 +0000
+++ breezy/tests/test_config.py	2017-06-13 00:26:53 +0000
@@ -956,7 +956,7 @@
         self.assertEqual(True, suppress_warning('b'))
 
 
-class TestGetConfig(tests.TestCase):
+class TestGetConfig(tests.TestCaseInTempDir):
 
     def test_constructs(self):
         config.GlobalConfig()
@@ -4114,6 +4114,38 @@
         self.assertIs(g1.store, g2.store)
 
 
+class TestAuthenticationConfigFilePermissions(tests.TestCaseInTempDir):
+    """Test warning for permissions of authentication.conf."""
+
+    def setUp(self):
+        super(TestAuthenticationConfigFilePermissions, self).setUp()
+        self.path = osutils.pathjoin(self.test_dir, 'authentication.conf')
+        with open(self.path, 'w') as f:
+            f.write(b"""[broken]
+scheme=ftp
+user=joe
+port=port # Error: Not an int
+""")
+        self.overrideAttr(config, 'authentication_config_filename',
+            lambda: self.path)
+        osutils.chmod_if_possible(self.path, 0o755)
+
+    def test_check_warning(self):
+        conf = config.AuthenticationConfig()
+        self.assertEqual(conf._filename, self.path)
+        self.assertContainsRe(self.get_log(),
+            'Saved passwords may be accessible by other users.')
+
+    def test_check_suppressed_warning(self):
+        global_config = config.GlobalConfig()
+        global_config.set_user_option('suppress_warnings',
+            'insecure_permissions')
+        conf = config.AuthenticationConfig()
+        self.assertEqual(conf._filename, self.path)
+        self.assertNotContainsRe(self.get_log(),
+            'Saved passwords may be accessible by other users.')
+
+
 class TestAuthenticationConfigFile(tests.TestCase):
     """Test the authentication.conf file matching"""
 
@@ -4350,7 +4382,7 @@
         self.assertEqual(CREDENTIALS, credentials)
 
 
-class TestAuthenticationConfig(tests.TestCase):
+class TestAuthenticationConfig(tests.TestCaseInTempDir):
     """Test AuthenticationConfig behaviour"""
 
     def _check_default_password_prompt(self, expected_prompt_format, scheme,

=== modified file 'doc/en/release-notes/brz-3.0.txt'
--- doc/en/release-notes/brz-3.0.txt	2017-06-11 13:36:02 +0000
+++ doc/en/release-notes/brz-3.0.txt	2017-06-13 00:26:53 +0000
@@ -84,6 +84,11 @@
 * The ``bisect`` command now works in non-``.bzr`` directories.
   (Jelmer Vernooij)
 
+* When creating ``authentication.conf``, umask is now set so only the
+  current user can read the file. Breezy warns if the file is
+  accessible for other users when it starts.
+  (Joke de Buhr, Jelmer Vernooij, #475501)
+
 Documentation
 *************