From 05b3d06e5d88b656d0f19fe8109fa973a35347f5 Mon Sep 17 00:00:00 2001 From: Carey Hoffman Date: Wed, 30 Aug 2023 10:00:04 -0700 Subject: [PATCH 01/10] Refactor OIDC mint token endpoint Make it easier to mint tokens with additional publishers --- tests/unit/oidc/test_views.py | 90 ++++++------- warehouse/oidc/models/_core.py | 4 +- warehouse/oidc/services.py | 2 +- warehouse/oidc/views.py | 237 +++++++++++++++++++-------------- 4 files changed, 187 insertions(+), 146 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index b32c53ec3034..eff4845a3c88 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -25,6 +25,8 @@ from warehouse.macaroons.interfaces import IMacaroonService from warehouse.oidc import errors, views from warehouse.oidc.interfaces import IOIDCPublisherService +from warehouse.oidc.models import github +from warehouse.packaging.models import Project from warehouse.rate_limiting.interfaces import IRateLimiter @@ -69,13 +71,13 @@ def test_oidc_audience(): assert response == {"audience": "fakeaudience"} -def test_mint_token_from_oidc_not_enabled(): +def test_mint_token_from_github_oidc_not_enabled(): request = pretend.stub( response=pretend.stub(status=None), flags=pretend.stub(disallow_oidc=lambda *a: True), ) - response = views.mint_token_from_oidc(request) + response = views.mint_token_from_oidc_github(request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -109,7 +111,7 @@ def test_mint_token_from_oidc_not_enabled(): {"token": {}}, ], ) -def test_mint_token_from_oidc_invalid_payload(body): +def test_mint_token_from_github_oidc_invalid_payload(body): class Request: def __init__(self): self.response = pretend.stub(status=None) @@ -120,7 +122,7 @@ def body(self): return json.dumps(body) req = Request() - resp = views.mint_token_from_oidc(req) + resp = views.mint_token_from_oidc_github(req) assert req.response.status == 422 assert resp["message"] == "Token request failed" @@ -142,7 +144,7 @@ def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token_from_oidc(request) + response = views.mint_token(oidc_service, request, "faketoken") assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -154,9 +156,6 @@ def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(): ], } - assert request.find_service.calls == [ - pretend.call(IOIDCPublisherService, name="github") - ] assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] @@ -176,7 +175,7 @@ def test_mint_token_from_trusted_publisher_lookup_fails(): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token_from_oidc(request) + response = views.mint_token_from_oidc_github(request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -216,7 +215,7 @@ def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_reques ) db_request.find_service = pretend.call_recorder(lambda *a, **kw: oidc_service) - resp = views.mint_token_from_oidc(db_request) + resp = views.mint_token(oidc_service, db_request, "faketoken") assert db_request.response.status_code == 422 assert resp == { "message": "Token request failed", @@ -230,9 +229,6 @@ def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_reques assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] assert oidc_service.find_publisher.calls == [pretend.call(claims, pending=True)] - assert db_request.find_service.calls == [ - pretend.call(IOIDCPublisherService, name="github") - ] def test_mint_token_from_oidc_pending_publisher_ok( @@ -279,7 +275,7 @@ def test_mint_token_from_oidc_pending_publisher_ok( } monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - resp = views.mint_token_from_oidc(db_request) + resp = views.mint_token_from_oidc_github(db_request) assert resp["success"] assert resp["token"].startswith("pypi-") @@ -328,25 +324,22 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ) db_request.flags.oidc_enabled = lambda f: False - db_request.body = json.dumps( - { - "token": ( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" - "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" - "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" - "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" - "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" - "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" - "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" - "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" - "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" - "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" - "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" - "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" - "0vbdNhK32c2oC-E" - ) - } + token = ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" + "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" + "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" + "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" + "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" + "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" + "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" + "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" + "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" + "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" + "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" + "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" + "0vbdNhK32c2oC-E" ) + db_request.body = json.dumps({"token": token}) db_request.remote_addr = "0.0.0.0" ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) @@ -356,7 +349,9 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( } monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - resp = views.mint_token_from_oidc(db_request) + oidc_service = db_request.find_service(IOIDCPublisherService, name="github") + + resp = views.mint_token(oidc_service, db_request, token) assert resp["success"] assert resp["token"].startswith("pypi-") @@ -388,18 +383,21 @@ def test_mint_token_from_oidc_no_pending_publisher_ok( time = pretend.stub(time=pretend.call_recorder(lambda: 0)) monkeypatch.setattr(views, "time", time) - project = pretend.stub( - id="fakeprojectid", - record_event=pretend.call_recorder(lambda **kw: None), + project = Project(id="fakeprojectid") + monkeypatch.setattr( + project, "record_event", pretend.call_recorder(lambda **kw: None) ) - publisher = pretend.stub( - id="fakepublisherid", - projects=[project], - publisher_name="fakepublishername", - publisher_url=lambda x=None: "https://fake/url", + + publisher = github.GitHubPublisher( + repository_name="fakerepo", + repository_owner="fakeowner", + repository_owner_id="fakeid", + workflow_filename="fakeworkflow.yml", + environment="fakeenv", ) + publisher.projects = [project] # NOTE: Can't set __str__ using pretend.stub() - monkeypatch.setattr(publisher.__class__, "__str__", lambda s: "fakespecifier") + monkeypatch.setattr(publisher, "id", "fakepublisherid") def _find_publisher(claims, pending=False): if pending: @@ -435,7 +433,7 @@ def find_service(iface, **kw): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token_from_oidc(request) + response = views.mint_token(oidc_service, request, "faketoken") assert response == { "success": True, "token": "raw-macaroon", @@ -449,7 +447,7 @@ def find_service(iface, **kw): assert macaroon_service.create_macaroon.calls == [ pretend.call( "fakedomain", - f"OpenID token: fakespecifier ({datetime.fromtimestamp(0).isoformat()})", + f"OpenID token: fakeworkflow.yml ({datetime.fromtimestamp(0).isoformat()})", [ caveats.OIDCPublisher( oidc_publisher_id="fakepublisherid", @@ -467,8 +465,8 @@ def find_service(iface, **kw): request=request, additional={ "expires": 900, - "publisher_name": "fakepublishername", - "publisher_url": "https://fake/url", + "publisher_name": "GitHub", + "publisher_url": f"https://github.com/{publisher.repository_owner}/{publisher.repository_name}", # noqa }, ) ] diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 8330449a4f4f..5f2edf7e580b 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -232,7 +232,9 @@ def publisher_name(self) -> str: # pragma: no cover # Only concrete subclasses are constructed. raise NotImplementedError - def publisher_url(self, claims=None) -> str | None: # pragma: no cover + def publisher_url( + self, claims: SignedClaims | None = None + ) -> str: # pragma: no cover """ NOTE: This is **NOT** a `@property` because we pass `claims` to it. When calling, make sure to use `publisher_url()` diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index c5635dc7c176..b78ba8e323d8 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -282,7 +282,7 @@ def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: def find_publisher( self, signed_claims: SignedClaims, *, pending: bool = False - ) -> OIDCPublisher | PendingOIDCPublisher | None: + ) -> OIDCPublisher | PendingOIDCPublisher: metrics_tags = [f"publisher:{self.publisher}"] self.metrics.increment( "warehouse.oidc.find_publisher.attempt", diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 251e13677c1c..79b05e45b66d 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -13,8 +13,10 @@ import time from datetime import datetime +from typing import TypedDict from pydantic import BaseModel, StrictStr, ValidationError +from pyramid.request import Request from pyramid.response import Response from pyramid.view import view_config from sqlalchemy import func @@ -24,19 +26,32 @@ from warehouse.events.tags import EventTag from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService -from warehouse.oidc.errors import InvalidPublisherError +from warehouse.macaroons.services import DatabaseMacaroonService from warehouse.oidc.interfaces import IOIDCPublisherService -from warehouse.oidc.models import PendingOIDCPublisher +from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher +from warehouse.oidc.services import OIDCPublisherService from warehouse.packaging.interfaces import IProjectService from warehouse.packaging.models import ProjectFactory from warehouse.rate_limiting.interfaces import IRateLimiter +class Error(TypedDict): + code: str + description: str + + +class JsonRespone(TypedDict, total=False): + message: str | None + errors: list[Error] | None + token: StrictStr | None + success: bool | None + + class TokenPayload(BaseModel): token: StrictStr -def _ratelimiters(request): +def _ratelimiters(request: Request) -> dict[str, IRateLimiter]: return { "user.oidc": request.find_service( IRateLimiter, name="user_oidc.publisher.register" @@ -47,6 +62,15 @@ def _ratelimiters(request): } +def _invalid(errors: list[Error], request: Request) -> JsonRespone: + request.response.status = 422 + + return { + "message": "Token request failed", + "errors": errors, + } + + @view_config( route_name="oidc.audience", require_methods=["GET"], @@ -54,13 +78,13 @@ def _ratelimiters(request): require_csrf=False, has_translations=False, ) -def oidc_audience(request): +def oidc_audience(request: Request): if request.flags.disallow_oidc(): return Response( status=403, json={"message": "Trusted publishing functionality not enabled"} ) - audience = request.registry.settings["warehouse.oidc.audience"] + audience: str = request.registry.settings["warehouse.oidc.audience"] return {"audience": audience} @@ -71,96 +95,109 @@ def oidc_audience(request): require_csrf=False, has_translations=True, ) -def mint_token_from_oidc(request): - def _invalid(errors): - request.response.status = 422 - return {"message": "Token request failed", "errors": errors} - +def mint_token_from_oidc_github(request: Request): if request.flags.disallow_oidc(AdminFlagValue.DISALLOW_GITHUB_OIDC): return _invalid( errors=[ { "code": "not-enabled", - "description": ( - "GitHub-based trusted publishing functionality not enabled" - ), + "description": "GitHub-based trusted publishing functionality not enabled", # noqa } - ] + ], + request=request, ) try: payload = TokenPayload.model_validate_json(request.body) unverified_jwt = payload.token except ValidationError as exc: - return _invalid(errors=[{"code": "invalid-payload", "description": str(exc)}]) + return _invalid( + errors=[{"code": "invalid-payload", "description": str(exc)}], + request=request, + ) # For the time being, GitHub is our only OIDC publisher. # In the future, this should locate the correct service based on an # identifier in the request body. - oidc_service = request.find_service(IOIDCPublisherService, name="github") + oidc_service: OIDCPublisherService = request.find_service( + IOIDCPublisherService, name="github" + ) + + return mint_token(oidc_service, request, unverified_jwt) + + +def mint_token( + oidc_service: OIDCPublisherService, request: Request, unverified_jwt: str +) -> JsonRespone: claims = oidc_service.verify_jwt_signature(unverified_jwt) if not claims: return _invalid( errors=[ {"code": "invalid-token", "description": "malformed or invalid token"} - ] + ], + request=request, ) # First, try to find a pending publisher. try: pending_publisher = oidc_service.find_publisher(claims, pending=True) - factory = ProjectFactory(request) - - # If the project already exists, this pending publisher is no longer - # valid and needs to be removed. - # NOTE: This is mostly a sanity check, since we dispose of invalidated - # pending publishers below. - if pending_publisher.project_name in factory: - request.db.delete(pending_publisher) - return _invalid( - errors=[ - { - "code": "invalid-pending-publisher", - "description": "valid token, but project already exists", - } - ] + if isinstance(pending_publisher, PendingOIDCPublisher): + factory = ProjectFactory(request) + + # If the project already exists, this pending publisher is no longer + # valid and needs to be removed. + # NOTE: This is mostly a sanity check, since we dispose of invalidated + # pending publishers below. + if pending_publisher.project_name in factory: + request.db.delete(pending_publisher) + return _invalid( + errors=[ + { + "code": "invalid-pending-publisher", + "description": "valid token, but project already exists", + } + ], + request=request, + ) + + # Create the new project, and reify the pending publisher against it. + project_service = request.find_service(IProjectService) + new_project = project_service.create_project( + pending_publisher.project_name, + pending_publisher.added_by, + request, + ratelimited=False, ) - # Create the new project, and reify the pending publisher against it. - project_service = request.find_service(IProjectService) - new_project = project_service.create_project( - pending_publisher.project_name, - pending_publisher.added_by, - request, - ratelimited=False, - ) - oidc_service.reify_pending_publisher(pending_publisher, new_project) - - # Successfully converting a pending publisher into a normal publisher - # is a positive signal, so we reset the associated ratelimits. - ratelimiters = _ratelimiters(request) - ratelimiters["user.oidc"].clear(pending_publisher.added_by.id) - ratelimiters["ip.oidc"].clear(request.remote_addr) - - # There might be other pending publishers for the same project name, - # which we've now invalidated by creating the project. These would - # be disposed of on use, but we explicitly dispose of them here while - # also sending emails to their owners. - stale_pending_publishers = ( - request.db.query(PendingOIDCPublisher) - .filter( - func.normalize_pep426_name(PendingOIDCPublisher.project_name) - == func.normalize_pep426_name(pending_publisher.project_name) + publisher = oidc_service.reify_pending_publisher( + pending_publisher, new_project ) - .all() - ) - for stale_publisher in stale_pending_publishers: - send_pending_trusted_publisher_invalidated_email( - request, - stale_publisher.added_by, - project_name=stale_publisher.project_name, + + # Successfully converting a pending publisher into a normal publisher + # is a positive signal, so we reset the associated ratelimits. + ratelimiters = _ratelimiters(request) + ratelimiters["user.oidc"].clear(pending_publisher.added_by.id) + ratelimiters["ip.oidc"].clear(request.remote_addr) + + # There might be other pending publishers for the same project name, + # which we've now invalidated by creating the project. These would + # be disposed of on use, but we explicitly dispose of them here while + # also sending emails to their owners. + stale_pending_publishers = ( + request.db.query(PendingOIDCPublisher) + .filter( + func.normalize_pep426_name(PendingOIDCPublisher.project_name) + == func.normalize_pep426_name(pending_publisher.project_name) + ) + .all() ) - request.db.delete(stale_publisher) + for stale_publisher in stale_pending_publishers: + send_pending_trusted_publisher_invalidated_email( + request, + stale_publisher.added_by, + project_name=stale_publisher.project_name, + ) + request.db.delete(stale_publisher) except InvalidPublisherError: # If the claim set isn't valid for a pending publisher, it's OK, we # will try finding a regular publisher @@ -178,40 +215,44 @@ def _invalid(errors): "code": "invalid-publisher", "description": f"valid token, but no corresponding publisher ({e})", } - ] + ], + request=request, ) - # At this point, we've verified that the given JWT is valid for the given - # project. All we need to do is mint a new token. - # NOTE: For OIDC-minted API tokens, the Macaroon's description string - # is purely an implementation detail and is not displayed to the user. - macaroon_service = request.find_service(IMacaroonService, context=None) - not_before = int(time.time()) - expires_at = not_before + 900 - serialized, dm = macaroon_service.create_macaroon( - request.domain, - ( - f"OpenID token: {str(publisher)} " - f"({datetime.fromtimestamp(not_before).isoformat()})" - ), - [ - caveats.OIDCPublisher( - oidc_publisher_id=str(publisher.id), + if isinstance(publisher, OIDCPublisher): + # At this point, we've verified that the given JWT is valid for the given + # project. All we need to do is mint a new token. + # NOTE: For OIDC-minted API tokens, the Macaroon's description string + # is purely an implementation detail and is not displayed to the user. + macaroon_service: DatabaseMacaroonService = request.find_service( + IMacaroonService, context=None + ) + not_before = int(time.time()) + expires_at = not_before + 900 + serialized, dm = macaroon_service.create_macaroon( + request.domain, + ( + f"OpenID token: {str(publisher)} " + f"({datetime.fromtimestamp(not_before).isoformat()})" ), - caveats.ProjectID(project_ids=[str(p.id) for p in publisher.projects]), - caveats.Expiration(expires_at=expires_at, not_before=not_before), - ], - oidc_publisher_id=publisher.id, - additional={"oidc": {"ref": claims.get("ref"), "sha": claims.get("sha")}}, - ) - for project in publisher.projects: - project.record_event( - tag=EventTag.Project.ShortLivedAPITokenAdded, - request=request, - additional={ - "expires": expires_at, - "publisher_name": publisher.publisher_name, - "publisher_url": publisher.publisher_url(), - }, + [ + caveats.OIDCPublisher( + oidc_publisher_id=str(publisher.id), + ), + caveats.ProjectID(project_ids=[str(p.id) for p in publisher.projects]), + caveats.Expiration(expires_at=expires_at, not_before=not_before), + ], + oidc_publisher_id=publisher.id, + additional={"oidc": {"ref": claims.get("ref"), "sha": claims.get("sha")}}, ) - return {"success": True, "token": serialized} + for project in publisher.projects: + project.record_event( + tag=EventTag.Project.ShortLivedAPITokenAdded, + request=request, + additional={ + "expires": expires_at, + "publisher_name": publisher.publisher_name, + "publisher_url": publisher.publisher_url(), + }, + ) + return {"success": True, "token": serialized} From 40b17082fb04a4868a1cf1f7929eac86b7159a3f Mon Sep 17 00:00:00 2001 From: Carey Hoffman Date: Wed, 30 Aug 2023 10:00:04 -0700 Subject: [PATCH 02/10] Move default JWT validation to general OIDC handler function --- tests/unit/oidc/test_views.py | 11 ++++++----- warehouse/oidc/views.py | 25 ++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index eff4845a3c88..5bc19f149341 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -122,7 +122,8 @@ def body(self): return json.dumps(body) req = Request() - resp = views.mint_token_from_oidc_github(req) + oidc_service = pretend.stub() + resp = views.mint_token(oidc_service, req) assert req.response.status == 422 assert resp["message"] == "Token request failed" @@ -144,7 +145,7 @@ def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token(oidc_service, request, "faketoken") + response = views.mint_token(oidc_service, request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -215,7 +216,7 @@ def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_reques ) db_request.find_service = pretend.call_recorder(lambda *a, **kw: oidc_service) - resp = views.mint_token(oidc_service, db_request, "faketoken") + resp = views.mint_token(oidc_service, db_request) assert db_request.response.status_code == 422 assert resp == { "message": "Token request failed", @@ -351,7 +352,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( oidc_service = db_request.find_service(IOIDCPublisherService, name="github") - resp = views.mint_token(oidc_service, db_request, token) + resp = views.mint_token(oidc_service, db_request) assert resp["success"] assert resp["token"].startswith("pypi-") @@ -433,7 +434,7 @@ def find_service(iface, **kw): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token(oidc_service, request, "faketoken") + response = views.mint_token(oidc_service, request) assert response == { "success": True, "token": "raw-macaroon", diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 79b05e45b66d..0bce78775e4d 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -107,15 +107,6 @@ def mint_token_from_oidc_github(request: Request): request=request, ) - try: - payload = TokenPayload.model_validate_json(request.body) - unverified_jwt = payload.token - except ValidationError as exc: - return _invalid( - errors=[{"code": "invalid-payload", "description": str(exc)}], - request=request, - ) - # For the time being, GitHub is our only OIDC publisher. # In the future, this should locate the correct service based on an # identifier in the request body. @@ -123,12 +114,20 @@ def mint_token_from_oidc_github(request: Request): IOIDCPublisherService, name="github" ) - return mint_token(oidc_service, request, unverified_jwt) + return mint_token(oidc_service, request) -def mint_token( - oidc_service: OIDCPublisherService, request: Request, unverified_jwt: str -) -> JsonRespone: +def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonRespone: + unverified_jwt: str + try: + payload = TokenPayload.model_validate_json(request.body) + unverified_jwt = payload.token + except ValidationError as exc: + return _invalid( + errors=[{"code": "invalid-payload", "description": str(exc)}], + request=request, + ) + claims = oidc_service.verify_jwt_signature(unverified_jwt) if not claims: return _invalid( From e7505315958a19896dcb1bad21d18dfecd213324 Mon Sep 17 00:00:00 2001 From: Carey Hoffman Date: Wed, 30 Aug 2023 10:00:04 -0700 Subject: [PATCH 03/10] publisher_name to method. Add more factories. Publisher agnostic token handler tests --- tests/unit/oidc/test_views.py | 159 ++++++++++++++++++++++----------- warehouse/oidc/models/_core.py | 2 +- warehouse/oidc/services.py | 3 +- warehouse/oidc/views.py | 89 ++++++++++-------- 4 files changed, 161 insertions(+), 92 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 5bc19f149341..e58c9bd61e10 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -18,15 +18,13 @@ import pytest from tests.common.db.accounts import UserFactory -from tests.common.db.oidc import PendingGitHubPublisherFactory +from tests.common.db.oidc import GitHubPublisherFactory, PendingGitHubPublisherFactory from tests.common.db.packaging import ProjectFactory from warehouse.events.tags import EventTag from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService from warehouse.oidc import errors, views from warehouse.oidc.interfaces import IOIDCPublisherService -from warehouse.oidc.models import github -from warehouse.packaging.models import Project from warehouse.rate_limiting.interfaces import IRateLimiter @@ -111,7 +109,7 @@ def test_mint_token_from_github_oidc_not_enabled(): {"token": {}}, ], ) -def test_mint_token_from_github_oidc_invalid_payload(body): +def test_mint_token_oidc_invalid_payload(body): class Request: def __init__(self): self.response = pretend.stub(status=None) @@ -202,7 +200,9 @@ def test_mint_token_from_trusted_publisher_lookup_fails(): def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_request): project = ProjectFactory.create() - pending_publisher = PendingGitHubPublisherFactory.create(project_name=project.name) + pending_publisher = PendingGitHubPublisherFactory.create( + project_name=project.name, + ) db_request.flags.disallow_oidc = lambda f=None: False db_request.body = json.dumps({"token": "faketoken"}) @@ -325,22 +325,25 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ) db_request.flags.oidc_enabled = lambda f: False - token = ( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" - "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" - "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" - "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" - "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" - "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" - "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" - "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" - "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" - "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" - "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" - "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" - "0vbdNhK32c2oC-E" + db_request.body = json.dumps( + { + "token": ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" + "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" + "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" + "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" + "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" + "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" + "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" + "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" + "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" + "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" + "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" + "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" + "0vbdNhK32c2oC-E" + ) + } ) - db_request.body = json.dumps({"token": token}) db_request.remote_addr = "0.0.0.0" ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) @@ -350,9 +353,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( } monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - oidc_service = db_request.find_service(IOIDCPublisherService, name="github") - - resp = views.mint_token(oidc_service, db_request) + resp = views.mint_token_from_oidc_github(db_request) assert resp["success"] assert resp["token"].startswith("pypi-") @@ -370,6 +371,71 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ] +def test_mint_token_from_oidc_only_pending_publisher_fail(monkeypatch, db_request): + pending_publisher = PendingGitHubPublisherFactory() + + def _find_publisher(claims, pending=False): + return pending_publisher + + oidc_service = pretend.stub( + verify_jwt_signature=pretend.call_recorder( + lambda token: {"ref": "someref", "sha": "somesha"} + ), + find_publisher=pretend.call_recorder(_find_publisher), + reify_pending_publisher=pretend.call_recorder( + lambda *a, **kw: pending_publisher + ), + ) + + db_request.body = json.dumps( + { + "token": ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" + "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" + "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" + "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" + "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" + "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" + "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" + "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" + "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" + "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" + "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" + "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" + "0vbdNhK32c2oC-E" + ) + } + ) + + ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) + ratelimiters = { + "user.oidc": ratelimiter, + "ip.oidc": ratelimiter, + } + monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) + + send_pending_trusted_publisher_invalidated_email = pretend.call_recorder( + lambda *a, **kw: None + ) + monkeypatch.setattr( + views, + "send_pending_trusted_publisher_invalidated_email", + send_pending_trusted_publisher_invalidated_email, + ) + + response = views.mint_token(oidc_service, db_request) + + assert response == { + "message": "Token request failed", + "errors": [ + { + "code": "invalid-publisher", + "description": ("valid token, but no corresponding publisher"), + } + ], + } + + @pytest.mark.parametrize( ("claims_in_token", "claims_input"), [ @@ -379,30 +445,25 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ], ) def test_mint_token_from_oidc_no_pending_publisher_ok( - monkeypatch, claims_in_token, claims_input + monkeypatch, db_request, claims_in_token, claims_input ): time = pretend.stub(time=pretend.call_recorder(lambda: 0)) monkeypatch.setattr(views, "time", time) - project = Project(id="fakeprojectid") - monkeypatch.setattr( - project, "record_event", pretend.call_recorder(lambda **kw: None) + project = pretend.stub( + id="fakeprojectid", + record_event=pretend.call_recorder(lambda **kw: None), ) - publisher = github.GitHubPublisher( - repository_name="fakerepo", - repository_owner="fakeowner", - repository_owner_id="fakeid", - workflow_filename="fakeworkflow.yml", - environment="fakeenv", - ) - publisher.projects = [project] + publisher = GitHubPublisherFactory() + monkeypatch.setattr(publisher.__class__, "projects", [project]) + publisher.publisher_url = pretend.call_recorder(lambda **kw: "https://fake/url") # NOTE: Can't set __str__ using pretend.stub() - monkeypatch.setattr(publisher, "id", "fakepublisherid") + monkeypatch.setattr(publisher.__class__, "__str__", lambda s: "fakespecifier") def _find_publisher(claims, pending=False): if pending: - raise errors.InvalidPublisherError + return None else: return publisher @@ -425,16 +486,11 @@ def find_service(iface, **kw): return macaroon_service assert False, iface - request = pretend.stub( - response=pretend.stub(status=None), - body=json.dumps({"token": "faketoken"}), - find_service=find_service, - domain="fakedomain", - remote_addr="0.0.0.0", - flags=pretend.stub(disallow_oidc=lambda *a: False), - ) + monkeypatch.setattr(db_request, "find_service", find_service) + monkeypatch.setattr(db_request, "body", json.dumps({"token": "faketoken"})) + monkeypatch.setattr(db_request, "domain", "fakedomain") - response = views.mint_token(oidc_service, request) + response = views.mint_token(oidc_service, db_request) assert response == { "success": True, "token": "raw-macaroon", @@ -445,29 +501,30 @@ def find_service(iface, **kw): pretend.call(claims_in_token, pending=True), pretend.call(claims_in_token, pending=False), ] + assert macaroon_service.create_macaroon.calls == [ pretend.call( "fakedomain", - f"OpenID token: fakeworkflow.yml ({datetime.fromtimestamp(0).isoformat()})", + f"OpenID token: fakespecifier ({datetime.fromtimestamp(0).isoformat()})", [ caveats.OIDCPublisher( - oidc_publisher_id="fakepublisherid", + oidc_publisher_id=str(publisher.id), ), caveats.ProjectID(project_ids=["fakeprojectid"]), caveats.Expiration(expires_at=900, not_before=0), ], - oidc_publisher_id="fakepublisherid", + oidc_publisher_id=str(publisher.id), additional={"oidc": claims_input}, ) ] assert project.record_event.calls == [ pretend.call( tag=EventTag.Project.ShortLivedAPITokenAdded, - request=request, + request=db_request, additional={ "expires": 900, "publisher_name": "GitHub", - "publisher_url": f"https://github.com/{publisher.repository_owner}/{publisher.repository_name}", # noqa + "publisher_url": "https://fake/url", }, ) ] diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 5f2edf7e580b..63765ccaa72e 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -234,7 +234,7 @@ def publisher_name(self) -> str: # pragma: no cover def publisher_url( self, claims: SignedClaims | None = None - ) -> str: # pragma: no cover + ) -> str | None: # pragma: no cover """ NOTE: This is **NOT** a `@property` because we pass `claims` to it. When calling, make sure to use `publisher_url()` diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index b78ba8e323d8..d3921d4fc30d 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -283,6 +283,7 @@ def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: def find_publisher( self, signed_claims: SignedClaims, *, pending: bool = False ) -> OIDCPublisher | PendingOIDCPublisher: + """Returns a publisher for the given claims, or raises an error.""" metrics_tags = [f"publisher:{self.publisher}"] self.metrics.increment( "warehouse.oidc.find_publisher.attempt", @@ -306,7 +307,7 @@ def find_publisher( ) raise e - def reify_pending_publisher(self, pending_publisher, project): + def reify_pending_publisher(self, pending_publisher, project) -> OIDCPublisher: new_publisher = pending_publisher.reify(self.db) project.oidc_publishers.append(new_publisher) return new_publisher diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 0bce78775e4d..3ca3861fa9ab 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -27,6 +27,7 @@ from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService from warehouse.macaroons.services import DatabaseMacaroonService +from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.services import OIDCPublisherService @@ -140,9 +141,9 @@ def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonResp # First, try to find a pending publisher. try: pending_publisher = oidc_service.find_publisher(claims, pending=True) - if isinstance(pending_publisher, PendingOIDCPublisher): - factory = ProjectFactory(request) + factory = ProjectFactory(request) + if isinstance(pending_publisher, PendingOIDCPublisher): # If the project already exists, this pending publisher is no longer # valid and needs to be removed. # NOTE: This is mostly a sanity check, since we dispose of invalidated @@ -168,9 +169,7 @@ def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonResp ratelimited=False, ) - publisher = oidc_service.reify_pending_publisher( - pending_publisher, new_project - ) + oidc_service.reify_pending_publisher(pending_publisher, new_project) # Successfully converting a pending publisher into a normal publisher # is a positive signal, so we reset the associated ratelimits. @@ -218,40 +217,52 @@ def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonResp request=request, ) - if isinstance(publisher, OIDCPublisher): - # At this point, we've verified that the given JWT is valid for the given - # project. All we need to do is mint a new token. - # NOTE: For OIDC-minted API tokens, the Macaroon's description string - # is purely an implementation detail and is not displayed to the user. - macaroon_service: DatabaseMacaroonService = request.find_service( - IMacaroonService, context=None + if not isinstance(publisher, OIDCPublisher): + # This should be impossible, but we have to perform this type check to + # appease mypy otherwise we get type errors in the code after this + # point. + return _invalid( + errors=[ + { + "code": "invalid-publisher", + "description": "valid token, but no corresponding publisher", + } + ], + request=request, ) - not_before = int(time.time()) - expires_at = not_before + 900 - serialized, dm = macaroon_service.create_macaroon( - request.domain, - ( - f"OpenID token: {str(publisher)} " - f"({datetime.fromtimestamp(not_before).isoformat()})" + # At this point, we've verified that the given JWT is valid for the given + # project. All we need to do is mint a new token. + # NOTE: For OIDC-minted API tokens, the Macaroon's description string + # is purely an implementation detail and is not displayed to the user. + macaroon_service: DatabaseMacaroonService = request.find_service( + IMacaroonService, context=None + ) + not_before = int(time.time()) + expires_at = not_before + 900 + serialized, dm = macaroon_service.create_macaroon( + request.domain, + ( + f"OpenID token: {str(publisher)} " + f"({datetime.fromtimestamp(not_before).isoformat()})" + ), + [ + caveats.OIDCPublisher( + oidc_publisher_id=str(publisher.id), ), - [ - caveats.OIDCPublisher( - oidc_publisher_id=str(publisher.id), - ), - caveats.ProjectID(project_ids=[str(p.id) for p in publisher.projects]), - caveats.Expiration(expires_at=expires_at, not_before=not_before), - ], - oidc_publisher_id=publisher.id, - additional={"oidc": {"ref": claims.get("ref"), "sha": claims.get("sha")}}, + caveats.ProjectID(project_ids=[str(p.id) for p in publisher.projects]), + caveats.Expiration(expires_at=expires_at, not_before=not_before), + ], + oidc_publisher_id=str(publisher.id), + additional={"oidc": {"ref": claims.get("ref"), "sha": claims.get("sha")}}, + ) + for project in publisher.projects: + project.record_event( + tag=EventTag.Project.ShortLivedAPITokenAdded, + request=request, + additional={ + "expires": expires_at, + "publisher_name": publisher.publisher_name, + "publisher_url": publisher.publisher_url(), + }, ) - for project in publisher.projects: - project.record_event( - tag=EventTag.Project.ShortLivedAPITokenAdded, - request=request, - additional={ - "expires": expires_at, - "publisher_name": publisher.publisher_name, - "publisher_url": publisher.publisher_url(), - }, - ) - return {"success": True, "token": serialized} + return {"success": True, "token": serialized} From 2c0b230aaaa95c372c4003ef06764cf21b93787e Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 5 Jan 2024 15:45:06 -0500 Subject: [PATCH 04/10] oidc/views: typo: JsonRespone -> JsonResponse Signed-off-by: William Woodruff --- warehouse/oidc/views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 3ca10c857341..8d10443058f7 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -38,7 +38,7 @@ class Error(TypedDict): description: str -class JsonRespone(TypedDict, total=False): +class JsonResponse(TypedDict, total=False): message: str | None errors: list[Error] | None token: StrictStr | None @@ -60,7 +60,7 @@ def _ratelimiters(request: Request) -> dict[str, IRateLimiter]: } -def _invalid(errors: list[Error], request: Request) -> JsonRespone: +def _invalid(errors: list[Error], request: Request) -> JsonResponse: request.response.status = 422 return { @@ -115,7 +115,7 @@ def mint_token_from_oidc_github(request: Request): return mint_token(oidc_service, request) -def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonRespone: +def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonResponse: unverified_jwt: str try: payload = TokenPayload.model_validate_json(request.body) From 437a4901668df9f2e4c7c259f8f1a5668c0cfec1 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 5 Jan 2024 15:53:21 -0500 Subject: [PATCH 05/10] lintage, begin making OIDC endpoint generic Signed-off-by: William Woodruff --- tests/unit/oidc/test_views.py | 9 +++++---- warehouse/oidc/__init__.py | 1 + warehouse/oidc/views.py | 10 +++++++++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index cdcbd5c71736..62112de752ab 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -11,6 +11,7 @@ # limitations under the License. import json + from datetime import datetime import pretend @@ -75,7 +76,7 @@ def test_mint_token_from_github_oidc_not_enabled(): flags=pretend.stub(disallow_oidc=lambda *a: True), ) - response = views.mint_token_from_oidc_github(request) + response = views.mint_token_from_oidc(request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -174,7 +175,7 @@ def test_mint_token_from_trusted_publisher_lookup_fails(): flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token_from_oidc_github(request) + response = views.mint_token_from_oidc(request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -276,7 +277,7 @@ def test_mint_token_from_oidc_pending_publisher_ok( } monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - resp = views.mint_token_from_oidc_github(db_request) + resp = views.mint_token_from_oidc(db_request) assert resp["success"] assert resp["token"].startswith("pypi-") @@ -353,7 +354,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( } monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - resp = views.mint_token_from_oidc_github(db_request) + resp = views.mint_token_from_oidc(db_request) assert resp["success"] assert resp["token"].startswith("pypi-") diff --git a/warehouse/oidc/__init__.py b/warehouse/oidc/__init__.py index 15452ce8871c..8fddb01d1696 100644 --- a/warehouse/oidc/__init__.py +++ b/warehouse/oidc/__init__.py @@ -47,6 +47,7 @@ def includeme(config): auth = config.get_settings().get("auth.domain") config.add_route("oidc.audience", "/_/oidc/audience", domain=auth) + config.add_route("oidc.mint_token", "/_/oidc/mint-token", domain=auth) config.add_route("oidc.github.mint_token", "/_/oidc/github/mint-token", domain=auth) # Compute OIDC metrics periodically diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 8d10443058f7..f2cf20b9f95c 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -11,6 +11,7 @@ # limitations under the License. import time + from datetime import datetime from typing import TypedDict @@ -93,7 +94,14 @@ def oidc_audience(request: Request): require_csrf=False, has_translations=True, ) -def mint_token_from_oidc_github(request: Request): +@view_config( + route_name="oidc.mint_token", + require_methods=["POST"], + renderer="json", + require_csrf=False, + has_translations=True, +) +def mint_token_from_oidc(request: Request): if request.flags.disallow_oidc(AdminFlagValue.DISALLOW_GITHUB_OIDC): return _invalid( errors=[ From 6de12b0055d3e05e8c2d15ff249c9050b1a69992 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 5 Jan 2024 16:55:06 -0500 Subject: [PATCH 06/10] tests, warehouse: generic OIDC minting endpoint Needs coverage, but the idea is done. Signed-off-by: William Woodruff --- tests/conftest.py | 52 +++++++++++++++ tests/unit/oidc/test_views.py | 116 ++++++++-------------------------- warehouse/oidc/utils.py | 11 ++++ warehouse/oidc/views.py | 66 +++++++++++++------ 4 files changed, 138 insertions(+), 107 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 1a205f4bb79e..204daa776c18 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json import os import os.path import re @@ -379,6 +380,57 @@ def oidc_service(db_session): ) +@pytest.fixture +def dummy_oidc_jwt(): + # { + # "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", + # "sub": "repo:foo/bar", + # "aud": "pypi", + # "ref": "fake", + # "sha": "fake", + # "repository": "foo/bar", + # "repository_owner": "foo", + # "repository_owner_id": "123", + # "run_id": "fake", + # "run_number": "fake", + # "run_attempt": "1", + # "repository_id": "fake", + # "actor_id": "fake", + # "actor": "foo", + # "workflow": "fake", + # "head_ref": "fake", + # "base_ref": "fake", + # "event_name": "fake", + # "ref_type": "fake", + # "environment": "fake", + # "job_workflow_ref": "foo/bar/.github/workflows/example.yml@fake", + # "iss": "https://token.actions.githubusercontent.com", + # "nbf": 1650663265, + # "exp": 1650664165, + # "iat": 1650663865 + # } + return ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRiZ" + "TUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJweXB" + "pIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2JhciIsI" + "nJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTIzIiw" + "icnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQiOiIxI" + "iwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3RvciI6ImZ" + "vbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2VfcmVmIjoiZ" + "mFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW52aXJvbm1" + "lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1Yi93b3JrZ" + "mxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uYWN0aW9ucy5" + "naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cCI6MTY1MDY2N" + "DE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et0vbdNhK32c2o" + "C-E" + ) + + +@pytest.fixture +def dummy_oidc_payload(dummy_oidc_jwt): + return json.dumps({"token": dummy_oidc_jwt}) + + @pytest.fixture def macaroon_service(db_session): return macaroon_services.DatabaseMacaroonService(db_session) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 62112de752ab..8e963816a327 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -70,8 +70,9 @@ def test_oidc_audience(): assert response == {"audience": "fakeaudience"} -def test_mint_token_from_github_oidc_not_enabled(): +def test_mint_token_oidc_not_enabled(dummy_oidc_payload): request = pretend.stub( + body=dummy_oidc_payload, response=pretend.stub(status=None), flags=pretend.stub(disallow_oidc=lambda *a: True), ) @@ -83,9 +84,7 @@ def test_mint_token_from_github_oidc_not_enabled(): "errors": [ { "code": "not-enabled", - "description": ( - "GitHub-based trusted publishing functionality not enabled" - ), + "description": ("Trusted publishing functionality not enabled"), } ], } @@ -110,7 +109,7 @@ def test_mint_token_from_github_oidc_not_enabled(): {"token": {}}, ], ) -def test_mint_token_oidc_invalid_payload(body): +def test_mint_token_from_oidc_invalid_payload(body): class Request: def __init__(self): self.response = pretend.stub(status=None) @@ -121,8 +120,7 @@ def body(self): return json.dumps(body) req = Request() - oidc_service = pretend.stub() - resp = views.mint_token(oidc_service, req) + resp = views.mint_token_from_oidc(req) assert req.response.status == 422 assert resp["message"] == "Token request failed" @@ -133,18 +131,17 @@ def body(self): assert isinstance(err["description"], str) -def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(): +def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(dummy_oidc_jwt): oidc_service = pretend.stub( verify_jwt_signature=pretend.call_recorder(lambda token: None), ) request = pretend.stub( response=pretend.stub(status=None), - body=json.dumps({"token": "faketoken"}), find_service=pretend.call_recorder(lambda cls, **kw: oidc_service), flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token(oidc_service, request) + response = views.mint_token(oidc_service, dummy_oidc_jwt, request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -156,10 +153,10 @@ def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(): ], } - assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] + assert oidc_service.verify_jwt_signature.calls == [pretend.call(dummy_oidc_jwt)] -def test_mint_token_from_trusted_publisher_lookup_fails(): +def test_mint_token_trusted_publisher_lookup_fails(dummy_oidc_jwt): claims = pretend.stub() message = "some message" oidc_service = pretend.stub( @@ -170,12 +167,11 @@ def test_mint_token_from_trusted_publisher_lookup_fails(): ) request = pretend.stub( response=pretend.stub(status=None), - body=json.dumps({"token": "faketoken"}), find_service=pretend.call_recorder(lambda cls, **kw: oidc_service), flags=pretend.stub(disallow_oidc=lambda *a: False), ) - response = views.mint_token_from_oidc(request) + response = views.mint_token(oidc_service, dummy_oidc_jwt, request) assert request.response.status == 422 assert response == { "message": "Token request failed", @@ -189,24 +185,22 @@ def test_mint_token_from_trusted_publisher_lookup_fails(): ], } - assert request.find_service.calls == [ - pretend.call(IOIDCPublisherService, name="github"), - ] - assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] + assert oidc_service.verify_jwt_signature.calls == [pretend.call(dummy_oidc_jwt)] assert oidc_service.find_publisher.calls == [ pretend.call(claims, pending=True), pretend.call(claims, pending=False), ] -def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_request): +def test_mint_token_pending_publisher_project_already_exists( + db_request, dummy_oidc_jwt +): project = ProjectFactory.create() pending_publisher = PendingGitHubPublisherFactory.create( project_name=project.name, ) db_request.flags.disallow_oidc = lambda f=None: False - db_request.body = json.dumps({"token": "faketoken"}) claims = pretend.stub() oidc_service = pretend.stub( @@ -217,7 +211,7 @@ def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_reques ) db_request.find_service = pretend.call_recorder(lambda *a, **kw: oidc_service) - resp = views.mint_token(oidc_service, db_request) + resp = views.mint_token(oidc_service, dummy_oidc_jwt, db_request) assert db_request.response.status_code == 422 assert resp == { "message": "Token request failed", @@ -229,13 +223,14 @@ def test_mint_token_from_oidc_pending_publisher_project_already_exists(db_reques ], } - assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] + assert oidc_service.verify_jwt_signature.calls == [pretend.call(dummy_oidc_jwt)] assert oidc_service.find_publisher.calls == [pretend.call(claims, pending=True)] def test_mint_token_from_oidc_pending_publisher_ok( monkeypatch, db_request, + dummy_oidc_payload, ): user = UserFactory.create() pending_publisher = PendingGitHubPublisherFactory.create( @@ -249,25 +244,7 @@ def test_mint_token_from_oidc_pending_publisher_ok( ) db_request.flags.disallow_oidc = lambda f=None: False - db_request.body = json.dumps( - { - "token": ( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" - "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" - "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" - "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" - "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" - "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" - "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" - "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" - "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" - "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" - "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" - "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" - "0vbdNhK32c2oC-E" - ) - } - ) + db_request.body = dummy_oidc_payload db_request.remote_addr = "0.0.0.0" ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) @@ -288,7 +265,7 @@ def test_mint_token_from_oidc_pending_publisher_ok( def test_mint_token_from_pending_trusted_publisher_invalidates_others( - monkeypatch, db_request + monkeypatch, db_request, dummy_oidc_payload ): time = pretend.stub(time=pretend.call_recorder(lambda: 0)) monkeypatch.setattr(views, "time", time) @@ -326,25 +303,7 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ) db_request.flags.oidc_enabled = lambda f: False - db_request.body = json.dumps( - { - "token": ( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" - "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" - "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" - "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" - "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" - "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" - "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" - "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" - "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" - "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" - "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" - "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" - "0vbdNhK32c2oC-E" - ) - } - ) + db_request.body = dummy_oidc_payload db_request.remote_addr = "0.0.0.0" ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) @@ -372,7 +331,9 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ] -def test_mint_token_from_oidc_only_pending_publisher_fail(monkeypatch, db_request): +def test_mint_token_from_oidc_only_pending_publisher_fail( + monkeypatch, db_request, dummy_oidc_jwt +): pending_publisher = PendingGitHubPublisherFactory() def _find_publisher(claims, pending=False): @@ -388,26 +349,6 @@ def _find_publisher(claims, pending=False): ), ) - db_request.body = json.dumps( - { - "token": ( - "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI2ZTY3YjFjYi0yYjhkLTRi" - "ZTUtOTFjYi03NTdlZGIyZWM5NzAiLCJzdWIiOiJyZXBvOmZvby9iYXIiLCJhdWQiOiJwe" - "XBpIiwicmVmIjoiZmFrZSIsInNoYSI6ImZha2UiLCJyZXBvc2l0b3J5IjoiZm9vL2Jhci" - "IsInJlcG9zaXRvcnlfb3duZXIiOiJmb28iLCJyZXBvc2l0b3J5X293bmVyX2lkIjoiMTI" - "zIiwicnVuX2lkIjoiZmFrZSIsInJ1bl9udW1iZXIiOiJmYWtlIiwicnVuX2F0dGVtcHQi" - "OiIxIiwicmVwb3NpdG9yeV9pZCI6ImZha2UiLCJhY3Rvcl9pZCI6ImZha2UiLCJhY3Rvc" - "iI6ImZvbyIsIndvcmtmbG93IjoiZmFrZSIsImhlYWRfcmVmIjoiZmFrZSIsImJhc2Vfcm" - "VmIjoiZmFrZSIsImV2ZW50X25hbWUiOiJmYWtlIiwicmVmX3R5cGUiOiJmYWtlIiwiZW5" - "2aXJvbm1lbnQiOiJmYWtlIiwiam9iX3dvcmtmbG93X3JlZiI6ImZvby9iYXIvLmdpdGh1" - "Yi93b3JrZmxvd3MvZXhhbXBsZS55bWxAZmFrZSIsImlzcyI6Imh0dHBzOi8vdG9rZW4uY" - "WN0aW9ucy5naXRodWJ1c2VyY29udGVudC5jb20iLCJuYmYiOjE2NTA2NjMyNjUsImV4cC" - "I6MTY1MDY2NDE2NSwiaWF0IjoxNjUwNjYzODY1fQ.f-FMv5FF5sdxAWeUilYDt9NoE7Et" - "0vbdNhK32c2oC-E" - ) - } - ) - ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) ratelimiters = { "user.oidc": ratelimiter, @@ -424,7 +365,7 @@ def _find_publisher(claims, pending=False): send_pending_trusted_publisher_invalidated_email, ) - response = views.mint_token(oidc_service, db_request) + response = views.mint_token(oidc_service, dummy_oidc_jwt, db_request) assert response == { "message": "Token request failed", @@ -445,8 +386,8 @@ def _find_publisher(claims, pending=False): ({"sha": "somesha"}, {"ref": None, "sha": "somesha"}), ], ) -def test_mint_token_from_oidc_no_pending_publisher_ok( - monkeypatch, db_request, claims_in_token, claims_input +def test_mint_token_no_pending_publisher_ok( + monkeypatch, db_request, claims_in_token, claims_input, dummy_oidc_jwt ): time = pretend.stub(time=pretend.call_recorder(lambda: 0)) monkeypatch.setattr(views, "time", time) @@ -488,16 +429,15 @@ def find_service(iface, **kw): assert False, iface monkeypatch.setattr(db_request, "find_service", find_service) - monkeypatch.setattr(db_request, "body", json.dumps({"token": "faketoken"})) monkeypatch.setattr(db_request, "domain", "fakedomain") - response = views.mint_token(oidc_service, db_request) + response = views.mint_token(oidc_service, dummy_oidc_jwt, db_request) assert response == { "success": True, "token": "raw-macaroon", } - assert oidc_service.verify_jwt_signature.calls == [pretend.call("faketoken")] + assert oidc_service.verify_jwt_signature.calls == [pretend.call(dummy_oidc_jwt)] assert oidc_service.find_publisher.calls == [ pretend.call(claims_in_token, pending=True), pretend.call(claims_in_token, pending=False), diff --git a/warehouse/oidc/utils.py b/warehouse/oidc/utils.py index 8cdef771214a..b66a60a73988 100644 --- a/warehouse/oidc/utils.py +++ b/warehouse/oidc/utils.py @@ -16,6 +16,7 @@ from pyramid.authorization import Authenticated +from warehouse.admin.flags import AdminFlagValue from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import SignedClaims from warehouse.oidc.models import ( @@ -30,6 +31,16 @@ GITHUB_OIDC_ISSUER_URL = "https://token.actions.githubusercontent.com" GOOGLE_OIDC_ISSUER_URL = "https://accounts.google.com" +OIDC_ISSUER_SERVICE_NAMES = { + GITHUB_OIDC_ISSUER_URL: "github", + GOOGLE_OIDC_ISSUER_URL: "google", +} + +OIDC_ISSUER_ADMIN_FLAGS = { + GITHUB_OIDC_ISSUER_URL: AdminFlagValue.DISALLOW_GITHUB_OIDC, + GOOGLE_OIDC_ISSUER_URL: AdminFlagValue.DISALLOW_GOOGLE_OIDC, +} + OIDC_ISSUER_URLS = {GITHUB_OIDC_ISSUER_URL, GOOGLE_OIDC_ISSUER_URL} OIDC_PUBLISHER_CLASSES: dict[str, dict[bool, type[OIDCPublisherMixin]]] = { diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index f2cf20b9f95c..47d96f31c092 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -15,12 +15,13 @@ from datetime import datetime from typing import TypedDict +import jwt + from pydantic import BaseModel, StrictStr, ValidationError from pyramid.request import Request from pyramid.response import Response from pyramid.view import view_config -from warehouse.admin.flags import AdminFlagValue from warehouse.events.tags import EventTag from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService @@ -29,6 +30,7 @@ from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.services import OIDCPublisherService +from warehouse.oidc.utils import OIDC_ISSUER_ADMIN_FLAGS, OIDC_ISSUER_SERVICE_NAMES from warehouse.packaging.interfaces import IProjectService from warehouse.packaging.models import ProjectFactory from warehouse.rate_limiting.interfaces import IRateLimiter @@ -102,38 +104,64 @@ def oidc_audience(request: Request): has_translations=True, ) def mint_token_from_oidc(request: Request): - if request.flags.disallow_oidc(AdminFlagValue.DISALLOW_GITHUB_OIDC): + try: + payload = TokenPayload.model_validate_json(request.body) + unverified_jwt = payload.token + except ValidationError as exc: + return _invalid( + errors=[{"code": "invalid-payload", "description": str(exc)}], + request=request, + ) + + # We currently have an **unverified** JWT. To verify it, we need to + # know which OIDC service's keyring to check it against. + # To do this, we gingerly peek into the unverified claims and + # use the `iss` to key into the right `OIDCPublisherService`. + try: + unverified_claims = jwt.decode( + unverified_jwt, options=dict(verify_signature=False) + ) + unverified_issuer: str = unverified_claims["iss"] + except Exception: + return _invalid( + errors=[{"code": "invalid-payload", "description": "malformed JWT"}], + request=request, + ) + + # Associate the given issuer claim with Warehouse's OIDCPublisherService. + service_name = OIDC_ISSUER_SERVICE_NAMES.get(unverified_issuer) + if not service_name: + return _invalid( + errors=[ + { + "code": "invalid-payload", + "description": "unknown trusted publishing issuer", + } + ], + request=request, + ) + + if request.flags.disallow_oidc(OIDC_ISSUER_ADMIN_FLAGS[unverified_issuer]): return _invalid( errors=[ { "code": "not-enabled", - "description": "GitHub-based trusted publishing functionality not enabled", # noqa + "description": "Trusted publishing functionality not enabled", # noqa } ], request=request, ) - # For the time being, GitHub is our only OIDC publisher. - # In the future, this should locate the correct service based on an - # identifier in the request body. oidc_service: OIDCPublisherService = request.find_service( - IOIDCPublisherService, name="github" + IOIDCPublisherService, name=service_name ) - return mint_token(oidc_service, request) - + return mint_token(oidc_service, unverified_jwt, request) -def mint_token(oidc_service: OIDCPublisherService, request: Request) -> JsonResponse: - unverified_jwt: str - try: - payload = TokenPayload.model_validate_json(request.body) - unverified_jwt = payload.token - except ValidationError as exc: - return _invalid( - errors=[{"code": "invalid-payload", "description": str(exc)}], - request=request, - ) +def mint_token( + oidc_service: OIDCPublisherService, unverified_jwt: str, request: Request +) -> JsonResponse: claims = oidc_service.verify_jwt_signature(unverified_jwt) if not claims: return _invalid( From 29cf6a985436d0b99283b5b716d815751a210df2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 5 Jan 2024 17:24:26 -0500 Subject: [PATCH 07/10] tests: complete coverage Signed-off-by: William Woodruff --- tests/unit/oidc/test_utils.py | 18 ++++++++ tests/unit/oidc/test_views.py | 84 ++++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/tests/unit/oidc/test_utils.py b/tests/unit/oidc/test_utils.py index 1cd6a86a460d..8b8bc8af3e52 100644 --- a/tests/unit/oidc/test_utils.py +++ b/tests/unit/oidc/test_utils.py @@ -115,3 +115,21 @@ def test_oidc_context_principals(): Authenticated, "oidc:17", ] + + +def test_oidc_maps_consistent(): + # Our various mappings should have equivalent cardinalities. + assert len(utils.OIDC_ISSUER_URLS) == len(utils.OIDC_ISSUER_SERVICE_NAMES) + assert len(utils.OIDC_ISSUER_URLS) == len(utils.OIDC_ISSUER_ADMIN_FLAGS) + assert len(utils.OIDC_ISSUER_URLS) == len(utils.OIDC_PUBLISHER_CLASSES) + + for iss in utils.OIDC_ISSUER_URLS: + # Each issuer should be present in each mapping. + assert iss in utils.OIDC_ISSUER_SERVICE_NAMES + assert iss in utils.OIDC_ISSUER_ADMIN_FLAGS + assert iss in utils.OIDC_PUBLISHER_CLASSES + + for class_map in utils.OIDC_PUBLISHER_CLASSES.values(): + # The class mapping for pending and non-pending publisher models + # should be distinct. + assert class_map[True] != class_map[False] diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 8e963816a327..06c5f6bccd84 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -70,7 +70,7 @@ def test_oidc_audience(): assert response == {"audience": "fakeaudience"} -def test_mint_token_oidc_not_enabled(dummy_oidc_payload): +def test_mint_token_from_oidc_not_enabled(dummy_oidc_payload): request = pretend.stub( body=dummy_oidc_payload, response=pretend.stub(status=None), @@ -107,6 +107,15 @@ def test_mint_token_oidc_not_enabled(dummy_oidc_payload): {"token": [""]}, {"token": []}, {"token": {}}, + {"token": "not-a-jwt"}, + { + # Well-formed JWT, but no `iss` claim + "token": ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwib" + "mFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fw" + "pMeJf36POk6yJV_adQssw5c" + ) + }, ], ) def test_mint_token_from_oidc_invalid_payload(body): @@ -131,6 +140,79 @@ def body(self): assert isinstance(err["description"], str) +def test_mint_token_from_oidc_unknown_issuer(): + class Request: + def __init__(self): + self.response = pretend.stub(status=None) + self.flags = pretend.stub(disallow_oidc=lambda *a: False) + + @property + def body(self): + return json.dumps( + { + "token": ( + # iss: nonexistent-issuer + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJ" + "ub25leGlzdGVudC1pc3N1ZXIifQ.TYGmZaQXhjS3KA8o3POV" + "HeiD3FR5bz4X6UhRA4ykTFM" + ) + } + ) + + req = Request() + resp = views.mint_token_from_oidc(req) + + assert req.response.status == 422 + assert resp["message"] == "Token request failed" + assert isinstance(resp["errors"], list) + for err in resp["errors"]: + assert isinstance(err, dict) + assert err["code"] == "invalid-payload" + assert err["description"] == "unknown trusted publishing issuer" + + +@pytest.mark.parametrize( + ("token", "service_name"), + [ + ( + ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL3Rva2Vu" + "LmFjdGlvbnMuZ2l0aHVidXNlcmNvbnRlbnQuY29tIn0.saN7OFQBav8qXzgMCfERf" + "ZWPGfHu-0EEQMlVyO5UVdQ" + ), + "github", + ), + ( + ( + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL2FjY291b" + "nRzLmdvb2dsZS5jb20ifQ.2RJ6Y52Rap0LEj61yBGDokUg8r92SYQq6l3cflSWBVI" + ), + "google", + ), + ], +) +def test_mint_token_from_oidc_creates_expected_service( + monkeypatch, token, service_name +): + mint_token = pretend.call_recorder(lambda *a: pretend.stub()) + monkeypatch.setattr(views, "mint_token", mint_token) + + oidc_service = pretend.stub() + request = pretend.stub( + response=pretend.stub(status=None), + find_service=pretend.call_recorder(lambda cls, **kw: oidc_service), + flags=pretend.stub(disallow_oidc=lambda *a: False), + body=json.dumps({"token": token}), + ) + + views.mint_token_from_oidc(request) + + assert request.find_service.calls == [ + pretend.call(IOIDCPublisherService, name=service_name) + ] + assert mint_token.calls == [pretend.call(oidc_service, token, request)] + + def test_mint_token_from_trusted_publisher_verify_jwt_signature_fails(dummy_oidc_jwt): oidc_service = pretend.stub( verify_jwt_signature=pretend.call_recorder(lambda token: None), From e11aedb454104e25c32d493c7a7eeb034edf6ee9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 5 Jan 2024 17:34:40 -0500 Subject: [PATCH 08/10] tests, warehouse: eliminate impossible states Signed-off-by: William Woodruff --- tests/unit/oidc/test_views.py | 47 ----------------------------------- warehouse/oidc/views.py | 15 ++--------- 2 files changed, 2 insertions(+), 60 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 06c5f6bccd84..6f191006d10e 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -413,53 +413,6 @@ def test_mint_token_from_pending_trusted_publisher_invalidates_others( ] -def test_mint_token_from_oidc_only_pending_publisher_fail( - monkeypatch, db_request, dummy_oidc_jwt -): - pending_publisher = PendingGitHubPublisherFactory() - - def _find_publisher(claims, pending=False): - return pending_publisher - - oidc_service = pretend.stub( - verify_jwt_signature=pretend.call_recorder( - lambda token: {"ref": "someref", "sha": "somesha"} - ), - find_publisher=pretend.call_recorder(_find_publisher), - reify_pending_publisher=pretend.call_recorder( - lambda *a, **kw: pending_publisher - ), - ) - - ratelimiter = pretend.stub(clear=pretend.call_recorder(lambda id: None)) - ratelimiters = { - "user.oidc": ratelimiter, - "ip.oidc": ratelimiter, - } - monkeypatch.setattr(views, "_ratelimiters", lambda r: ratelimiters) - - send_pending_trusted_publisher_invalidated_email = pretend.call_recorder( - lambda *a, **kw: None - ) - monkeypatch.setattr( - services, - "send_pending_trusted_publisher_invalidated_email", - send_pending_trusted_publisher_invalidated_email, - ) - - response = views.mint_token(oidc_service, dummy_oidc_jwt, db_request) - - assert response == { - "message": "Token request failed", - "errors": [ - { - "code": "invalid-publisher", - "description": ("valid token, but no corresponding publisher"), - } - ], - } - - @pytest.mark.parametrize( ("claims_in_token", "claims_input"), [ diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 47d96f31c092..abfe9431e51f 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -219,6 +219,8 @@ def mint_token( # to actually do the macaroon minting with. try: publisher = oidc_service.find_publisher(claims, pending=False) + # NOTE: assert to persuade mypy of the correct type here. + assert isinstance(publisher, OIDCPublisher) except InvalidPublisherError as e: return _invalid( errors=[ @@ -230,19 +232,6 @@ def mint_token( request=request, ) - if not isinstance(publisher, OIDCPublisher): - # This should be impossible, but we have to perform this type check to - # appease mypy otherwise we get type errors in the code after this - # point. - return _invalid( - errors=[ - { - "code": "invalid-publisher", - "description": "valid token, but no corresponding publisher", - } - ], - request=request, - ) # At this point, we've verified that the given JWT is valid for the given # project. All we need to do is mint a new token. # NOTE: For OIDC-minted API tokens, the Macaroon's description string From 8c6933d1377e76a1e690464ad2720039b97c92c0 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 8 Jan 2024 12:52:56 -0500 Subject: [PATCH 09/10] tests, warehouse: feedback Signed-off-by: William Woodruff --- tests/unit/oidc/test_views.py | 2 +- warehouse/oidc/views.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 6f191006d10e..5c99dc0c5b06 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -84,7 +84,7 @@ def test_mint_token_from_oidc_not_enabled(dummy_oidc_payload): "errors": [ { "code": "not-enabled", - "description": ("Trusted publishing functionality not enabled"), + "description": "github trusted publishing functionality not enabled", } ], } diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index abfe9431e51f..c3a038bea102 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -94,14 +94,12 @@ def oidc_audience(request: Request): require_methods=["POST"], renderer="json", require_csrf=False, - has_translations=True, ) @view_config( route_name="oidc.mint_token", require_methods=["POST"], renderer="json", require_csrf=False, - has_translations=True, ) def mint_token_from_oidc(request: Request): try: @@ -146,7 +144,7 @@ def mint_token_from_oidc(request: Request): errors=[ { "code": "not-enabled", - "description": "Trusted publishing functionality not enabled", # noqa + "description": f"{service_name} trusted publishing functionality not enabled", # noqa } ], request=request, From c6a8d965ef9b367dfabf53d898cf0f3dbac6b604 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 9 Jan 2024 16:24:50 -0500 Subject: [PATCH 10/10] error specialization Signed-off-by: William Woodruff --- tests/unit/oidc/test_views.py | 69 ++++++++++++++++++++++++++++++++++- warehouse/oidc/views.py | 14 ++++++- 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 5c99dc0c5b06..578607df1efa 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -107,6 +107,33 @@ def test_mint_token_from_oidc_not_enabled(dummy_oidc_payload): {"token": [""]}, {"token": []}, {"token": {}}, + ], +) +def test_mint_token_from_oidc_invalid_payload(body): + class Request: + def __init__(self): + self.response = pretend.stub(status=None) + self.flags = pretend.stub(disallow_oidc=lambda *a: False) + + @property + def body(self): + return json.dumps(body) + + req = Request() + resp = views.mint_token_from_oidc(req) + + assert req.response.status == 422 + assert resp["message"] == "Token request failed" + assert isinstance(resp["errors"], list) + for err in resp["errors"]: + assert isinstance(err, dict) + assert err["code"] == "invalid-payload" + assert isinstance(err["description"], str) + + +@pytest.mark.parametrize( + "body", + [ {"token": "not-a-jwt"}, { # Well-formed JWT, but no `iss` claim @@ -118,7 +145,7 @@ def test_mint_token_from_oidc_not_enabled(dummy_oidc_payload): }, ], ) -def test_mint_token_from_oidc_invalid_payload(body): +def test_mint_token_from_oidc_invalid_payload_malformed_jwt(body): class Request: def __init__(self): self.response = pretend.stub(status=None) @@ -128,6 +155,9 @@ def __init__(self): def body(self): return json.dumps(body) + def find_service(self, *a, **kw): + return pretend.stub(increment=pretend.call_recorder(lambda s: None)) + req = Request() resp = views.mint_token_from_oidc(req) @@ -137,7 +167,42 @@ def body(self): for err in resp["errors"]: assert isinstance(err, dict) assert err["code"] == "invalid-payload" - assert isinstance(err["description"], str) + assert err["description"] == "malformed JWT" + + +def test_mint_token_from_oidc_jwt_decode_leaky_exception( + monkeypatch, dummy_oidc_payload +): + class Request: + def __init__(self): + self.response = pretend.stub(status=None) + self.flags = pretend.stub(disallow_oidc=lambda *a: False) + + @property + def body(self): + return dummy_oidc_payload + + def find_service(self, *a, **kw): + return pretend.stub(increment=pretend.call_recorder(lambda s: None)) + + capture_message = pretend.call_recorder(lambda s: None) + monkeypatch.setattr(views.sentry_sdk, "capture_message", capture_message) + monkeypatch.setattr(views.jwt, "decode", pretend.raiser(ValueError("oops"))) + + req = Request() + resp = views.mint_token_from_oidc(req) + + assert capture_message.calls == [ + pretend.call("jwt.decode raised generic error: oops") + ] + + assert req.response.status == 422 + assert resp["message"] == "Token request failed" + assert isinstance(resp["errors"], list) + for err in resp["errors"]: + assert isinstance(err, dict) + assert err["code"] == "invalid-payload" + assert err["description"] == "malformed JWT" def test_mint_token_from_oidc_unknown_issuer(): diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index c3a038bea102..f047868b5245 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -16,6 +16,7 @@ from typing import TypedDict import jwt +import sentry_sdk from pydantic import BaseModel, StrictStr, ValidationError from pyramid.request import Request @@ -26,6 +27,7 @@ from warehouse.macaroons import caveats from warehouse.macaroons.interfaces import IMacaroonService from warehouse.macaroons.services import DatabaseMacaroonService +from warehouse.metrics.interfaces import IMetricsService from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher @@ -120,7 +122,17 @@ def mint_token_from_oidc(request: Request): unverified_jwt, options=dict(verify_signature=False) ) unverified_issuer: str = unverified_claims["iss"] - except Exception: + except Exception as e: + metrics = request.find_service(IMetricsService, context=None) + metrics.increment("warehouse.oidc.mint_token_from_oidc.malformed_jwt") + + # We expect only PyJWTError and KeyError; anything else indicates + # an abstraction leak in jwt that we'll log for upstream reporting. + if not isinstance(e, (jwt.PyJWTError, KeyError)): + with sentry_sdk.push_scope() as scope: + scope.fingerprint = e + sentry_sdk.capture_message(f"jwt.decode raised generic error: {e}") + return _invalid( errors=[{"code": "invalid-payload", "description": "malformed JWT"}], request=request,