Merge ~cjwatson/launchpad:snap-validate-macaroons-harder into launchpad:master

[Colin Watson]

Validate macaroons in Snap.completeAuthorization

This isn't a full verification (which Launchpad can't do anyway), just a check that the macaroons are well-formed.

[Thiago F. Pappacena (pappacena)]

LGTM

review: Approve

diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index 0a481dc..37b3cda 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap package views."""
@@ -1187,7 +1187,7 @@ class TestSnapAuthorizeView(BaseTestSnapView):
         # If the form does not include a discharge macaroon, the "complete"
         # action fails.
         with person_logged_in(self.snap.owner):
-            self.snap.store_secrets = {"root": "root"}
+            self.snap.store_secrets = {"root": Macaroon().serialize()}
             transaction.commit()
             form = {"field.actions.complete": "1"}
             view = create_initialized_view(
@@ -1203,12 +1203,14 @@ class TestSnapAuthorizeView(BaseTestSnapView):
     def test_complete_authorization(self):
         # If the form includes a discharge macaroon, the "complete" action
         # succeeds and records the new secrets.
+        root_macaroon = Macaroon()
+        discharge_macaroon = Macaroon()
         with person_logged_in(self.snap.owner):
-            self.snap.store_secrets = {"root": "root"}
+            self.snap.store_secrets = {"root": root_macaroon.serialize()}
             transaction.commit()
             form = {
                 "field.actions.complete": "1",
-                "field.discharge_macaroon": "discharge",
+                "field.discharge_macaroon": discharge_macaroon.serialize(),
                 }
             view = create_initialized_view(
                 self.snap, "+authorize", form=form, method="POST",
@@ -1223,7 +1225,8 @@ class TestSnapAuthorizeView(BaseTestSnapView):
                 self.snap.name,
                 view.request.response.notifications[0].message)
             self.assertEqual(
-                {"root": "root", "discharge": "discharge"},
+                {"root": root_macaroon.serialize(),
+                 "discharge": discharge_macaroon.serialize()},
                 self.snap.store_secrets)
 
 
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index a6460b7..eca689b 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Snap package interfaces."""
@@ -8,6 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 __all__ = [
+    'BadMacaroon',
     'BadSnapSearchContext',
     'BadSnapSource',
     'CannotAuthorizeStoreUploads',
@@ -28,7 +29,7 @@ __all__ = [
     'SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG',
     'SNAP_TESTING_FLAGS',
     'SNAP_WEBHOOKS_FEATURE_FLAG',
-    'SnapAuthorizationBadMacaroon',
+    'SnapAuthorizationBadGeneratedMacaroon',
     'SnapBuildAlreadyPending',
     'SnapBuildArchiveOwnerMismatch',
     'SnapBuildDisallowedArchitecture',
@@ -237,11 +238,16 @@ class CannotAuthorizeStoreUploads(Exception):
 
 
 @error_status(http_client.INTERNAL_SERVER_ERROR)
-class SnapAuthorizationBadMacaroon(Exception):
+class SnapAuthorizationBadGeneratedMacaroon(Exception):
     """The macaroon generated to authorize store uploads is unusable."""
 
 
 @error_status(http_client.BAD_REQUEST)
+class BadMacaroon(Exception):
+    """A macaroon supplied by the user is invalid."""
+
+
+@error_status(http_client.BAD_REQUEST)
 class CannotRequestAutoBuilds(Exception):
     """Snap package is not configured for automatic builds."""
 
@@ -597,8 +603,8 @@ class ISnapEdit(IWebhookTarget):
         :raises BadRequestPackageUploadResponse: if the store returns an
             error or a response without a macaroon when asked to issue a
             package_upload macaroon.
-        :raises SnapAuthorizationBadMacaroon: if the package_upload macaroon
-            returned by the store has unsuitable SSO caveats.
+        :raises SnapAuthorizationBadGeneratedMacaroon: if the package_upload
+            macaroon returned by the store has unsuitable SSO caveats.
         :return: The SSO caveat ID from the package_upload macaroon returned
             by the store.  The third-party site should acquire a discharge
             macaroon for this caveat using OpenID and then call
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 4860fad..aa147ef 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -139,6 +139,7 @@ from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.adapters.buildarch import determine_architectures_to_build
 from lp.snappy.interfaces.snap import (
+    BadMacaroon,
     BadSnapSearchContext,
     BadSnapSource,
     CannotAuthorizeStoreUploads,
@@ -154,7 +155,7 @@ from lp.snappy.interfaces.snap import (
     NoSourceForSnap,
     NoSuchSnap,
     SNAP_PRIVATE_FEATURE_FLAG,
-    SnapAuthorizationBadMacaroon,
+    SnapAuthorizationBadGeneratedMacaroon,
     SnapBuildAlreadyPending,
     SnapBuildArchiveOwnerMismatch,
     SnapBuildDisallowedArchitecture,
@@ -588,9 +589,10 @@ class Snap(Storm, WebhookTargetMixin):
         # in some other service, since the user can't do anything about it
         # and it should show up in our OOPS reports.
         if not sso_caveats:
-            raise SnapAuthorizationBadMacaroon("Macaroon has no SSO caveats")
+            raise SnapAuthorizationBadGeneratedMacaroon(
+                "Macaroon has no SSO caveats")
         elif len(sso_caveats) > 1:
-            raise SnapAuthorizationBadMacaroon(
+            raise SnapAuthorizationBadGeneratedMacaroon(
                 "Macaroon has multiple SSO caveats")
         self.store_secrets = {'root': root_macaroon_raw}
         return sso_caveats[0].caveat_id
@@ -599,6 +601,10 @@ class Snap(Storm, WebhookTargetMixin):
                               discharge_macaroon=None):
         """See `ISnap`."""
         if root_macaroon is not None:
+            try:
+                Macaroon.deserialize(root_macaroon)
+            except Exception:
+                raise BadMacaroon("root_macaroon is invalid.")
             self.store_secrets = {"root": root_macaroon}
         else:
             if self.store_secrets is None or "root" not in self.store_secrets:
@@ -606,6 +612,10 @@ class Snap(Storm, WebhookTargetMixin):
                     "beginAuthorization must be called before "
                     "completeAuthorization.")
         if discharge_macaroon is not None:
+            try:
+                Macaroon.deserialize(discharge_macaroon)
+            except Exception:
+                raise BadMacaroon("discharge_macaroon is invalid.")
             container = getUtility(IEncryptedContainer, "snap-store-secrets")
             if container.can_encrypt:
                 self.store_secrets["discharge_encrypted"] = (
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 0b712fe..8c87c8a 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -3287,20 +3287,23 @@ class TestSnapWebservice(TestCaseWithFactory):
     def test_completeAuthorization(self):
         with admin_logged_in():
             snappy_series = self.factory.makeSnappySeries()
+        root_macaroon = Macaroon()
+        discharge_macaroon = Macaroon()
         snap = self.factory.makeSnap(
             registrant=self.person, store_upload=True,
             store_series=snappy_series,
             store_name=self.factory.getUniqueUnicode(),
-            store_secrets={"root": "dummy-root"})
+            store_secrets={"root": root_macaroon.serialize()})
         snap_url = api_url(snap)
         logout()
         response = self.webservice.named_post(
             snap_url, "completeAuthorization",
-            discharge_macaroon="dummy-discharge")
+            discharge_macaroon=discharge_macaroon.serialize())
         self.assertEqual(200, response.status)
         with person_logged_in(self.person):
             self.assertEqual(
-                {"root": "dummy-root", "discharge": "dummy-discharge"},
+                {"root": root_macaroon.serialize(),
+                 "discharge": discharge_macaroon.serialize()},
                 snap.store_secrets)
 
     def test_completeAuthorization_without_beginAuthorization(self):
@@ -3312,22 +3315,26 @@ class TestSnapWebservice(TestCaseWithFactory):
             store_name=self.factory.getUniqueUnicode())
         snap_url = api_url(snap)
         logout()
+        discharge_macaroon = Macaroon()
         response = self.webservice.named_post(
             snap_url, "completeAuthorization",
-            discharge_macaroon="dummy-discharge")
-        self.assertEqual(400, response.status)
-        self.assertEqual(
-            "beginAuthorization must be called before completeAuthorization.",
-            response.body)
+            discharge_macaroon=discharge_macaroon.serialize())
+        self.assertThat(response, MatchesStructure.byEquality(
+            status=400,
+            body=(
+                b"beginAuthorization must be called before "
+                b"completeAuthorization.")))
 
     def test_completeAuthorization_unauthorized(self):
         with admin_logged_in():
             snappy_series = self.factory.makeSnappySeries()
+        root_macaroon = Macaroon()
+        discharge_macaroon = Macaroon()
         snap = self.factory.makeSnap(
             registrant=self.person, store_upload=True,
             store_series=snappy_series,
             store_name=self.factory.getUniqueUnicode(),
-            store_secrets={"root": "dummy-root"})
+            store_secrets={"root": root_macaroon.serialize()})
         snap_url = api_url(snap)
         other_person = self.factory.makePerson()
         other_webservice = webservice_for_person(
@@ -3335,7 +3342,7 @@ class TestSnapWebservice(TestCaseWithFactory):
         other_webservice.default_api_version = "devel"
         response = other_webservice.named_post(
             snap_url, "completeAuthorization",
-            discharge_macaroon="dummy-discharge")
+            discharge_macaroon=discharge_macaroon.serialize())
         self.assertEqual(401, response.status)
 
     def test_completeAuthorization_both_macaroons(self):
@@ -3349,13 +3356,17 @@ class TestSnapWebservice(TestCaseWithFactory):
             store_name=self.factory.getUniqueUnicode())
         snap_url = api_url(snap)
         logout()
+        root_macaroon = Macaroon()
+        discharge_macaroon = Macaroon()
         response = self.webservice.named_post(
             snap_url, "completeAuthorization",
-            root_macaroon="dummy-root", discharge_macaroon="dummy-discharge")
+            root_macaroon=root_macaroon.serialize(),
+            discharge_macaroon=discharge_macaroon.serialize())
         self.assertEqual(200, response.status)
         with person_logged_in(self.person):
             self.assertEqual(
-                {"root": "dummy-root", "discharge": "dummy-discharge"},
+                {"root": root_macaroon.serialize(),
+                 "discharge": discharge_macaroon.serialize()},
                 snap.store_secrets)
 
     def test_completeAuthorization_only_root_macaroon(self):
@@ -3370,11 +3381,44 @@ class TestSnapWebservice(TestCaseWithFactory):
             store_name=self.factory.getUniqueUnicode())
         snap_url = api_url(snap)
         logout()
+        root_macaroon = Macaroon()
         response = self.webservice.named_post(
-            snap_url, "completeAuthorization", root_macaroon="dummy-root")
+            snap_url, "completeAuthorization",
+            root_macaroon=root_macaroon.serialize())
         self.assertEqual(200, response.status)
         with person_logged_in(self.person):
-            self.assertEqual({"root": "dummy-root"}, snap.store_secrets)
+            self.assertEqual(
+                {"root": root_macaroon.serialize()}, snap.store_secrets)
+
+    def test_completeAuthorization_malformed_root_macaroon(self):
+        with admin_logged_in():
+            snappy_series = self.factory.makeSnappySeries()
+        snap = self.factory.makeSnap(
+            registrant=self.person, store_upload=True,
+            store_series=snappy_series,
+            store_name=self.factory.getUniqueUnicode())
+        snap_url = api_url(snap)
+        logout()
+        response = self.webservice.named_post(
+            snap_url, "completeAuthorization", root_macaroon="nonsense")
+        self.assertThat(response, MatchesStructure.byEquality(
+            status=400, body=b"root_macaroon is invalid."))
+
+    def test_completeAuthorization_malformed_discharge_macaroon(self):
+        with admin_logged_in():
+            snappy_series = self.factory.makeSnappySeries()
+        snap = self.factory.makeSnap(
+            registrant=self.person, store_upload=True,
+            store_series=snappy_series,
+            store_name=self.factory.getUniqueUnicode())
+        snap_url = api_url(snap)
+        logout()
+        response = self.webservice.named_post(
+            snap_url, "completeAuthorization",
+            root_macaroon=Macaroon().serialize(),
+            discharge_macaroon="nonsense")
+        self.assertThat(response, MatchesStructure.byEquality(
+            status=400, body=b"discharge_macaroon is invalid."))
 
     def test_completeAuthorization_encrypted(self):
         private_key = PrivateKey.generate()
@@ -3384,16 +3428,18 @@ class TestSnapWebservice(TestCaseWithFactory):
                 bytes(private_key.public_key)).decode("UTF-8"))
         with admin_logged_in():
             snappy_series = self.factory.makeSnappySeries()
+        root_macaroon = Macaroon()
+        discharge_macaroon = Macaroon()
         snap = self.factory.makeSnap(
             registrant=self.person, store_upload=True,
             store_series=snappy_series,
             store_name=self.factory.getUniqueUnicode(),
-            store_secrets={"root": "dummy-root"})
+            store_secrets={"root": root_macaroon.serialize()})
         snap_url = api_url(snap)
         logout()
         response = self.webservice.named_post(
             snap_url, "completeAuthorization",
-            discharge_macaroon="dummy-discharge")
+            discharge_macaroon=discharge_macaroon.serialize())
         self.assertEqual(200, response.status)
         self.pushConfig(
             "snappy",
@@ -3402,9 +3448,9 @@ class TestSnapWebservice(TestCaseWithFactory):
         container = getUtility(IEncryptedContainer, "snap-store-secrets")
         with person_logged_in(self.person):
             self.assertThat(snap.store_secrets, MatchesDict({
-                "root": Equals("dummy-root"),
+                "root": Equals(root_macaroon.serialize()),
                 "discharge_encrypted": AfterPreprocessing(
-                    container.decrypt, Equals("dummy-discharge")),
+                    container.decrypt, Equals(discharge_macaroon.serialize())),
                 }))
 
     def makeBuildableDistroArchSeries(self, **kwargs):