launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25220
[Merge] ~cjwatson/launchpad:snap-validate-macaroons-harder into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:snap-validate-macaroons-harder into launchpad:master.
Commit message:
Validate macaroons in Snap.completeAuthorization
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/389774
This isn't a full verification (which Launchpad can't do anyway), just a check that the macaroons are well-formed.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:snap-validate-macaroons-harder into launchpad:master.
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):
