From 4b5327d523198b758f223626eda211f7093a39f3 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Wed, 9 Aug 2023 15:49:33 -0400 Subject: [PATCH] Add more detailed error messages to job_workflow_ref check (#14319) --- tests/unit/oidc/models/test_github.py | 90 +++++++++++++++++++++++---- warehouse/oidc/models/github.py | 12 +++- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index 3283b9698a30..8efb0f055874 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/tests/unit/oidc/models/test_github.py @@ -236,54 +236,115 @@ def test_github_publisher_verifies(self, monkeypatch, environment, missing_claim ) @pytest.mark.parametrize( - ("claim", "ref", "valid"), + ("claim", "ref", "valid", "expected"), [ # okay: workflow name, followed by a nonempty ref ( "foo/bar/.github/workflows/baz.yml@refs/tags/v0.0.1", "refs/tags/v0.0.1", True, + None, + ), + ( + "foo/bar/.github/workflows/baz.yml@refs/pulls/6", + "refs/pulls/6", + True, + None, ), - ("foo/bar/.github/workflows/baz.yml@refs/pulls/6", "refs/pulls/6", True), ( "foo/bar/.github/workflows/baz.yml@refs/heads/main", "refs/heads/main", True, + None, ), ( "foo/bar/.github/workflows/baz.yml@notrailingslash", "notrailingslash", True, + None, ), # bad: workflow name, empty or missing ref - ("foo/bar/.github/workflows/baz.yml@emptyref", "", False), - ("foo/bar/.github/workflows/baz.yml@missingref", None, False), + ( + "foo/bar/.github/workflows/baz.yml@emptyref", + "", + False, + "The ref claim is empty", + ), + ( + "foo/bar/.github/workflows/baz.yml@missingref", + None, + False, + "The ref claim is empty", + ), # bad: workflow name with various attempted impersonations ( "foo/bar/.github/workflows/baz.yml@fake.yml@notrailingslash", "notrailingslash", False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/baz.yml@fake.yml@notrailingslash'", ), ( "foo/bar/.github/workflows/baz.yml@fake.yml@refs/pulls/6", "refs/pulls/6", False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@refs/pulls/6', got " + "'foo/bar/.github/workflows/baz.yml@fake.yml@refs/pulls/6'", ), # bad: missing tail or workflow name or otherwise partial - ("foo/bar/.github/workflows/baz.yml@", "notrailingslash", False), - ("foo/bar/.github/workflows/@", "notrailingslash", False), - ("foo/bar/.github/workflows/", "notrailingslash", False), - ("baz.yml", "notrailingslash", False), + ( + "foo/bar/.github/workflows/baz.yml@", + "notrailingslash", + False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/baz.yml@'", + ), + ( + "foo/bar/.github/workflows/@", + "notrailingslash", + False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/@'", + ), + ( + "foo/bar/.github/workflows/", + "notrailingslash", + False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/'", + ), + ( + "baz.yml", + "notrailingslash", + False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got 'baz.yml'", + ), ( "foo/bar/.github/workflows/baz.yml@malicious.yml@", "notrailingslash", False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/baz.yml@malicious.yml@'", + ), + ( + "foo/bar/.github/workflows/baz.yml@@", + "notrailingslash", + False, + "The claim does not match, expecting " + "'foo/bar/.github/workflows/baz.yml@notrailingslash', got " + "'foo/bar/.github/workflows/baz.yml@@'", ), - ("foo/bar/.github/workflows/baz.yml@@", "notrailingslash", False), - ("", "notrailingslash", False), + ("", "notrailingslash", False, "The job_workflow_ref claim is empty"), ], ) - def test_github_publisher_job_workflow_ref(self, claim, ref, valid): + def test_github_publisher_job_workflow_ref(self, claim, ref, valid, expected): publisher = github.GitHubPublisher( repository_name="bar", repository_owner="foo", @@ -294,7 +355,12 @@ def test_github_publisher_job_workflow_ref(self, claim, ref, valid): check = github.GitHubPublisher.__required_verifiable_claims__[ "job_workflow_ref" ] - assert check(publisher.job_workflow_ref, claim, {"ref": ref}) is valid + if valid: + assert check(publisher.job_workflow_ref, claim, {"ref": ref}) is True + else: + with pytest.raises(errors.InvalidPublisherError) as e: + check(publisher.job_workflow_ref, claim, {"ref": ref}) is True + assert str(e.value) == expected @pytest.mark.parametrize( ("truth", "claim", "valid"), diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index 4ed368d1e62e..21fcdc9b3657 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -15,6 +15,7 @@ from sqlalchemy.orm import Query, mapped_column from sqlalchemy.sql.expression import func, literal +from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import SignedClaims from warehouse.oidc.models._core import ( OIDCPublisher, @@ -31,13 +32,18 @@ def _check_job_workflow_ref(ground_truth, signed_claim, all_signed_claims): # Defensive: GitHub should never give us an empty job_workflow_ref, # but we check for one anyways just in case. if not signed_claim: - return False + raise InvalidPublisherError("The job_workflow_ref claim is empty") ref = all_signed_claims.get("ref") if not ref: - return False + raise InvalidPublisherError("The ref claim is empty") + + if not (expected := f"{ground_truth}@{ref}") == signed_claim: + raise InvalidPublisherError( + f"The claim does not match, expecting {expected!r}, got {signed_claim!r}" + ) - return f"{ground_truth}@{ref}" == signed_claim + return True def _check_environment(ground_truth, signed_claim, all_signed_claims):