Merge ~cjwatson/turnip:non-ascii-ref-permissions into turnip:master

[Colin Watson]

Use new bytes-capable checkRefPermissions protocol

Git ref paths may not be valid UTF-8, so we need to treat them as bytes.
We can't deal perfectly with non-UTF-8 refs - they won't get scanned and
so won't show up in the webservice API or the web UI - but we can at
least allow them to round-trip through Launchpad at the git level. This
migrates to a new version of the checkRefPermissions protocol which can
cope with ref paths being bytes rather than text.

LP: #1517559

https://code.launchpad.net/~cjwatson/launchpad/git-permissions-non-unicode-refs/+merge/359088 must be on production before landing this.

[Tom Wardill (twom)]

review: Approve

diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index 7d818bf..b024734 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """RPC server for Git hooks.
@@ -20,8 +20,10 @@ from __future__ import (
     unicode_literals,
     )
 
+import base64
 import json
 
+from six.moves import xmlrpc_client
 from twisted.internet import (
     defer,
     protocol,
@@ -124,22 +126,24 @@ class HookRPCHandler(object):
     def checkRefPermissions(self, proto, args):
         """Get permissions for a set of refs."""
         cached_permissions = self.ref_permissions[args['key']]
-        missing = [x for x in args['paths']
-                   if x not in cached_permissions]
+        paths = [
+            base64.b64decode(path.encode('UTF-8')) for path in args['paths']]
+        missing = [x for x in paths if x not in cached_permissions]
         if missing:
             proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
             result = yield proxy.callRemote(
                 b'checkRefPermissions',
                 self.ref_paths[args['key']],
-                missing,
+                [xmlrpc_client.Binary(path) for path in missing],
                 self.auth_params[args['key']]
             )
-            for ref, permission in result.items():
-                cached_permissions[ref] = permission
+            for ref, permission in result:
+                cached_permissions[ref.data] = permission
         # cached_permissions is a shallow copy of the key index for
         # self.ref_permissions, so changes will be updated in that.
         defer.returnValue(
-            {ref: cached_permissions[ref] for ref in args['paths']})
+            {base64.b64encode(ref).decode('UTF-8'): cached_permissions[ref]
+             for ref in paths})
 
     @defer.inlineCallbacks
     def notify(self, path):
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index 1594a4b..22251a2 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -1,6 +1,6 @@
 #!/usr/bin/python
 
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import (
@@ -9,6 +9,7 @@ from __future__ import (
     unicode_literals,
     )
 
+import base64
 import json
 import os
 import socket
@@ -41,13 +42,13 @@ def determine_permissions_outcome(old, ref, rule_lines):
         if 'create' in rule:
             return
         else:
-            return 'You do not have permission to create %s.' % ref
+            return b'You do not have permission to create ' + ref + b'.'
     # We have push permission, everything is okay
     # force_push is checked later (in update-hook)
     if 'push' in rule:
         return
     # If we're here, there are no matching rules
-    return "You do not have permission to push to %s." % ref
+    return b"You do not have permission to push to " + ref + b"."
 
 
 def match_rules(rule_lines, ref_lines):
@@ -89,7 +90,7 @@ def match_update_rules(rule_lines, ref_line):
     rule = rule_lines.get(ref, [])
     if 'force_push' in rule:
         return []
-    return ['You do not have permission to force-push to %s.' % ref]
+    return [b'You do not have permission to force-push to ' + ref + b'.']
 
 
 def netstring_send(sock, s):
@@ -120,6 +121,16 @@ def rpc_invoke(sock, method, args):
     return res['result']
 
 
+def check_ref_permissions(sock, rpc_key, ref_paths):
+    ref_paths = [base64.b64encode(path).decode('UTF-8') for path in ref_paths]
+    rule_lines = rpc_invoke(
+        sock, b'check_ref_permissions',
+        {'key': rpc_key, 'paths': ref_paths})
+    return {
+        base64.b64decode(path.encode('UTF-8')): permissions
+        for path, permissions in rule_lines.items()}
+
+
 if __name__ == '__main__':
     # Connect to the RPC server, authenticating using the random key
     # from the environment.
@@ -132,27 +143,23 @@ if __name__ == '__main__':
         # Verify the proposed changes against rules from the server.
         raw_paths = sys.stdin.readlines()
         ref_paths = [p.rstrip(b'\n').split(b' ', 2)[2] for p in raw_paths]
-        rule_lines = rpc_invoke(
-            sock, b'check_ref_permissions',
-            {'key': rpc_key, 'paths': ref_paths})
+        rule_lines = check_ref_permissions(sock, rpc_key, ref_paths)
         errors = match_rules(rule_lines, raw_paths)
         for error in errors:
-            sys.stdout.write(error + '\n')
+            sys.stdout.write(error + b'\n')
         sys.exit(1 if errors else 0)
     elif hook == 'post-receive':
         # Notify the server about the push if there were any changes.
         # Details of the changes aren't currently included.
         if sys.stdin.readlines():
-            rule_lines = rpc_invoke(sock, b'notify_push', {'key': rpc_key})
+            rpc_invoke(sock, b'notify_push', {'key': rpc_key})
         sys.exit(0)
     elif hook == 'update':
         ref = sys.argv[1]
-        rule_lines = rpc_invoke(
-            sock, b'check_ref_permissions',
-            {'key': rpc_key, 'paths': [ref]})
+        rule_lines = check_ref_permissions(sock, rpc_key, [ref])
         errors = match_update_rules(rule_lines, sys.argv[1:4])
         for error in errors:
-            sys.stdout.write(error + '\n')
+            sys.stdout.write(error + b'\n')
         sys.exit(1 if errors else 0)
     else:
         sys.stderr.write('Invalid hook name: %s' % hook)
diff --git a/turnip/pack/tests/fake_servers.py b/turnip/pack/tests/fake_servers.py
index 1e0f310..e5951f0 100644
--- a/turnip/pack/tests/fake_servers.py
+++ b/turnip/pack/tests/fake_servers.py
@@ -11,6 +11,7 @@ from collections import defaultdict
 import hashlib
 
 from lazr.sshserver.auth import NoSuchPersonWithName
+from six.moves import xmlrpc_client
 from twisted.web import xmlrpc
 
 __all__ = [
@@ -88,4 +89,6 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
 
     def xmlrpc_checkRefPermissions(self, path, ref_paths, auth_params):
         self.ref_permissions_checks.append((path, ref_paths, auth_params))
-        return self.ref_permissions
+        return [
+            (xmlrpc_client.Binary(ref), permissions)
+            for ref, permissions in self.ref_permissions.items()]
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index 2fa1782..7d80cdf 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -81,11 +81,13 @@ class FunctionalTestMixin(object):
         self.virtinfo_url = b'http://localhost:%d/' % self.virtinfo_port
         self.addCleanup(self.virtinfo_listener.stopListening)
         self.virtinfo.ref_permissions = {
-            'refs/heads/master': ['create', 'push']}
+            b'refs/heads/master': ['create', 'push']}
 
     def startHookRPC(self):
         self.hookrpc_handler = HookRPCHandler(self.virtinfo_url)
-        dir = tempfile.mkdtemp(prefix='turnip-test-hook-')
+        # XXX cjwatson 2018-11-20: Use bytes so that shutil.rmtree doesn't
+        # get confused on Python 2.
+        dir = tempfile.mkdtemp(prefix=b'turnip-test-hook-')
         self.addCleanup(shutil.rmtree, dir, ignore_errors=True)
 
         self.hookrpc_path = os.path.join(dir, 'hookrpc_sock')
@@ -94,7 +96,9 @@ class FunctionalTestMixin(object):
         self.addCleanup(self.hookrpc_listener.stopListening)
 
     def startPackBackend(self):
-        self.root = tempfile.mkdtemp(prefix='turnip-test-root-')
+        # XXX cjwatson 2018-11-20: Use bytes so that shutil.rmtree doesn't
+        # get confused on Python 2.
+        self.root = tempfile.mkdtemp(prefix=b'turnip-test-root-')
         self.addCleanup(shutil.rmtree, self.root, ignore_errors=True)
         self.backend_listener = reactor.listenTCP(
             0,
@@ -224,7 +228,7 @@ class FunctionalTestMixin(object):
     @defer.inlineCallbacks
     def test_no_permissions(self):
         # Update the test ref_permissions
-        self.virtinfo.ref_permissions = {'refs/heads/master': ['push']}
+        self.virtinfo.ref_permissions = {b'refs/heads/master': ['push']}
         # Test a push fails if the user has no permissions to that ref
         test_root = self.useFixture(TempDir()).path
         clone1 = os.path.join(test_root, 'clone1')
@@ -248,7 +252,7 @@ class FunctionalTestMixin(object):
             error)
 
         # add create, disable push
-        self.virtinfo.ref_permissions = {'refs/heads/master': ['create']}
+        self.virtinfo.ref_permissions = {b'refs/heads/master': ['create']}
         # Can now create the ref
         yield self.assertCommandSuccess(
             (b'git', b'push', b'origin', b'master'), path=clone1)
@@ -263,10 +267,44 @@ class FunctionalTestMixin(object):
             b"You do not have permission to push to refs/heads/master", error)
 
     @defer.inlineCallbacks
+    def test_push_non_ascii_refs(self):
+        # Pushing non-ASCII refs works.
+        self.virtinfo.ref_permissions = {
+            b'refs/heads/\x80': ['create', 'push'],
+            u'refs/heads/\N{SNOWMAN}'.encode('UTF-8'): ['create', 'push'],
+            }
+        test_root = self.useFixture(TempDir()).path
+        clone1 = os.path.join(test_root, 'clone1')
+        clone2 = os.path.join(test_root, 'clone2')
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.name', b'Test User'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.email', b'[e-mail]'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'--allow-empty', b'-m', b'Non-ASCII test'),
+            path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'push', b'origin', b'master:\x80',
+             u'master:\N{SNOWMAN}'.encode('UTF-8')), path=clone1)
+        # We get the new branches when we re-clone.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone2))
+        out = yield self.assertCommandSuccess(
+            (b'git', b'for-each-ref', b'--format=%(refname)',
+             b'refs/remotes/origin/*'),
+            path=clone2)
+        self.assertEqual(
+            sorted([
+                b'refs/remotes/origin/\x80',
+                u'refs/remotes/origin/\N{SNOWMAN}'.encode('UTF-8')]),
+            sorted(out.splitlines()))
+
+    @defer.inlineCallbacks
     def test_force_push(self):
         # Update the test ref_permissions
         self.virtinfo.ref_permissions = {
-            'refs/heads/master': ['create', 'push']}
+            b'refs/heads/master': ['create', 'push']}
 
         # Test a force-push fails if the user has no permissions
         test_root = self.useFixture(TempDir()).path
@@ -347,7 +385,7 @@ class FunctionalTestMixin(object):
     @defer.inlineCallbacks
     def test_delete_ref(self):
         self.virtinfo.ref_permissions = {
-            'refs/heads/newref': ['create', 'push', 'force_push']}
+            b'refs/heads/newref': ['create', 'push', 'force_push']}
         test_root = self.useFixture(TempDir()).path
         clone1 = os.path.join(test_root, 'clone1')
 
@@ -383,7 +421,7 @@ class FunctionalTestMixin(object):
     @defer.inlineCallbacks
     def test_delete_ref_without_permission(self):
         self.virtinfo.ref_permissions = {
-            'refs/heads/newref': ['create', 'push']}
+            b'refs/heads/newref': ['create', 'push']}
         test_root = self.useFixture(TempDir()).path
         clone1 = os.path.join(test_root, 'clone1')
 
@@ -471,7 +509,7 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
                 b'localhost', self.backend_port, self.virtinfo_url))
         self.virt_port = self.virt_listener.getHost().port
         self.virtinfo.ref_permissions = {
-            'refs/heads/master': ['create', 'push']}
+            b'refs/heads/master': ['create', 'push']}
 
     @defer.inlineCallbacks
     def tearDown(self):
@@ -482,7 +520,7 @@ class FrontendFunctionalTestMixin(FunctionalTestMixin):
     @defer.inlineCallbacks
     def test_read_only(self):
         self.virtinfo.ref_permissions = {
-            'refs/heads/master': ['create', 'push']}
+            b'refs/heads/master': ['create', 'push']}
         test_root = self.useFixture(TempDir()).path
         clone1 = os.path.join(test_root, 'clone1')
         clone2 = os.path.join(test_root, 'clone2')
diff --git a/turnip/pack/tests/test_hookrpc.py b/turnip/pack/tests/test_hookrpc.py
index 9af0408..5109ecd 100644
--- a/turnip/pack/tests/test_hookrpc.py
+++ b/turnip/pack/tests/test_hookrpc.py
@@ -7,11 +7,20 @@ from __future__ import (
     unicode_literals,
     )
 
+import base64
 import contextlib
 import uuid
 
+from six.moves import xmlrpc_client
 from testtools import TestCase
 from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.matchers import (
+    Equals,
+    IsInstance,
+    MatchesAll,
+    MatchesListwise,
+    MatchesStructure,
+    )
 from twisted.internet import (
     defer,
     reactor,
@@ -164,22 +173,39 @@ class TestHookRPCHandler(TestCase):
         finally:
             self.hookrpc_handler.unregisterKey(key)
 
+    def encodeRefPath(self, ref_path):
+        return base64.b64encode(ref_path).decode('UTF-8')
+
     def assertCheckedRefPermissions(self, path, ref_paths, auth_params):
-        self.assertEqual(
-            [(path, ref_paths, auth_params)],
-            self.virtinfo.ref_permissions_checks)
+        self.assertThat(self.virtinfo.ref_permissions_checks, MatchesListwise([
+            MatchesListwise([
+                Equals(path),
+                MatchesListwise([
+                    MatchesAll(
+                        IsInstance(xmlrpc_client.Binary),
+                        MatchesStructure.byEquality(data=ref_path))
+                    for ref_path in ref_paths
+                    ]),
+                Equals(auth_params),
+                ]),
+            ]))
 
     @defer.inlineCallbacks
     def test_checkRefPermissions_fresh(self):
         self.virtinfo.ref_permissions = {
-            'refs/heads/master': ['push'],
-            'refs/heads/next': ['force_push'],
+            b'refs/heads/master': ['push'],
+            b'refs/heads/next': ['force_push'],
             }
+        encoded_paths = [
+            self.encodeRefPath(ref_path)
+            for ref_path in sorted(self.virtinfo.ref_permissions)]
         with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
             permissions = yield self.hookrpc_handler.checkRefPermissions(
-                None,
-                {'key': key, 'paths': sorted(self.virtinfo.ref_permissions)})
-        self.assertEqual(self.virtinfo.ref_permissions, permissions)
+                None, {'key': key, 'paths': encoded_paths})
+        expected_permissions = {
+            self.encodeRefPath(ref_path): perms
+            for ref_path, perms in self.virtinfo.ref_permissions.items()}
+        self.assertEqual(expected_permissions, permissions)
         self.assertCheckedRefPermissions(
             '/translated', [b'refs/heads/master', b'refs/heads/next'],
             {'uid': 42})
@@ -187,15 +213,16 @@ class TestHookRPCHandler(TestCase):
     @defer.inlineCallbacks
     def test_checkRefPermissions_cached(self):
         cached_ref_permissions = {
-            'refs/heads/master': ['push'],
-            'refs/heads/next': ['force_push'],
+            b'refs/heads/master': ['push'],
+            b'refs/heads/next': ['force_push'],
             }
+        encoded_master = self.encodeRefPath(b'refs/heads/master')
         with self.registeredKey(
                 '/translated', auth_params={'uid': 42},
                 permissions=cached_ref_permissions) as key:
             permissions = yield self.hookrpc_handler.checkRefPermissions(
-                None, {'key': key, 'paths': ['refs/heads/master']})
-        expected_permissions = {'refs/heads/master': ['push']}
+                None, {'key': key, 'paths': [encoded_master]})
+        expected_permissions = {encoded_master: ['push']}
         self.assertEqual(expected_permissions, permissions)
         self.assertEqual([], self.virtinfo.ref_permissions_checks)
 
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index ebf9c80..7aaf356 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -1,4 +1,5 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# -*- coding: utf-8 -*-
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import (
@@ -7,6 +8,7 @@ from __future__ import (
     unicode_literals,
     )
 
+import base64
 import os.path
 import uuid
 
@@ -61,7 +63,9 @@ class MockHookRPCHandler(hookrpc.HookRPCHandler):
         self.notifications.append(self.ref_paths[args['key']])
 
     def checkRefPermissions(self, proto, args):
-        return self.ref_permissions[args['key']]
+        return {
+            base64.b64encode(ref).decode('UTF-8'): permissions
+            for ref, permissions in self.ref_permissions[args['key']].items()}
 
 
 class MockRef(object):
@@ -147,7 +151,15 @@ class TestPreReceiveHook(HookTestMixin, TestCase):
         # A single valid ref is accepted.
         yield self.assertAccepted(
             [(b'refs/heads/master', self.old_sha1, self.new_sha1)],
-            {'refs/heads/master': ['push']})
+            {b'refs/heads/master': ['push']})
+
+    @defer.inlineCallbacks
+    def test_accepted_non_ascii(self):
+        # Valid non-ASCII refs are accepted.
+        paths = [b'refs/heads/\x80', u'refs/heads/géag'.encode('UTF-8')]
+        yield self.assertAccepted(
+            [(path, self.old_sha1, self.new_sha1) for path in paths],
+            {path: ['push'] for path in paths})
 
     @defer.inlineCallbacks
     def test_rejected(self):
@@ -171,6 +183,16 @@ class TestPreReceiveHook(HookTestMixin, TestCase):
             b"You do not have permission to push "
             b"to refs/heads/super-verboten.\n")
 
+    @defer.inlineCallbacks
+    def test_rejected_non_ascii(self):
+        # Invalid non-ASCII refs are rejected.
+        paths = [b'refs/heads/\x80', u'refs/heads/géag'.encode('UTF-8')]
+        yield self.assertRejected(
+            [(path, self.old_sha1, self.new_sha1) for path in paths],
+            {path: [] for path in paths},
+            b"You do not have permission to push to refs/heads/\x80.\n"
+            b"You do not have permission to push to refs/heads/g\xc3\xa9ag.\n")
+
 
 class TestPostReceiveHook(HookTestMixin, TestCase):
     """Tests for the git post-receive hook."""
@@ -212,12 +234,12 @@ class TestUpdateHook(TestCase):
         # Creation is determined by an all 0 base sha
         self.assertEqual(
             [], hook.match_update_rules(
-                [], ['ref', pygit2.GIT_OID_HEX_ZERO, 'new']))
+                {}, [b'ref', pygit2.GIT_OID_HEX_ZERO, 'new']))
 
     def test_fast_forward(self):
         # If the old sha is a merge ancestor of the new
         self.assertEqual(
-            [], hook.match_update_rules([], ['ref', 'somehex', 'new']))
+            [], hook.match_update_rules({}, ['ref', 'somehex', 'new']))
 
     def test_rules_fall_through(self):
         # The default is to deny
@@ -229,10 +251,27 @@ class TestUpdateHook(TestCase):
         # No matches means deny by default
         output = hook.match_update_rules(
             {'notamatch': []},
-            ['ref', 'old', 'new'])
+            [b'ref', 'old', 'new'])
         self.assertEqual(
             [b'You do not have permission to force-push to ref.'], output)
 
+    def test_no_matching_non_utf8_ref(self):
+        # An unmatched non-UTF-8 ref is denied.
+        output = hook.match_update_rules(
+            {}, [b'refs/heads/\x80', 'old', 'new'])
+        self.assertEqual(
+            [b'You do not have permission to force-push to refs/heads/\x80.'],
+            output)
+
+    def test_no_matching_utf8_ref(self):
+        # An unmatched UTF-8 ref is denied.
+        output = hook.match_update_rules(
+            {}, [u'refs/heads/géag'.encode('UTF-8'), 'old', 'new'])
+        self.assertEqual(
+            [b'You do not have permission to force-push to '
+             b'refs/heads/g\xc3\xa9ag.'],
+            output)
+
     def test_matching_ref(self):
         # Permission given to force-push
         output = hook.match_update_rules(
@@ -240,6 +279,20 @@ class TestUpdateHook(TestCase):
             ['ref', 'old', 'new'])
         self.assertEqual([], output)
 
+    def test_matching_non_utf8_ref(self):
+        # A non-UTF-8 ref with force-push permission is accepted.
+        output = hook.match_update_rules(
+            {b'refs/heads/\x80': ['force_push']},
+            [b'refs/heads/\x80', 'old', 'new'])
+        self.assertEqual([], output)
+
+    def test_matching_utf8_ref(self):
+        # A UTF-8 ref with force-push permission is accepted.
+        output = hook.match_update_rules(
+            {u'refs/heads/géag'.encode('UTF-8'): ['force_push']},
+            [u'refs/heads/géag'.encode('UTF-8'), 'old', 'new'])
+        self.assertEqual([], output)
+
     def test_no_permission(self):
         # User does not have permission to force-push
         output = hook.match_update_rules(