launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-fix-macaroon-auth into lp:launchpad.

Commit message:
Bind the SSO discharge macaroon before sending it to the store.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1572605 in Launchpad itself: "Automatically upload snap builds to store"
  https://bugs.launchpad.net/launchpad/+bug/1572605

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-fix-macaroon-auth/+merge/295563

I wrote most of SnapStoreClient before we switched to the new handling of discharge macaroons which requires the client to bind them, and forgot to update it.  This rectifies that omission.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-fix-macaroon-auth into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2016-05-12 09:27:59 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2016-05-24 09:41:29 +0000
@@ -11,12 +11,9 @@
     ]
 
 import string
-try:
-    from urllib.parse import quote_plus
-except ImportError:
-    from urllib import quote_plus
 
 from lazr.restful.utils import get_current_browser_request
+from pymacaroons import Macaroon
 import requests
 from requests_toolbelt import MultipartEncoder
 from zope.interface import implementer
@@ -59,16 +56,31 @@
     # The union of the base64 and URL-safe base64 alphabets.
     allowed_chars = set(string.digits + string.letters + "+/=-_")
 
-    def __init__(self, tokens):
-        self.tokens = tokens
+    def __init__(self, root_macaroon_raw, unbound_discharge_macaroon_raw):
+        self.root_macaroon_raw = root_macaroon_raw
+        self.unbound_discharge_macaroon_raw = unbound_discharge_macaroon_raw
+
+    @classmethod
+    def _makeAuthParam(cls, key, value):
+        # Check framing.
+        assert set(key).issubset(cls.allowed_chars)
+        assert set(value).issubset(cls.allowed_chars)
+        return '%s="%s"' % (key, value)
+
+    @property
+    def discharge_macaroon_raw(self):
+        root_macaroon = Macaroon.deserialize(self.root_macaroon_raw)
+        unbound_discharge_macaroon = Macaroon.deserialize(
+            self.unbound_discharge_macaroon_raw)
+        discharge_macaroon = root_macaroon.prepare_for_request(
+            unbound_discharge_macaroon)
+        return discharge_macaroon.serialize()
 
     def __call__(self, r):
         params = []
-        for k, v in self.tokens.items():
-            # Check framing.
-            assert set(k).issubset(self.allowed_chars)
-            assert set(v).issubset(self.allowed_chars)
-            params.append('%s="%s"' % (k, v))
+        params.append(self._makeAuthParam("root", self.root_macaroon_raw))
+        params.append(
+            self._makeAuthParam("discharge", self.discharge_macaroon_raw))
         r.headers["Authorization"] = "Macaroon " + ", ".join(params)
         return r
 
@@ -144,10 +156,11 @@
         # that's currently difficult in jobs.
         try:
             assert snap.store_secrets is not None
-            assert "discharge" in snap.store_secrets
             urlfetch(
                 upload_url, method="POST", data=data,
-                auth=MacaroonAuth(snap.store_secrets))
+                auth=MacaroonAuth(
+                    snap.store_secrets["root"],
+                    snap.store_secrets["discharge"]))
         except requests.HTTPError as e:
             raise BadUploadResponse(e.args[0])
 

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2016-05-12 09:27:59 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2016-05-24 09:41:29 +0000
@@ -8,7 +8,7 @@
 __metaclass__ = type
 
 from cgi import FieldStorage
-from collections import OrderedDict
+import hashlib
 import io
 import json
 
@@ -18,14 +18,20 @@
     urlmatch,
     )
 from lazr.restful.utils import get_current_browser_request
+from pymacaroons import (
+    Macaroon,
+    Verifier,
+    )
 from requests import Request
 from requests.utils import parse_dict_header
 from testtools.matchers import (
     Contains,
     Equals,
+    KeysEqual,
     Matcher,
     MatchesDict,
     MatchesStructure,
+    Mismatch,
     StartsWith,
     )
 import transaction
@@ -46,17 +52,49 @@
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
+class MacaroonsVerify(Matcher):
+    """Matches if serialised macaroons pass verification."""
+
+    def __init__(self, key):
+        self.key = key
+
+    def match(self, macaroons):
+        mismatch = KeysEqual("root", "discharge").match(macaroons)
+        if mismatch is not None:
+            return mismatch
+        root_macaroon = Macaroon.deserialize(macaroons["root"])
+        discharge_macaroon = Macaroon.deserialize(macaroons["discharge"])
+        try:
+            Verifier().verify(root_macaroon, self.key, [discharge_macaroon])
+        except Exception as e:
+            return Mismatch("Macaroons do not verify: %s" % e)
+
+
 class TestMacaroonAuth(TestCase):
 
     def test_good(self):
         r = Request()
-        MacaroonAuth(OrderedDict([("root", "abc"), ("discharge", "def")]))(r)
-        self.assertEqual(
-            'Macaroon root="abc", discharge="def"', r.headers["Authorization"])
+        root_key = hashlib.sha256("root").hexdigest()
+        root_macaroon = Macaroon(key=root_key)
+        discharge_key = hashlib.sha256("discharge").hexdigest()
+        discharge_caveat_id = '{"secret": "thing"}'
+        root_macaroon.add_third_party_caveat(
+            "sso.example", discharge_key, discharge_caveat_id)
+        unbound_discharge_macaroon = Macaroon(
+            location="sso.example", key=discharge_key,
+            identifier=discharge_caveat_id)
+        MacaroonAuth(
+            root_macaroon.serialize(),
+            unbound_discharge_macaroon.serialize())(r)
+        auth_value = r.headers["Authorization"]
+        self.assertThat(auth_value, StartsWith("Macaroon "))
+        self.assertThat(
+            parse_dict_header(auth_value[len("Macaroon "):]),
+            MacaroonsVerify(root_key))
 
     def test_bad_framing(self):
         r = Request()
-        self.assertRaises(AssertionError, MacaroonAuth({"root": 'ev"il'}), r)
+        self.assertRaises(AssertionError, MacaroonAuth('ev"il', 'wic"ked'), r)
 
 
 class RequestMatches(Matcher):
@@ -79,11 +117,11 @@
             if mismatch is not None:
                 return mismatch
             auth_value = request.headers["Authorization"]
-            auth_scheme, auth_params = self.auth
+            auth_scheme, auth_params_matcher = self.auth
             mismatch = StartsWith(auth_scheme + " ").match(auth_value)
             if mismatch is not None:
                 return mismatch
-            mismatch = Equals(auth_params).match(
+            mismatch = auth_params_matcher.match(
                 parse_dict_header(auth_value[len(auth_scheme + " "):]))
             if mismatch is not None:
                 return mismatch
@@ -181,7 +219,20 @@
             self.snap_upload_request = request
             return {"status_code": 202, "content": {"success": True}}
 
-        store_secrets = {"root": "dummy-root", "discharge": "dummy-discharge"}
+        root_key = hashlib.sha256(self.factory.getUniqueString()).hexdigest()
+        root_macaroon = Macaroon(key=root_key)
+        discharge_key = hashlib.sha256(
+            self.factory.getUniqueString()).hexdigest()
+        discharge_caveat_id = self.factory.getUniqueString()
+        root_macaroon.add_third_party_caveat(
+            "sso.example", discharge_key, discharge_caveat_id)
+        unbound_discharge_macaroon = Macaroon(
+            location="sso.example", key=discharge_key,
+            identifier=discharge_caveat_id)
+        store_secrets = {
+            "root": root_macaroon.serialize(),
+            "discharge": unbound_discharge_macaroon.serialize(),
+            }
         snap = self.factory.makeSnap(
             store_upload=True,
             store_series=self.factory.makeSnappySeries(name="rolling"),
@@ -203,7 +254,8 @@
                     )}))
         self.assertThat(self.snap_upload_request, RequestMatches(
             url=Equals("http://sca.example/dev/api/snap-upload/";),
-            method=Equals("POST"), auth=("Macaroon", store_secrets),
+            method=Equals("POST"),
+            auth=("Macaroon", MacaroonsVerify(root_key)),
             form_data={
                 "name": MatchesStructure.byEquality(
                     name="name", value="test-snap"),