From 8f8b7a38ce424fc5a85fc7aee86dcb299e13f224 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 18 Jul 2022 18:10:42 -0400 Subject: [PATCH 1/5] warehouse/macaroons: propagate errors nicely Signed-off-by: William Woodruff --- warehouse/macaroons/caveats.py | 46 +++++++++++++++++++------- warehouse/macaroons/security_policy.py | 2 +- warehouse/macaroons/services.py | 5 +-- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/warehouse/macaroons/caveats.py b/warehouse/macaroons/caveats.py index 03fe718940d3..550e90f2a8ce 100644 --- a/warehouse/macaroons/caveats.py +++ b/warehouse/macaroons/caveats.py @@ -15,6 +15,9 @@ import pymacaroons +from pyramid.security import Allowed + +from warehouse.errors import WarehouseDenied from warehouse.packaging.models import Project @@ -58,22 +61,23 @@ def verify_projects(self, projects) -> bool: def verify(self, predicate) -> bool: try: data = json.loads(predicate) - except ValueError: - self.failure_reason = "malformatted predicate" + version = data["version"] + permissions = data["permissions"] + except (KeyError, ValueError, TypeError): return False - if data.get("version") != 1: - self.failure_reason = "invalid version in predicate" + if version != 1: return False - permissions = data.get("permissions") if permissions is None: - self.failure_reason = "invalid permissions in predicate" return False if permissions == "user": # User-scoped tokens behave exactly like a user's normal credentials. return True + elif not isinstance(permissions, dict): + self.failure_reason = "invalid permissions format" + return False projects = permissions.get("projects") if projects is None: @@ -90,7 +94,6 @@ def verify(self, predicate): expiry = data["exp"] not_before = data["nbf"] except (KeyError, ValueError, TypeError): - self.failure_reason = "malformatted predicate" return False if not expiry or not not_before: @@ -111,7 +114,6 @@ def verify(self, predicate): data = json.loads(predicate) project_ids = data["project_ids"] except (KeyError, ValueError, TypeError): - self.failure_reason = "malformatted predicate" return False if not project_ids: @@ -125,7 +127,6 @@ def verify(self, predicate): return False if str(self.verifier.context.id) not in project_ids: - self.failure_reason = "current project does not matched scoped project IDs" return False return True @@ -145,9 +146,30 @@ def verify(self, key): self.verifier.satisfy_general(ProjectIDsCaveat(self)) try: - return self.verifier.verify(self.macaroon, key) + result = self.verifier.verify(self.macaroon, key) except ( pymacaroons.exceptions.MacaroonInvalidSignatureException, Exception, # https://github.com/ecordell/pymacaroons/issues/50 - ): - return False + ) as exc: + failure_reasons = [] + for cb in self.verifier.callbacks: + failure_reason = getattr(cb, "failure_reason", None) + if failure_reason is not None: + failure_reasons.append(failure_reason) + + # If we have a more detailed set of failure reasons, use them. + # Otherwise, use whatever the exception gives us. + if len(failure_reasons) > 0: + return WarehouseDenied( + ", ".join(failure_reasons), reason="invalid_api_token" + ) + else: + return WarehouseDenied(str(exc), reason="invalid_api_token") + + # NOTE: We should never hit this case, since pymacaroons *should* always either + # raise on failure *or* return true. But there's nothing stopping that from + # silently breaking in the future, so we check the result defensively here. + if not result: + return WarehouseDenied("unknown error", reason="invalid_api_token") + else: + return Allowed("signature and caveats OK") diff --git a/warehouse/macaroons/security_policy.py b/warehouse/macaroons/security_policy.py index 9f5ee5f49cb4..e5f15148013c 100644 --- a/warehouse/macaroons/security_policy.py +++ b/warehouse/macaroons/security_policy.py @@ -153,7 +153,7 @@ def permits(self, context, principals, permission): macaroon_service.verify(macaroon, context, principals, permission) except InvalidMacaroonError as exc: return WarehouseDenied( - f"Invalid API Token: {exc!r}", reason="invalid_api_token" + f"Invalid API Token: {exc}", reason="invalid_api_token" ) # If our Macaroon is verified, and for a valid permission then we'll pass diff --git a/warehouse/macaroons/services.py b/warehouse/macaroons/services.py index bb27125f90b4..2c9d602a5b52 100644 --- a/warehouse/macaroons/services.py +++ b/warehouse/macaroons/services.py @@ -121,11 +121,12 @@ def verify(self, raw_macaroon, context, principals, permission): raise InvalidMacaroonError("deleted or nonexistent macaroon") verifier = Verifier(m, context, principals, permission) - if verifier.verify(dm.key): + verified = verifier.verify(dm.key) + if verified: dm.last_used = datetime.datetime.now() return True - raise InvalidMacaroonError("invalid macaroon") + raise InvalidMacaroonError(verified.msg) def create_macaroon(self, location, user_id, description, caveats): """ From 5395ddc6704d2224dc735d63cf622741ec54186b Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 19 Jul 2022 11:36:29 -0400 Subject: [PATCH 2/5] tests: update tests for improved macaroons errors Signed-off-by: William Woodruff --- tests/unit/macaroons/test_caveats.py | 46 ++++++++++++++++---- tests/unit/macaroons/test_security_policy.py | 2 +- tests/unit/macaroons/test_services.py | 19 +++++--- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/tests/unit/macaroons/test_caveats.py b/tests/unit/macaroons/test_caveats.py index 09bb47bb862b..44955aa7ae14 100644 --- a/tests/unit/macaroons/test_caveats.py +++ b/tests/unit/macaroons/test_caveats.py @@ -19,7 +19,9 @@ import pytest from pymacaroons.exceptions import MacaroonInvalidSignatureException +from pyramid.security import Allowed +from warehouse.errors import WarehouseDenied from warehouse.macaroons.caveats import ( Caveat, ExpiryCaveat, @@ -48,6 +50,9 @@ class TestV1Caveat: ("invalid json", False), ('{"version": 2}', False), ('{"permissions": null, "version": 1}', False), + ('{"permissions": null, "version": 2}', False), + ('{"permissions": "user", "version": 2}', False), + ('{"permissions": "", "version": 2}', False), ], ) def test_verify_invalid_predicates(self, predicate, result): @@ -227,11 +232,26 @@ def test_verify_invalid_signature(self, monkeypatch): verifier = Verifier(macaroon, context, principals, permission) monkeypatch.setattr(verifier.verifier, "verify", verify) - assert verifier.verify(key) is False + assert not verifier.verify(key) + assert verify.calls == [pretend.call(macaroon, key)] + + def test_verify_inner_verifier_returns_false(self, monkeypatch): + verify = pretend.call_recorder(lambda macaroon, key: False) + macaroon = pretend.stub() + context = pretend.stub() + principals = pretend.stub() + permission = pretend.stub() + key = pretend.stub() + verifier = Verifier(macaroon, context, principals, permission) + + monkeypatch.setattr(verifier.verifier, "verify", verify) + status = verifier.verify(key) + assert not status + assert status.msg == "unknown error" assert verify.calls == [pretend.call(macaroon, key)] @pytest.mark.parametrize( - ["caveats", "valid"], + ["caveats", "expected_status"], [ # Both V1 and expiry present and valid. ( @@ -239,23 +259,31 @@ def test_verify_invalid_signature(self, monkeypatch): {"permissions": "user", "version": 1}, {"exp": int(time.time()) + 3600, "nbf": int(time.time()) - 1}, ], - True, + Allowed("signature and caveats OK"), ), # V1 only present and valid. - ([{"permissions": "user", "version": 1}], True), + ( + [{"permissions": "user", "version": 1}], + Allowed("signature and caveats OK"), + ), # V1 and expiry present but V1 invalid. - ([{"permissions": "bad", "version": 1}], False), + ( + [{"permissions": "bad", "version": 1}], + WarehouseDenied( + "invalid permissions format", reason="invalid_api_token" + ), + ), # V1 and expiry present but expiry invalid. ( [ {"permissions": "user", "version": 1}, {"exp": int(time.time()) + 1, "nbf": int(time.time()) + 3600}, ], - False, + WarehouseDenied("token is expired", reason="invalid_api_token"), ), ], ) - def test_verify(self, monkeypatch, caveats, valid): + def test_verify(self, monkeypatch, caveats, expected_status): key = os.urandom(32) m = pymacaroons.Macaroon( location="fakelocation", @@ -276,4 +304,6 @@ def test_verify(self, monkeypatch, caveats, valid): permission = pretend.stub() verifier = Verifier(deserialized_macaroon, context, principals, permission) - assert verifier.verify(key) is valid + status = verifier.verify(key) + assert bool(status) is bool(expected_status) + assert status.msg == expected_status.msg diff --git a/tests/unit/macaroons/test_security_policy.py b/tests/unit/macaroons/test_security_policy.py index 47f8859dc7dd..3016cfcfaed2 100644 --- a/tests/unit/macaroons/test_security_policy.py +++ b/tests/unit/macaroons/test_security_policy.py @@ -235,7 +235,7 @@ def test_permits_invalid_macaroon(self, monkeypatch): result = policy.permits(pretend.stub(), pretend.stub(), pretend.stub()) assert result == Denied("") - assert result.s == "Invalid API Token: InvalidMacaroonError('foo')" + assert result.s == "Invalid API Token: foo" def test_permits_valid_macaroon(self, monkeypatch): macaroon_service = pretend.stub( diff --git a/tests/unit/macaroons/test_services.py b/tests/unit/macaroons/test_services.py index 8f0487eb27b1..bc64c6e6588e 100644 --- a/tests/unit/macaroons/test_services.py +++ b/tests/unit/macaroons/test_services.py @@ -22,6 +22,7 @@ from pymacaroons.exceptions import MacaroonDeserializationException +from warehouse.errors import WarehouseDenied from warehouse.macaroons import services from warehouse.macaroons.models import Macaroon @@ -137,7 +138,9 @@ def test_verify_unprefixed_macaroon(self, macaroon_service): version=pymacaroons.MACAROON_V2, ).serialize() - with pytest.raises(services.InvalidMacaroonError): + with pytest.raises( + services.InvalidMacaroonError, match="malformed or nonexistent macaroon" + ): macaroon_service.verify( raw_macaroon, pretend.stub(), pretend.stub(), pretend.stub() ) @@ -151,7 +154,9 @@ def test_verify_no_macaroon(self, macaroon_service): ).serialize() raw_macaroon = f"pypi-{raw_macaroon}" - with pytest.raises(services.InvalidMacaroonError): + with pytest.raises( + services.InvalidMacaroonError, match="deleted or nonexistent macaroon" + ): macaroon_service.verify( raw_macaroon, pretend.stub(), pretend.stub(), pretend.stub() ) @@ -162,7 +167,9 @@ def test_verify_invalid_macaroon(self, monkeypatch, user_service, macaroon_servi "fake location", user.id, "fake description", [{"permissions": "user"}] ) - verifier_obj = pretend.stub(verify=pretend.call_recorder(lambda k: False)) + verifier_obj = pretend.stub( + verify=pretend.call_recorder(lambda k: WarehouseDenied("foo")) + ) verifier_cls = pretend.call_recorder(lambda *a: verifier_obj) monkeypatch.setattr(services, "Verifier", verifier_cls) @@ -170,7 +177,7 @@ def test_verify_invalid_macaroon(self, monkeypatch, user_service, macaroon_servi principals = pretend.stub() permissions = pretend.stub() - with pytest.raises(services.InvalidMacaroonError): + with pytest.raises(services.InvalidMacaroonError, match="foo"): macaroon_service.verify(raw_macaroon, context, principals, permissions) assert verifier_cls.calls == [ pretend.call(mock.ANY, context, principals, permissions) @@ -180,7 +187,9 @@ def test_deserialize_raw_macaroon_when_none(self, macaroon_service): raw_macaroon = pretend.stub() macaroon_service._extract_raw_macaroon = pretend.call_recorder(lambda a: None) - with pytest.raises(services.InvalidMacaroonError): + with pytest.raises( + services.InvalidMacaroonError, match="malformed or nonexistent macaroon" + ): macaroon_service._deserialize_raw_macaroon(raw_macaroon) assert macaroon_service._extract_raw_macaroon.calls == [ From 6826cf9aac86d8a2d863cc8a070820b7d146bbef Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 21 Jul 2022 12:23:58 -0400 Subject: [PATCH 3/5] macaroons/caveats: don't stringify generic exceptions Signed-off-by: William Woodruff --- warehouse/macaroons/caveats.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/warehouse/macaroons/caveats.py b/warehouse/macaroons/caveats.py index 550e90f2a8ce..bb537b0b8921 100644 --- a/warehouse/macaroons/caveats.py +++ b/warehouse/macaroons/caveats.py @@ -147,10 +147,7 @@ def verify(self, key): try: result = self.verifier.verify(self.macaroon, key) - except ( - pymacaroons.exceptions.MacaroonInvalidSignatureException, - Exception, # https://github.com/ecordell/pymacaroons/issues/50 - ) as exc: + except (pymacaroons.exceptions.MacaroonInvalidSignatureException,) as exc: failure_reasons = [] for cb in self.verifier.callbacks: failure_reason = getattr(cb, "failure_reason", None) @@ -165,6 +162,14 @@ def verify(self, key): ) else: return WarehouseDenied(str(exc), reason="invalid_api_token") + except Exception: + # The pymacaroons `verify` API with leak exceptions raised during caveat + # verification, which *normally* indicate a deserialization error + # (i.e., a malformed caveat body). + # When this happens, we don't want to display a random stringified + # Python exception to the user, so instead we emit a generic error. + # See https://github.com/ecordell/pymacaroons/issues/50 + return WarehouseDenied("malformed macaroon", reason="invalid_api_token") # NOTE: We should never hit this case, since pymacaroons *should* always either # raise on failure *or* return true. But there's nothing stopping that from From 369a68db8984483bbf9c9a0067ea9367317f4b37 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 21 Jul 2022 12:24:11 -0400 Subject: [PATCH 4/5] tests: update caveats tests Signed-off-by: William Woodruff --- tests/unit/macaroons/test_caveats.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/unit/macaroons/test_caveats.py b/tests/unit/macaroons/test_caveats.py index 44955aa7ae14..1610e31043e4 100644 --- a/tests/unit/macaroons/test_caveats.py +++ b/tests/unit/macaroons/test_caveats.py @@ -222,7 +222,7 @@ def test_creation(self): def test_verify_invalid_signature(self, monkeypatch): verify = pretend.call_recorder( - pretend.raiser(MacaroonInvalidSignatureException) + pretend.raiser(MacaroonInvalidSignatureException("Signatures do not match")) ) macaroon = pretend.stub() context = pretend.stub() @@ -232,7 +232,24 @@ def test_verify_invalid_signature(self, monkeypatch): verifier = Verifier(macaroon, context, principals, permission) monkeypatch.setattr(verifier.verifier, "verify", verify) - assert not verifier.verify(key) + status = verifier.verify(key) + assert not status + assert status.msg == "Signatures do not match" + assert verify.calls == [pretend.call(macaroon, key)] + + def test_verify_generic_exception(self, monkeypatch): + verify = pretend.call_recorder(pretend.raiser(ValueError)) + macaroon = pretend.stub() + context = pretend.stub() + principals = pretend.stub() + permission = pretend.stub() + key = pretend.stub() + verifier = Verifier(macaroon, context, principals, permission) + + monkeypatch.setattr(verifier.verifier, "verify", verify) + status = verifier.verify(key) + assert not status + assert status.msg == "malformed macaroon" assert verify.calls == [pretend.call(macaroon, key)] def test_verify_inner_verifier_returns_false(self, monkeypatch): From bd2f5ca323e8611570ffc1739cc681a6273b27e0 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 21 Jul 2022 13:27:11 -0400 Subject: [PATCH 5/5] Update warehouse/macaroons/caveats.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Éric --- warehouse/macaroons/caveats.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/macaroons/caveats.py b/warehouse/macaroons/caveats.py index bb537b0b8921..79040c100de8 100644 --- a/warehouse/macaroons/caveats.py +++ b/warehouse/macaroons/caveats.py @@ -147,7 +147,7 @@ def verify(self, key): try: result = self.verifier.verify(self.macaroon, key) - except (pymacaroons.exceptions.MacaroonInvalidSignatureException,) as exc: + except pymacaroons.exceptions.MacaroonInvalidSignatureException as exc: failure_reasons = [] for cb in self.verifier.callbacks: failure_reason = getattr(cb, "failure_reason", None)