Merge lp:~mvo/aptdaemon/use-apt-auth.conf into lp:aptdaemon

[Michael Vogt]

This branch moves all username/password data from a repository into /etc/apt/auth.conf

[lp:~mvo/aptdaemon/use-apt-auth.conf]

779. By Michael Vogt on 2012-03-06

add a comment when credentials are moved

[Martin Pitt (pitti)]

nitpick:

> comment += " ;credentials put into /etc/apt/auth.conf"

This should be "; credentials ..."

> + res = urlsplit(uri)

Do we need to be concerned about exceptions here? or does the code already ensure that this is a valid URL? Is there any test case which runs that with an invalid URL, and one with a passwordless one?

Does netrc need any quoting if the password contains a space? I suppose that the URL is already encoded (i. e. %20), just making sure that this is handled.

58 + try:
59 + uid = gid = 0
60 + os.chown(auth_conf_path, uid, gid)
61 + os.chmod(auth_conf_path, 0640)

I think that's fine when creating the file. But I wouldn't change the permissions if it already exists; if the admin configures it to be writable for some sub-admin group we should respect that IMHO.

Seems ok to me otherwise.

review: Needs Fixing

[Michael Vogt (mvo)]

Thanks for your review!

I fixed the issues you mentioned in r780.

review: Resubmit

[lp:~mvo/aptdaemon/use-apt-auth.conf]

780. By Michael Vogt on 2012-03-08

fix review comments from Martin Pitt: a) typo fix b) do not chmod/chown but instead set umask c) check for exceptions in splituri()

=== modified file 'aptdaemon/worker.py'
--- aptdaemon/worker.py	2012-03-08 04:58:28 +0000
+++ aptdaemon/worker.py	2012-03-08 19:50:25 +0000
@@ -445,13 +445,14 @@
             sourcesfile = os.path.join(dir, os.path.basename(sourcesfile))
         else:
             sourcesfile = None
-        # if there is a password in the uri, protect the file from
-        # non-admin users
+        # if there is a password in the uri move it into the auth.conf
         password_in_uri = re.match("(http|https|ftp)://\S+?:\S+?@\S+", uri)
         if password_in_uri:
-            old_umask = os.umask(0027)
-        else:
-            old_umask = os.umask(0022)
+            uri = self._update_auth_conf_and_get_passwordless_uri(uri)
+            if not comment:
+                comment = "credentials put into /etc/apt/auth.conf"
+            else:
+                comment += " ; credentials put into /etc/apt/auth.conf"
         try:
             sources = SourcesList()
             entry = sources.add(src_type, uri, dist, comps, comment,
@@ -464,15 +465,38 @@
             raise
         else:
             sources.save()
-            # set to sourcesfile root.admin only if there is a password
-            if password_in_uri and sourcesfile:
-                import grp
-                try:
-                    os.chown(sourcesfile, 0, grp.getgrnam("admin")[2])
-                except Exception as e:
-                    logging.warn("os.chmod() failed '%s'" % e)
+
+    def _update_auth_conf_and_get_passwordless_uri(self, uri):
+        """Add the credentials in uri to /etc/apt/auth.conf 
+           and return the uri passwordless
+        """
+        from urlparse import urlsplit, urlunsplit
+        try:
+            res = urlsplit(uri)
+        except ValueError as e:
+            logging.warn("failed to urlsplit '%s'" % uri)
+            return uri
+        # split into bits and clean netloc
+        scheme = res.scheme
+        netloc = res.netloc.replace("%s:%s@" % (res.username, res.password), "")
+        path = res.path
+        query = res.query
+        fragment = res.fragment
+        # write auth.conf with tight default permissions
+        old_umask = os.umask(0027)
+        try:
+            auth_conf_path = apt.apt_pkg.config.FindFile("Dir::etc::netrc")
+            auth_conf = open(auth_conf_path, "a")
+            auth_conf.write("""machine %s
+login %s
+password %s\n\n""" % (netloc+path, res.username, res.password))
+            auth_conf.close()
+        except OSError as e:
+            logging.warn("writing auth.conf: '%s'" % e)
         finally:
             os.umask(old_umask)
+        # return uri without user/pass
+        return urlunsplit((scheme, netloc, path, query, fragment))
 
     def add_vendor_key_from_keyserver(self, trans, keyid, keyserver):
         """Add the signing key from the given (keyid, keyserver) to the

=== modified file 'tests/test_worker.py'
--- tests/test_worker.py	2012-01-23 10:32:24 +0000
+++ tests/test_worker.py	2012-03-08 19:50:25 +0000
@@ -25,6 +25,7 @@
 import glob
 import os
 import shutil
+import stat
 import sys
 import unittest
 
@@ -409,6 +410,30 @@
         self.assertEqual(license_key, open(verify_path).read(),
                          "Content of license key doesn't match")
 
+    def test_use_apt_auth_conf(self):
+        from mock import Mock
+        source_file_name = "private_source.list"
+        self.worker.add_repository(
+            Mock(), "deb", "https://user:[e-mail]/path", 
+            "natty", ["main"], "comment", source_file_name)
+        # check source file
+        source_file = os.path.join(
+            self.chroot.path, "etc", "apt", "sources.list.d", source_file_name)
+        sources_file_content = open(source_file).read()
+        #print sources_file_content
+        self.assertFalse("user:pass" in open(source_file).read())
+        # check auth file
+        auth_file = os.path.join(
+            self.chroot.path, "etc", "apt", "auth.conf")
+        auth_content =  open(auth_file).read()
+        #print auth_content
+        self.assertTrue("login user" in auth_content)
+        self.assertTrue("password pass" in auth_content)
+        self.assertTrue("machine host.example.com/path" in auth_content)
+        buf=os.stat(auth_file)
+        self.assertEqual(stat.S_IMODE(buf.st_mode), 0640)
+        
+
 
 if __name__ == "__main__":
     unittest.main()