Merge lp:~joke/bzr/file_permissions_authentication.conf into lp:bzr

[John A Meinel]

Bug #475501, create authentication.conf with mode 0600

We currently don't do much fancy with authentication.conf permissions. Joke de Buhr reasonably pointed out that we could create the file as 600 and avoid defaulting to having your passwords accessible by other users on the system.

I haven't looked over the patch at all, just trying to get it out of the In Progress queue and get a decision on it.

[Jelmer Vernooij (jelmer)]

I wonder how this will do on Windows? It seems to rely on Unix modes, and it would be nice to add some tests.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 5/3/2011 1:45 AM, Jelmer Vernooij wrote:
> I wonder how this will do on Windows? It seems to rely on Unix modes, and it would be nice to add some tests.

The attributes are all available on Windows. Probably the big concern is
that it is a bit hard to set the modes reliably, and then the warning
will occur repeatedly.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2/8IgACgkQJdeBCYSNAAONQwCfYs+M6SRAQ23cjkMe51+7EMTD
wRsAn0rJ1SE+hDQMK7XlKWmGkfc6MhSk
=vrH1
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

The approach I've seen used in other contexts for such sensible informations seems to be:
- create the file with user-only persmissions,
- refuse to use it if others (even group) can read it

That's not exactly the approach taken here.

Now, I think windows doesn't provide this (user-only access) easily so we may just don't check there.

So what do others think ? Should we:

- create with 0600,
- warn if group or other have read access (not on windows)

or

- create with 0600
- refuse to use the file if group or other have read access (not on windows)

Something else ?

I'm fine with finishing this proposal if we reach a consensus, in the mean time, I'll mark this as wip.

review: Needs Information

[Vincent Ladeuil (vila)]

I forgot:

27 + if not GlobalConfig().get_user_option_as_bool(
28 + 'no_insecure_permissions_warning') :

We have a suppress_warnings config option that should be used here instead of introducing a new one (it's a list of warning names that should not be displayed, see bzrlib.config.Config.suppress_warning.

Unmerged revisions

4791. By Joke de Buhr on 2009-11-06

Don't emit warning message if the option 'no_insecure_permissions_warning'
is set to 'True' in bazaar.conf [DEFAULT]

4790. By Joke de Buhr on 2009-11-05

Emit warning message if an authentication.conf file hase insecure file permissions.

4789. By Joke de Buhr on 2009-11-05

create authentication.conf with umask 166 (permissions: u=rw,go=)

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-04-09 20:01:11 +0000
+++ bzrlib/config.py	2011-04-21 13:18:39 +0000
@@ -65,6 +65,7 @@
 import os
 import string
 import sys
+import stat
 
 from bzrlib import commands
 from bzrlib.decorators import needs_write_lock
@@ -1591,6 +1592,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
@@ -1611,15 +1613,32 @@
             raise errors.ParseConfigError(e.errors, e.config.filename)
         return self._config
 
+    def _check_permissions(self):
+        """Check permission of auth file are user read/write able only."""
+        mode = stat.S_IMODE(os.stat(self._filename).st_mode)
+        if not GlobalConfig().get_user_option_as_bool(
+                'no_insecure_permissions_warning') :
+            if ( ( stat.S_IXOTH | stat.S_IWOTH | stat.S_IROTH | stat.S_IXGRP |
+                    stat.S_IWGRP | stat.S_IRGRP ) & mode ) > 0 :
+                trace.warning("The file '" + self._filename + "' has insecure "
+                        "file permissions. Saved passwords may be accessible "
+                        "by other users.")
+
     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)
+<<<<<<< TREE
         f = file(self._filename, 'wb')
         try:
             self._get_config().write(f)
         finally:
             f.close()
+=======
+        old_umask = os.umask(0177)
+        self._get_config().write(file(self._filename, 'wb'))
+        os.umask(old_umask)
+>>>>>>> MERGE-SOURCE
 
     def _set_option(self, section_name, option_name, value):
         """Set an authentication configuration option"""