Skip to content

Commit

Permalink
Better upload errors when using API tokens (pypi#11885)
Browse files Browse the repository at this point in the history
  • Loading branch information
woodruffw committed Jul 27, 2022
1 parent 6350f22 commit 1a837df
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 35 deletions.
65 changes: 56 additions & 9 deletions tests/unit/macaroons/test_caveats.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -217,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()
Expand All @@ -227,35 +232,75 @@ 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
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):
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.
(
[
{"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",
Expand All @@ -276,4 +321,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
2 changes: 1 addition & 1 deletion tests/unit/macaroons/test_security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
19 changes: 14 additions & 5 deletions tests/unit/macaroons/test_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -151,7 +152,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()
)
Expand All @@ -165,7 +168,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()
)
Expand All @@ -179,15 +184,17 @@ def test_verify_invalid_macaroon(self, monkeypatch, user_service, macaroon_servi
user_id=user.id,
)

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)

context = pretend.stub()
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)
Expand All @@ -197,7 +204,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 == [
Expand Down
58 changes: 43 additions & 15 deletions warehouse/macaroons/caveats.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

import pymacaroons

from pyramid.security import Allowed

from warehouse.errors import WarehouseDenied
from warehouse.oidc import models as oidc_models
from warehouse.packaging.models import Project

Expand Down Expand Up @@ -59,17 +62,15 @@ 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":
Expand All @@ -88,6 +89,10 @@ def verify(self, predicate) -> bool:
projects = [p.normalized_name for p in self.verifier.identity.projects]
return self.verify_projects(projects)

if not isinstance(permissions, dict):
self.failure_reason = "invalid permissions format"
return False

projects = permissions.get("projects")
if projects is None:
self.failure_reason = "invalid projects in predicate"
Expand All @@ -103,7 +108,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:
Expand All @@ -124,7 +128,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:
Expand All @@ -138,7 +141,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
Expand All @@ -159,9 +161,35 @@ def verify(self, key):
self.verifier.satisfy_general(ProjectIDsCaveat(self))

try:
return self.verifier.verify(self.macaroon, key)
except (
pymacaroons.exceptions.MacaroonInvalidSignatureException,
Exception, # https://github.com/ecordell/pymacaroons/issues/50
):
return False
result = self.verifier.verify(self.macaroon, key)
except pymacaroons.exceptions.MacaroonInvalidSignatureException 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")
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
# 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")
2 changes: 1 addition & 1 deletion warehouse/macaroons/security_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def permits(self, 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
Expand Down
7 changes: 3 additions & 4 deletions warehouse/macaroons/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@
from sqlalchemy.orm.exc import NoResultFound
from zope.interface import implementer

from warehouse.accounts.models import User
from warehouse.macaroons.caveats import InvalidMacaroonError, Verifier
from warehouse.macaroons.interfaces import IMacaroonService
from warehouse.macaroons.models import Macaroon
from warehouse.oidc import models as oidc_models


@implementer(IMacaroonService)
Expand Down Expand Up @@ -121,11 +119,12 @@ def verify(self, raw_macaroon, context, principals, permission, identity):
raise InvalidMacaroonError("deleted or nonexistent macaroon")

verifier = Verifier(m, context, principals, permission, identity)
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, description, caveats, user_id=None, oidc_provider_id=None
Expand Down

0 comments on commit 1a837df

Please sign in to comment.