From 747cbb290b57e6fc71c7361958ea5eb4cb08c48a Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 18 Jun 2024 17:43:32 +0200 Subject: [PATCH 01/15] Update documentation --- docs/user/trusted-publishers/using-a-publisher.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/user/trusted-publishers/using-a-publisher.md b/docs/user/trusted-publishers/using-a-publisher.md index e5679e97ab39..36f4afe63c24 100644 --- a/docs/user/trusted-publishers/using-a-publisher.md +++ b/docs/user/trusted-publishers/using-a-publisher.md @@ -108,8 +108,8 @@ below describe the setup process for each supported trusted publisher. 2. Submit that token to PyPI, which will return a short-lived API key; 3. Use that API key as you normally would (e.g. with `twine`) - GitHub is currently the only OIDC identity provider supported, so we'll use it - for examples below. + While the examples below demonstrate using GitHub as an identity providers, + this process is also compatible with the other identity providers. All code below assumes that it's being run in a GitHub Actions workflow runner with `id-token: write` permissions. That permission is From bccf3b81710ed4ba18532cfbf62d383f3505993d Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 18 Jun 2024 17:55:35 +0200 Subject: [PATCH 02/15] Remove outdated sentence --- docs/user/trusted-publishers/using-a-publisher.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/docs/user/trusted-publishers/using-a-publisher.md b/docs/user/trusted-publishers/using-a-publisher.md index 36f4afe63c24..7d5e09ab59bd 100644 --- a/docs/user/trusted-publishers/using-a-publisher.md +++ b/docs/user/trusted-publishers/using-a-publisher.md @@ -108,9 +108,6 @@ below describe the setup process for each supported trusted publisher. 2. Submit that token to PyPI, which will return a short-lived API key; 3. Use that API key as you normally would (e.g. with `twine`) - While the examples below demonstrate using GitHub as an identity providers, - this process is also compatible with the other identity providers. - All code below assumes that it's being run in a GitHub Actions workflow runner with `id-token: write` permissions. That permission is **critical**; without it, GitHub Actions will refuse to give you an OIDC token. From 905b10bf912b5ad5070519cfc56e085acb1cf2e8 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 9 Jul 2024 17:28:39 +0200 Subject: [PATCH 03/15] Create OIDCJtiTokens table --- tests/unit/oidc/test_tasks.py | 30 +++++++- .../4c6907394e44_add_jwt_expiration_table.py | 69 +++++++++++++++++++ warehouse/oidc/models/__init__.py | 3 +- warehouse/oidc/models/_core.py | 15 +++- warehouse/oidc/tasks.py | 22 +++++- 5 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py diff --git a/tests/unit/oidc/test_tasks.py b/tests/unit/oidc/test_tasks.py index fdcb927c0ba9..f415ad018824 100644 --- a/tests/unit/oidc/test_tasks.py +++ b/tests/unit/oidc/test_tasks.py @@ -16,7 +16,8 @@ from warehouse.macaroons import caveats from warehouse.macaroons.models import Macaroon -from warehouse.oidc.tasks import compute_oidc_metrics, delete_expired_oidc_macaroons +from warehouse.oidc.models import OIDCJtiTokens +from warehouse.oidc.tasks import compute_oidc_metrics, delete_expired_oidc_macaroons, delete_expired_jwt_tokens from ...common.db.oidc import GitHubPublisherFactory from ...common.db.packaging import ( @@ -100,6 +101,33 @@ def test_compute_oidc_metrics(db_request, metrics): ] +def test_delete_expired_jwt_tokens(db_request, metrics): + # We create 4 tokens : + # With two different providers + # With two different types of expiration date (before and after the current time) + + now = datetime.datetime.now(tz=datetime.timezone.utc) + jwt_tokens = [ + OIDCJtiTokens(oidc_provider_name="some-provider", jwt_token_identifier="some-expired-token", expiration=now - datetime.timedelta(days=1)), + OIDCJtiTokens(oidc_provider_name="some-provider", jwt_token_identifier="some-valid-token", expiration=now + datetime.timedelta(hours=1)), + OIDCJtiTokens(oidc_provider_name="other-provider", jwt_token_identifier="some-expired-token-other", expiration=now - datetime.timedelta(hours=1)), + OIDCJtiTokens(oidc_provider_name="other-provider", jwt_token_identifier="some-valid-token-other", expiration=now + datetime.timedelta(days=1)), + ] + db_request.db.add_all(jwt_tokens) + + assert db_request.db.query(OIDCJtiTokens).count() == 4 + + delete_expired_jwt_tokens(db_request) + + remaining_tokens = db_request.db.query(OIDCJtiTokens).all() + assert len(remaining_tokens) == 2 + assert not set(token.jwt_token_identifier for token in remaining_tokens) ^ {"some-valid-token", "some-valid-token-other"} + + assert metrics.gauge.calls == [ + pretend.call("warehouse.oidc.expired_jwt_deleted", 2), + ] + + def test_delete_expired_oidc_macaroons(db_request, macaroon_service, metrics): # We'll create 4 macaroons: # - An OIDC macaroon with creation time of 1 day ago diff --git a/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py b/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py new file mode 100644 index 000000000000..fa3e01bf6dc8 --- /dev/null +++ b/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py @@ -0,0 +1,69 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +add jwt expiration table + +Revision ID: 4c6907394e44 +Revises: b14df478c48f +Create Date: 2024-07-09 14:50:48.832866 +""" + +import sqlalchemy as sa + +from alembic import op + +revision = "4c6907394e44" +down_revision = "b14df478c48f" + +# Note: It is VERY important to ensure that a migration does not lock for a +# long period of time and to ensure that each individual migration does +# not break compatibility with the *previous* version of the code base. +# This is because the migrations will be ran automatically as part of the +# deployment process, but while the previous version of the code is still +# up and running. Thus backwards incompatible changes must be broken up +# over multiple migrations inside of multiple pull requests in order to +# phase them in over multiple deploys. +# +# By default, migrations cannot wait more than 4s on acquiring a lock +# and each individual statement cannot take more than 5s. This helps +# prevent situations where a slow migration takes the entire site down. +# +# If you need to increase this timeout for a migration, you can do so +# by adding: +# +# op.execute("SET statement_timeout = 5000") +# op.execute("SET lock_timeout = 4000") +# +# To whatever values are reasonable for this migration as part of your +# migration. + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + "oidc_jti_tokens", + sa.Column("oidc_provider_name", sa.String(length=128), nullable=False), + sa.Column("jwt_token_identifier", sa.String(length=128), nullable=False), + sa.Column("expiration", sa.DateTime(), nullable=False), + sa.Column( + "id", sa.UUID(), server_default=sa.text("gen_random_uuid()"), nullable=False + ), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("jwt_token_identifier", name="jwt_token_identifier_key"), + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table("oidc_jti_tokens") + # ### end Alembic commands ### diff --git a/warehouse/oidc/models/__init__.py b/warehouse/oidc/models/__init__.py index a125c451a58e..0d07a7997424 100644 --- a/warehouse/oidc/models/__init__.py +++ b/warehouse/oidc/models/__init__.py @@ -10,7 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from warehouse.oidc.models._core import OIDCPublisher, PendingOIDCPublisher +from warehouse.oidc.models._core import OIDCPublisher, PendingOIDCPublisher, OIDCJtiTokens from warehouse.oidc.models.activestate import ( ActiveStatePublisher, PendingActiveStatePublisher, @@ -20,6 +20,7 @@ from warehouse.oidc.models.google import GooglePublisher, PendingGooglePublisher __all__ = [ + "OIDCJtiTokens", "OIDCPublisher", "PendingOIDCPublisher", "PendingGitHubPublisher", diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 336dddf130b9..ec5c7056f5ce 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -12,12 +12,13 @@ from __future__ import annotations +import datetime from collections.abc import Callable from typing import TYPE_CHECKING, Any, TypeVar import sentry_sdk -from sqlalchemy import ForeignKey, String, orm +from sqlalchemy import ForeignKey, String, orm, UniqueConstraint from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.orm import Mapped, mapped_column @@ -81,6 +82,18 @@ class OIDCPublisherProjectAssociation(db.Model): ) +class OIDCJtiTokens(db.Model): + __tablename__ = "oidc_jti_tokens" + + __table_args__ = ( + UniqueConstraint("jwt_token_identifier", name="jwt_token_identifier_key"), + ) + + oidc_provider_name: Mapped[str] = mapped_column(String(length=128)) + jwt_token_identifier: Mapped[str] = mapped_column(String(length=128)) + expiration: Mapped[datetime.datetime] + + class OIDCPublisherMixin: """ A mixin for common functionality between all OIDC publishers, including diff --git a/warehouse/oidc/tasks.py b/warehouse/oidc/tasks.py index d5ba98976516..fccc24764d9f 100644 --- a/warehouse/oidc/tasks.py +++ b/warehouse/oidc/tasks.py @@ -15,7 +15,7 @@ from warehouse import tasks from warehouse.macaroons.models import Macaroon from warehouse.metrics import IMetricsService -from warehouse.oidc.models import OIDCPublisher +from warehouse.oidc.models import OIDCPublisher, OIDCJtiTokens from warehouse.packaging.models import File, Project, Release @@ -93,3 +93,23 @@ def delete_expired_oidc_macaroons(request): "warehouse.oidc.expired_oidc_tokens_deleted", rows_deleted, ) + + +@tasks.task(ignore_result=True, acks_late=True) +def delete_expired_jwt_tokens(request): + """ + Purge all JWT Token Identifier tokens that have been stored that are expired. + While OIDC tokens are short-lived, storing them prevent to generate multiple + OIDC-minted macaroons with a single JWT. + """ + rows_deleted = ( + request.db.query(OIDCJtiTokens) + .filter(OIDCJtiTokens.expiration < datetime.now(tz=timezone.utc)) + .delete(synchronize_session=False) + ) + + metrics = request.find_service(IMetricsService, context=None) + metrics.gauge( + "warehouse.oidc.expired_jwt_deleted", + rows_deleted, + ) From 9841636d48cddf0475a025927e27571c892752ab Mon Sep 17 00:00:00 2001 From: Alexis Date: Wed, 10 Jul 2024 18:21:24 +0200 Subject: [PATCH 04/15] Implement JWT token reuse detection --- tests/unit/oidc/models/test_github.py | 5 +- tests/unit/oidc/models/test_gitlab.py | 5 +- tests/unit/oidc/test_services.py | 111 +++++++++++++++++++++++++- tests/unit/oidc/test_views.py | 33 ++++++++ warehouse/oidc/errors.py | 4 + warehouse/oidc/models/github.py | 4 +- warehouse/oidc/models/gitlab.py | 3 +- warehouse/oidc/services.py | 34 +++++++- warehouse/oidc/views.py | 12 ++- 9 files changed, 198 insertions(+), 13 deletions(-) diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index a0bf7826dfec..1f0d2d3bb82a 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/tests/unit/oidc/models/test_github.py @@ -51,6 +51,7 @@ def test_github_publisher_all_known_claims(self): # required unverifiable claims "ref", "sha", + "jti", # optional verifiable claims "environment", # preverified claims @@ -62,7 +63,6 @@ def test_github_publisher_all_known_claims(self): # unchecked claims "actor", "actor_id", - "jti", "run_id", "run_number", "run_attempt", @@ -143,7 +143,7 @@ def test_github_publisher_unaccounted_claims(self, monkeypatch): ] assert scope.fingerprint == ["another-fake-claim", "fake-claim"] - @pytest.mark.parametrize("missing", ["sub", "ref"]) + @pytest.mark.parametrize("missing", ["sub", "ref", "jti"]) def test_github_publisher_missing_claims(self, monkeypatch, missing): publisher = github.GitHubPublisher( repository_name="fakerepo", @@ -198,6 +198,7 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch): signed_claims["ref"] = "ref" signed_claims["sha"] = "sha" signed_claims["job_workflow_ref"] = publisher.job_workflow_ref + "@ref" + signed_claims["jti"] = "jti" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: publisher.verify_claims(signed_claims=signed_claims) diff --git a/tests/unit/oidc/models/test_gitlab.py b/tests/unit/oidc/models/test_gitlab.py index 68a47969717b..7c8f5473e5af 100644 --- a/tests/unit/oidc/models/test_gitlab.py +++ b/tests/unit/oidc/models/test_gitlab.py @@ -49,6 +49,7 @@ def test_gitlab_publisher_all_known_claims(self): # required unverifiable claims "ref_path", "sha", + "jti", # optional verifiable claims "environment", # preverified claims @@ -78,7 +79,6 @@ def test_gitlab_publisher_all_known_claims(self): "runner_environment", "ci_config_sha", "project_visibility", - "jti", "user_access_level", "groups_direct", } @@ -141,7 +141,7 @@ def test_gitlab_publisher_unaccounted_claims(self, monkeypatch): ] assert scope.fingerprint == ["another-fake-claim", "fake-claim"] - @pytest.mark.parametrize("missing", ["sub", "ref_path"]) + @pytest.mark.parametrize("missing", ["sub", "ref_path", "jti"]) def test_gitlab_publisher_missing_claims(self, monkeypatch, missing): publisher = gitlab.GitLabPublisher( project="fakerepo", @@ -194,6 +194,7 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch): signed_claims["ref_path"] = "ref" signed_claims["sha"] = "sha" signed_claims["ci_config_ref_uri"] = publisher.ci_config_ref_uri + "@ref" + signed_claims["jti"] = "jti" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: publisher.verify_claims(signed_claims=signed_claims) diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 9b03cd94a1cf..e1f393f5e82a 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -9,6 +9,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import datetime import jwt import pretend @@ -22,6 +23,7 @@ from tests.common.db.oidc import GitHubPublisherFactory, PendingGitHubPublisherFactory from warehouse.oidc import errors, interfaces, services +from warehouse.oidc.interfaces import SignedClaims def test_oidc_publisher_service_factory(metrics): @@ -193,7 +195,7 @@ def test_find_publisher(self, metrics, monkeypatch): metrics=metrics, ) - token = pretend.stub() + token = SignedClaims({}) publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) @@ -262,7 +264,7 @@ def test_find_publisher_verify_claims_fails(self, metrics, monkeypatch): services, "find_publisher_by_issuer", find_publisher_by_issuer ) - claims = pretend.stub() + claims = SignedClaims({}) with pytest.raises(errors.InvalidPublisherError): service.find_publisher(claims) assert service.metrics.increment.calls == [ @@ -277,6 +279,110 @@ def test_find_publisher_verify_claims_fails(self, metrics, monkeypatch): ] assert publisher.verify_claims.calls == [pretend.call(claims)] + def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metrics): + service = services.OIDCPublisherService( + session=pretend.stub(), + publisher="fakepublisher", + issuer_url=pretend.stub(), + audience="fakeaudience", + cache_url="redis://fake.example.com", + metrics=metrics, + ) + + monkeypatch.setattr(services.redis, "StrictRedis", mockredis) + + publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) + monkeypatch.setattr( + services, "find_publisher_by_issuer", find_publisher_by_issuer + ) + + expiration = int( + (datetime.datetime.now(tz=datetime.timezone.utc) + datetime.timedelta(minutes=15)).timestamp() + ) + jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) + + claims = SignedClaims({ + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": expiration, + "aud": "pypi", + "jti": jwt_token_identifier, + }) + + with pytest.raises(errors.ReusedTokenError): + service.find_publisher(claims, pending=False) + + assert pretend.call( + "warehouse.oidc.reused_token", tags=["publisher:fakepublisher"] + ) in metrics.increment.calls + + def test_find_publisher_store_jti(self, monkeypatch, mockredis, metrics): + service = services.OIDCPublisherService( + session=pretend.stub(), + publisher="fakepublisher", + issuer_url=pretend.stub(), + audience="fakeaudience", + cache_url="redis://fake.example.com", + metrics=metrics, + ) + + monkeypatch.setattr(services.redis, "StrictRedis", mockredis) + + publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) + monkeypatch.setattr( + services, "find_publisher_by_issuer", find_publisher_by_issuer + ) + + expiration = int( + (datetime.datetime.now(tz=datetime.timezone.utc) + datetime.timedelta(minutes=15)).timestamp() + ) + jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + claims = SignedClaims({ + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": expiration, + "aud": "pypi", + "jti": jwt_token_identifier, + }) + + service.find_publisher(claims, pending=False) + assert service.token_identifier_exists(jwt_token_identifier) is True + + def test_find_publisher_jti_not_stored_if_pending(self, monkeypatch, mockredis, metrics): + service = services.OIDCPublisherService( + session=pretend.stub(), + publisher="fakepublisher", + issuer_url=pretend.stub(), + audience="fakeaudience", + cache_url="redis://fake.example.com", + metrics=metrics, + ) + + monkeypatch.setattr(services.redis, "StrictRedis", mockredis) + + publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) + monkeypatch.setattr( + services, "find_publisher_by_issuer", find_publisher_by_issuer + ) + jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + claims = SignedClaims({ + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": int(datetime.datetime.now(tz=datetime.timezone.utc).timestamp()), + "aud": "pypi", + "jti": jwt_token_identifier, + }) + + service.find_publisher(claims, pending=True) + assert service.token_identifier_exists(jwt_token_identifier) is False + def test_get_keyset_not_cached(self, monkeypatch, mockredis): service = services.OIDCPublisherService( session=pretend.stub(), @@ -829,6 +935,7 @@ def test_find_publisher(self, monkeypatch): "nbf": 1516239022, "exp": 9999999999, "aud": "pypi", + "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" } service = services.NullOIDCPublisherService( diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 2878e138f8a9..0637363d17f9 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -361,6 +361,39 @@ def test_mint_token_trusted_publisher_lookup_fails(dummy_github_oidc_jwt): ] +def test_mint_token_duplicate_token(dummy_github_oidc_jwt): + def find_publishers_mockup(_, pending: bool = False): + if pending is False: + raise errors.ReusedTokenError("some message") + else: + raise errors.InvalidPublisherError("some message") + + + claims = pretend.stub() + oidc_service = pretend.stub( + verify_jwt_signature=pretend.call_recorder(lambda token: claims), + find_publisher=find_publishers_mockup, + ) + 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), + ) + + response = views.mint_token(oidc_service, dummy_github_oidc_jwt, request) + assert request.response.status == 422 + assert response == { + "message": "Token request failed", + "errors": [ + { + "code": "invalid-reuse-token", + "description": "valid token, but already used", + } + ], + } + + + def test_mint_token_pending_publisher_project_already_exists( db_request, dummy_github_oidc_jwt ): diff --git a/warehouse/oidc/errors.py b/warehouse/oidc/errors.py index 30ea940ba852..3f831a7f2f89 100644 --- a/warehouse/oidc/errors.py +++ b/warehouse/oidc/errors.py @@ -13,3 +13,7 @@ class InvalidPublisherError(Exception): pass + + +class ReusedTokenError(Exception): + pass diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index 6223131066fc..f7e6c3c0a33d 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -132,7 +132,7 @@ class GitHubPublisherMixin: "job_workflow_ref": _check_job_workflow_ref, } - __required_unverifiable_claims__: set[str] = {"ref", "sha"} + __required_unverifiable_claims__: set[str] = {"ref", "sha", "jti"} __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { "environment": _check_environment, @@ -141,7 +141,6 @@ class GitHubPublisherMixin: __unchecked_claims__ = { "actor", "actor_id", - "jti", "run_id", "run_number", "run_attempt", @@ -161,7 +160,6 @@ class GitHubPublisherMixin: "enterprise_id", "ref_protected", } - @staticmethod def __lookup_all__(klass, signed_claims: SignedClaims) -> Query | None: # This lookup requires the environment claim to be present; diff --git a/warehouse/oidc/models/gitlab.py b/warehouse/oidc/models/gitlab.py index 54039e6bf54e..e16715343e67 100644 --- a/warehouse/oidc/models/gitlab.py +++ b/warehouse/oidc/models/gitlab.py @@ -120,7 +120,7 @@ class GitLabPublisherMixin: "ci_config_ref_uri": _check_ci_config_ref_uri, } - __required_unverifiable_claims__: set[str] = {"ref_path", "sha"} + __required_unverifiable_claims__: set[str] = {"ref_path", "sha", "jti"} __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { "environment": _check_environment, @@ -149,7 +149,6 @@ class GitLabPublisherMixin: "runner_environment", "ci_config_sha", "project_visibility", - "jti", "user_access_level", "groups_direct", } diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index ec522fe77335..a2b4526027c7 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -21,7 +21,7 @@ from zope.interface import implementer from warehouse.metrics.interfaces import IMetricsService -from warehouse.oidc.errors import InvalidPublisherError +from warehouse.oidc.errors import InvalidPublisherError, ReusedTokenError from warehouse.oidc.interfaces import IOIDCPublisherService, SignedClaims from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.utils import find_publisher_by_issuer @@ -219,6 +219,21 @@ def _get_key_for_token(self, token): unverified_header = jwt.get_unverified_header(token) return self._get_key(unverified_header["kid"]) + def token_identifier_exists(self, jti: str) -> bool: + """ + Check if a JWT Token Identifier has already been used. + """ + with redis.StrictRedis.from_url(self.cache_url) as r: + return bool(r.exists(jti)) + + def store_jwt_identifier(self, jti: str, expiration: int) -> None: + """ + Store the JTI with its expiration date if the key does not exist. + """ + with redis.StrictRedis.from_url(self.cache_url) as r: + r.set(jti, exat=expiration, value="placeholder", nx=True) + + def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: try: key = self._get_key_for_token(unverified_token) @@ -294,11 +309,28 @@ def find_publisher( publisher = find_publisher_by_issuer( self.db, self.issuer_url, signed_claims, pending=pending ) + + jwt_token_identifier: str | None = signed_claims.get("jti", None) + # It is ok to not perform the reused token check here if we don't have a JTI + # because it is going to be checked in `verify_claims` function if the claim + # was required and fail with the appropriate error. + if pending is False and jwt_token_identifier: + if self.token_identifier_exists(jwt_token_identifier): + self.metrics.increment( + "warehouse.oidc.reused_token", + tags=metrics_tags, + ) + raise ReusedTokenError("JWT Token already used to mint a token.") + publisher.verify_claims(signed_claims) self.metrics.increment( "warehouse.oidc.find_publisher.ok", tags=metrics_tags, ) + + if pending is False and jwt_token_identifier: + self.store_jwt_identifier(jwt_token_identifier, signed_claims.get("exp")) + return publisher except InvalidPublisherError as e: self.metrics.increment( diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 341a07657f2c..dc8f5d67c28c 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -28,7 +28,7 @@ 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.errors import InvalidPublisherError, ReusedTokenError from warehouse.oidc.interfaces import IOIDCPublisherService from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.services import OIDCPublisherService @@ -238,6 +238,16 @@ def mint_token( publisher = oidc_service.find_publisher(claims, pending=False) # NOTE: assert to persuade mypy of the correct type here. assert isinstance(publisher, OIDCPublisher) + except ReusedTokenError: + return _invalid( + errors=[ + { + "code": "invalid-reuse-token", + "description": "valid token, but already used", + } + ], + request=request + ) except InvalidPublisherError as e: return _invalid( errors=[ From 00837af445cabe6c6cb732a87ad737f78efc4bd5 Mon Sep 17 00:00:00 2001 From: Alexis Date: Wed, 10 Jul 2024 18:22:23 +0200 Subject: [PATCH 05/15] Revert "Create OIDCJtiTokens table" This reverts commit 905b10bf912b5ad5070519cfc56e085acb1cf2e8. --- tests/unit/oidc/test_tasks.py | 30 +------- .../4c6907394e44_add_jwt_expiration_table.py | 69 ------------------- warehouse/oidc/models/__init__.py | 3 +- warehouse/oidc/models/_core.py | 15 +--- warehouse/oidc/tasks.py | 22 +----- 5 files changed, 4 insertions(+), 135 deletions(-) delete mode 100644 warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py diff --git a/tests/unit/oidc/test_tasks.py b/tests/unit/oidc/test_tasks.py index f415ad018824..fdcb927c0ba9 100644 --- a/tests/unit/oidc/test_tasks.py +++ b/tests/unit/oidc/test_tasks.py @@ -16,8 +16,7 @@ from warehouse.macaroons import caveats from warehouse.macaroons.models import Macaroon -from warehouse.oidc.models import OIDCJtiTokens -from warehouse.oidc.tasks import compute_oidc_metrics, delete_expired_oidc_macaroons, delete_expired_jwt_tokens +from warehouse.oidc.tasks import compute_oidc_metrics, delete_expired_oidc_macaroons from ...common.db.oidc import GitHubPublisherFactory from ...common.db.packaging import ( @@ -101,33 +100,6 @@ def test_compute_oidc_metrics(db_request, metrics): ] -def test_delete_expired_jwt_tokens(db_request, metrics): - # We create 4 tokens : - # With two different providers - # With two different types of expiration date (before and after the current time) - - now = datetime.datetime.now(tz=datetime.timezone.utc) - jwt_tokens = [ - OIDCJtiTokens(oidc_provider_name="some-provider", jwt_token_identifier="some-expired-token", expiration=now - datetime.timedelta(days=1)), - OIDCJtiTokens(oidc_provider_name="some-provider", jwt_token_identifier="some-valid-token", expiration=now + datetime.timedelta(hours=1)), - OIDCJtiTokens(oidc_provider_name="other-provider", jwt_token_identifier="some-expired-token-other", expiration=now - datetime.timedelta(hours=1)), - OIDCJtiTokens(oidc_provider_name="other-provider", jwt_token_identifier="some-valid-token-other", expiration=now + datetime.timedelta(days=1)), - ] - db_request.db.add_all(jwt_tokens) - - assert db_request.db.query(OIDCJtiTokens).count() == 4 - - delete_expired_jwt_tokens(db_request) - - remaining_tokens = db_request.db.query(OIDCJtiTokens).all() - assert len(remaining_tokens) == 2 - assert not set(token.jwt_token_identifier for token in remaining_tokens) ^ {"some-valid-token", "some-valid-token-other"} - - assert metrics.gauge.calls == [ - pretend.call("warehouse.oidc.expired_jwt_deleted", 2), - ] - - def test_delete_expired_oidc_macaroons(db_request, macaroon_service, metrics): # We'll create 4 macaroons: # - An OIDC macaroon with creation time of 1 day ago diff --git a/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py b/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py deleted file mode 100644 index fa3e01bf6dc8..000000000000 --- a/warehouse/migrations/versions/4c6907394e44_add_jwt_expiration_table.py +++ /dev/null @@ -1,69 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -""" -add jwt expiration table - -Revision ID: 4c6907394e44 -Revises: b14df478c48f -Create Date: 2024-07-09 14:50:48.832866 -""" - -import sqlalchemy as sa - -from alembic import op - -revision = "4c6907394e44" -down_revision = "b14df478c48f" - -# Note: It is VERY important to ensure that a migration does not lock for a -# long period of time and to ensure that each individual migration does -# not break compatibility with the *previous* version of the code base. -# This is because the migrations will be ran automatically as part of the -# deployment process, but while the previous version of the code is still -# up and running. Thus backwards incompatible changes must be broken up -# over multiple migrations inside of multiple pull requests in order to -# phase them in over multiple deploys. -# -# By default, migrations cannot wait more than 4s on acquiring a lock -# and each individual statement cannot take more than 5s. This helps -# prevent situations where a slow migration takes the entire site down. -# -# If you need to increase this timeout for a migration, you can do so -# by adding: -# -# op.execute("SET statement_timeout = 5000") -# op.execute("SET lock_timeout = 4000") -# -# To whatever values are reasonable for this migration as part of your -# migration. - - -def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.create_table( - "oidc_jti_tokens", - sa.Column("oidc_provider_name", sa.String(length=128), nullable=False), - sa.Column("jwt_token_identifier", sa.String(length=128), nullable=False), - sa.Column("expiration", sa.DateTime(), nullable=False), - sa.Column( - "id", sa.UUID(), server_default=sa.text("gen_random_uuid()"), nullable=False - ), - sa.PrimaryKeyConstraint("id"), - sa.UniqueConstraint("jwt_token_identifier", name="jwt_token_identifier_key"), - ) - # ### end Alembic commands ### - - -def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### - op.drop_table("oidc_jti_tokens") - # ### end Alembic commands ### diff --git a/warehouse/oidc/models/__init__.py b/warehouse/oidc/models/__init__.py index 0d07a7997424..a125c451a58e 100644 --- a/warehouse/oidc/models/__init__.py +++ b/warehouse/oidc/models/__init__.py @@ -10,7 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from warehouse.oidc.models._core import OIDCPublisher, PendingOIDCPublisher, OIDCJtiTokens +from warehouse.oidc.models._core import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.models.activestate import ( ActiveStatePublisher, PendingActiveStatePublisher, @@ -20,7 +20,6 @@ from warehouse.oidc.models.google import GooglePublisher, PendingGooglePublisher __all__ = [ - "OIDCJtiTokens", "OIDCPublisher", "PendingOIDCPublisher", "PendingGitHubPublisher", diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index ec5c7056f5ce..336dddf130b9 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -12,13 +12,12 @@ from __future__ import annotations -import datetime from collections.abc import Callable from typing import TYPE_CHECKING, Any, TypeVar import sentry_sdk -from sqlalchemy import ForeignKey, String, orm, UniqueConstraint +from sqlalchemy import ForeignKey, String, orm from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.orm import Mapped, mapped_column @@ -82,18 +81,6 @@ class OIDCPublisherProjectAssociation(db.Model): ) -class OIDCJtiTokens(db.Model): - __tablename__ = "oidc_jti_tokens" - - __table_args__ = ( - UniqueConstraint("jwt_token_identifier", name="jwt_token_identifier_key"), - ) - - oidc_provider_name: Mapped[str] = mapped_column(String(length=128)) - jwt_token_identifier: Mapped[str] = mapped_column(String(length=128)) - expiration: Mapped[datetime.datetime] - - class OIDCPublisherMixin: """ A mixin for common functionality between all OIDC publishers, including diff --git a/warehouse/oidc/tasks.py b/warehouse/oidc/tasks.py index fccc24764d9f..d5ba98976516 100644 --- a/warehouse/oidc/tasks.py +++ b/warehouse/oidc/tasks.py @@ -15,7 +15,7 @@ from warehouse import tasks from warehouse.macaroons.models import Macaroon from warehouse.metrics import IMetricsService -from warehouse.oidc.models import OIDCPublisher, OIDCJtiTokens +from warehouse.oidc.models import OIDCPublisher from warehouse.packaging.models import File, Project, Release @@ -93,23 +93,3 @@ def delete_expired_oidc_macaroons(request): "warehouse.oidc.expired_oidc_tokens_deleted", rows_deleted, ) - - -@tasks.task(ignore_result=True, acks_late=True) -def delete_expired_jwt_tokens(request): - """ - Purge all JWT Token Identifier tokens that have been stored that are expired. - While OIDC tokens are short-lived, storing them prevent to generate multiple - OIDC-minted macaroons with a single JWT. - """ - rows_deleted = ( - request.db.query(OIDCJtiTokens) - .filter(OIDCJtiTokens.expiration < datetime.now(tz=timezone.utc)) - .delete(synchronize_session=False) - ) - - metrics = request.find_service(IMetricsService, context=None) - metrics.gauge( - "warehouse.oidc.expired_jwt_deleted", - rows_deleted, - ) From d446fd0068230604b733f504dff080e58ba30617 Mon Sep 17 00:00:00 2001 From: Alexis Date: Wed, 10 Jul 2024 18:30:07 +0200 Subject: [PATCH 06/15] Linting --- tests/unit/oidc/test_services.py | 75 +++++++++++++++++++------------- tests/unit/oidc/test_views.py | 2 - warehouse/oidc/models/github.py | 1 + warehouse/oidc/services.py | 5 ++- warehouse/oidc/views.py | 2 +- 5 files changed, 50 insertions(+), 35 deletions(-) diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 02d7ece64369..9a19c445e722 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -289,26 +289,33 @@ def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metric ) expiration = int( - (datetime.datetime.now(tz=datetime.timezone.utc) + datetime.timedelta(minutes=15)).timestamp() + ( + datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) + ).timestamp() ) jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) - claims = SignedClaims({ - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": expiration, - "aud": "pypi", - "jti": jwt_token_identifier, - }) + claims = SignedClaims( + { + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": expiration, + "aud": "pypi", + "jti": jwt_token_identifier, + } + ) with pytest.raises(errors.ReusedTokenError): service.find_publisher(claims, pending=False) - assert pretend.call( + assert ( + pretend.call( "warehouse.oidc.reused_token", tags=["publisher:fakepublisher"] - ) in metrics.increment.calls + ) + in metrics.increment.calls + ) def test_find_publisher_store_jti(self, monkeypatch, mockredis, metrics): service = services.OIDCPublisherService( @@ -329,22 +336,28 @@ def test_find_publisher_store_jti(self, monkeypatch, mockredis, metrics): ) expiration = int( - (datetime.datetime.now(tz=datetime.timezone.utc) + datetime.timedelta(minutes=15)).timestamp() + ( + datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) + ).timestamp() ) jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - claims = SignedClaims({ - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": expiration, - "aud": "pypi", - "jti": jwt_token_identifier, - }) + claims = SignedClaims( + { + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": expiration, + "aud": "pypi", + "jti": jwt_token_identifier, + } + ) service.find_publisher(claims, pending=False) assert service.token_identifier_exists(jwt_token_identifier) is True - def test_find_publisher_jti_not_stored_if_pending(self, monkeypatch, mockredis, metrics): + def test_find_publisher_jti_not_stored_if_pending( + self, monkeypatch, mockredis, metrics + ): service = services.OIDCPublisherService( session=pretend.stub(), publisher="fakepublisher", @@ -362,14 +375,16 @@ def test_find_publisher_jti_not_stored_if_pending(self, monkeypatch, mockredis, services, "find_publisher_by_issuer", find_publisher_by_issuer ) jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - claims = SignedClaims({ - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": int(datetime.datetime.now(tz=datetime.timezone.utc).timestamp()), - "aud": "pypi", - "jti": jwt_token_identifier, - }) + claims = SignedClaims( + { + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": int(datetime.datetime.now(tz=datetime.UTC).timestamp()), + "aud": "pypi", + "jti": jwt_token_identifier, + } + ) service.find_publisher(claims, pending=True) assert service.token_identifier_exists(jwt_token_identifier) is False @@ -926,7 +941,7 @@ def test_find_publisher(self, monkeypatch): "nbf": 1516239022, "exp": 9999999999, "aud": "pypi", - "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", } service = services.NullOIDCPublisherService( diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 0637363d17f9..9054c8958a18 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -368,7 +368,6 @@ def find_publishers_mockup(_, pending: bool = False): else: raise errors.InvalidPublisherError("some message") - claims = pretend.stub() oidc_service = pretend.stub( verify_jwt_signature=pretend.call_recorder(lambda token: claims), @@ -393,7 +392,6 @@ def find_publishers_mockup(_, pending: bool = False): } - def test_mint_token_pending_publisher_project_already_exists( db_request, dummy_github_oidc_jwt ): diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index f7e6c3c0a33d..3d9dc279b5f7 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -160,6 +160,7 @@ class GitHubPublisherMixin: "enterprise_id", "ref_protected", } + @staticmethod def __lookup_all__(klass, signed_claims: SignedClaims) -> Query | None: # This lookup requires the environment claim to be present; diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 3f21a3a9ea42..dde446bab484 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -233,7 +233,6 @@ def store_jwt_identifier(self, jti: str, expiration: int) -> None: with redis.StrictRedis.from_url(self.cache_url) as r: r.set(jti, exat=expiration, value="placeholder", nx=True) - def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: try: key = self._get_key_for_token(unverified_token) @@ -322,7 +321,9 @@ def find_publisher( ) if pending is False and jwt_token_identifier: - self.store_jwt_identifier(jwt_token_identifier, signed_claims.get("exp")) + self.store_jwt_identifier( + jwt_token_identifier, int(signed_claims.get("exp")) + ) return publisher except InvalidPublisherError as e: diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index dc8f5d67c28c..662742fb0f6b 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -246,7 +246,7 @@ def mint_token( "description": "valid token, but already used", } ], - request=request + request=request, ) except InvalidPublisherError as e: return _invalid( From 5a658928ff16e73a8e7ff5df2fe74422164e772d Mon Sep 17 00:00:00 2001 From: Alexis Date: Thu, 11 Jul 2024 10:19:23 +0200 Subject: [PATCH 07/15] Move jti to preverified claims --- tests/unit/oidc/models/test_github.py | 5 ++--- tests/unit/oidc/models/test_gitlab.py | 5 ++--- warehouse/oidc/models/github.py | 11 ++++++++++- warehouse/oidc/models/gitlab.py | 11 ++++++++++- warehouse/oidc/services.py | 18 +++++++++++------- 5 files changed, 35 insertions(+), 15 deletions(-) diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index 1f0d2d3bb82a..e823a2ae510f 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/tests/unit/oidc/models/test_github.py @@ -51,7 +51,6 @@ def test_github_publisher_all_known_claims(self): # required unverifiable claims "ref", "sha", - "jti", # optional verifiable claims "environment", # preverified claims @@ -60,6 +59,7 @@ def test_github_publisher_all_known_claims(self): "nbf", "exp", "aud", + "jti", # unchecked claims "actor", "actor_id", @@ -143,7 +143,7 @@ def test_github_publisher_unaccounted_claims(self, monkeypatch): ] assert scope.fingerprint == ["another-fake-claim", "fake-claim"] - @pytest.mark.parametrize("missing", ["sub", "ref", "jti"]) + @pytest.mark.parametrize("missing", ["sub", "ref"]) def test_github_publisher_missing_claims(self, monkeypatch, missing): publisher = github.GitHubPublisher( repository_name="fakerepo", @@ -198,7 +198,6 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch): signed_claims["ref"] = "ref" signed_claims["sha"] = "sha" signed_claims["job_workflow_ref"] = publisher.job_workflow_ref + "@ref" - signed_claims["jti"] = "jti" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: publisher.verify_claims(signed_claims=signed_claims) diff --git a/tests/unit/oidc/models/test_gitlab.py b/tests/unit/oidc/models/test_gitlab.py index 7c8f5473e5af..ca4e457a8d17 100644 --- a/tests/unit/oidc/models/test_gitlab.py +++ b/tests/unit/oidc/models/test_gitlab.py @@ -49,7 +49,6 @@ def test_gitlab_publisher_all_known_claims(self): # required unverifiable claims "ref_path", "sha", - "jti", # optional verifiable claims "environment", # preverified claims @@ -58,6 +57,7 @@ def test_gitlab_publisher_all_known_claims(self): "nbf", "exp", "aud", + "jti", # unchecked claims "project_id", "namespace_id", @@ -141,7 +141,7 @@ def test_gitlab_publisher_unaccounted_claims(self, monkeypatch): ] assert scope.fingerprint == ["another-fake-claim", "fake-claim"] - @pytest.mark.parametrize("missing", ["sub", "ref_path", "jti"]) + @pytest.mark.parametrize("missing", ["sub", "ref_path"]) def test_gitlab_publisher_missing_claims(self, monkeypatch, missing): publisher = gitlab.GitLabPublisher( project="fakerepo", @@ -194,7 +194,6 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch): signed_claims["ref_path"] = "ref" signed_claims["sha"] = "sha" signed_claims["ci_config_ref_uri"] = publisher.ci_config_ref_uri + "@ref" - signed_claims["jti"] = "jti" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: publisher.verify_claims(signed_claims=signed_claims) diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index 3d9dc279b5f7..4233c62564a2 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -132,12 +132,21 @@ class GitHubPublisherMixin: "job_workflow_ref": _check_job_workflow_ref, } - __required_unverifiable_claims__: set[str] = {"ref", "sha", "jti"} + __required_unverifiable_claims__: set[str] = {"ref", "sha"} __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { "environment": _check_environment, } + __preverified_claims__ = { + "iss", + "iat", + "nbf", + "exp", + "aud", + "jti", + } + __unchecked_claims__ = { "actor", "actor_id", diff --git a/warehouse/oidc/models/gitlab.py b/warehouse/oidc/models/gitlab.py index e16715343e67..2aac4b18c020 100644 --- a/warehouse/oidc/models/gitlab.py +++ b/warehouse/oidc/models/gitlab.py @@ -120,7 +120,16 @@ class GitLabPublisherMixin: "ci_config_ref_uri": _check_ci_config_ref_uri, } - __required_unverifiable_claims__: set[str] = {"ref_path", "sha", "jti"} + __required_unverifiable_claims__: set[str] = {"ref_path", "sha"} + + __preverified_claims__ = { + "iss", + "iat", + "nbf", + "exp", + "aud", + "jti", + } __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { "environment": _check_environment, diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index dde446bab484..c3281811b5e7 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -11,6 +11,7 @@ # limitations under the License. import json +import typing import warnings import jwt @@ -231,7 +232,10 @@ def store_jwt_identifier(self, jti: str, expiration: int) -> None: Store the JTI with its expiration date if the key does not exist. """ with redis.StrictRedis.from_url(self.cache_url) as r: - r.set(jti, exat=expiration, value="placeholder", nx=True) + # Defensive: to prevent races, we expire the JTI slightly after + # the token expiration date. Thus, the lock will not be + # released before the token invalidation. + r.set(jti, exat=expiration + 1, value="placeholder", nx=True) def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: try: @@ -303,9 +307,8 @@ def find_publisher( ) jwt_token_identifier: str | None = signed_claims.get("jti", None) - # It is ok to not perform the reused token check here if we don't have a JTI - # because it is going to be checked in `verify_claims` function if the claim - # was required and fail with the appropriate error. + # jti is in the __preverified_claims__ set, so if it was present, + # it was already checked if pending is False and jwt_token_identifier: if self.token_identifier_exists(jwt_token_identifier): self.metrics.increment( @@ -321,9 +324,10 @@ def find_publisher( ) if pending is False and jwt_token_identifier: - self.store_jwt_identifier( - jwt_token_identifier, int(signed_claims.get("exp")) - ) + # Of note, exp is coming from a trusted source here, + # so we don't validate it + expiration = typing.cast(int, signed_claims.get("exp")) + self.store_jwt_identifier(jwt_token_identifier, expiration) return publisher except InvalidPublisherError as e: From 1d8156737366208cdf3996514cb21665544e8f8e Mon Sep 17 00:00:00 2001 From: Alexis Date: Thu, 11 Jul 2024 15:17:30 +0200 Subject: [PATCH 08/15] Update lock period to 5s. --- warehouse/oidc/services.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index c3281811b5e7..990637830948 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -235,7 +235,7 @@ def store_jwt_identifier(self, jti: str, expiration: int) -> None: # Defensive: to prevent races, we expire the JTI slightly after # the token expiration date. Thus, the lock will not be # released before the token invalidation. - r.set(jti, exat=expiration + 1, value="placeholder", nx=True) + r.set(jti, exat=expiration + 5, value="placeholder", nx=True) def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: try: From 776238c0b2afd68270213f113dd9ee43f86ea542 Mon Sep 17 00:00:00 2001 From: Alexis Date: Thu, 11 Jul 2024 17:50:14 +0200 Subject: [PATCH 09/15] Add a prefix for the Redis key --- warehouse/oidc/services.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 990637830948..3433332a6fad 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -225,7 +225,7 @@ def token_identifier_exists(self, jti: str) -> bool: Check if a JWT Token Identifier has already been used. """ with redis.StrictRedis.from_url(self.cache_url) as r: - return bool(r.exists(jti)) + return bool(r.exists(f"/warehouse/oidc/{self.publisher}/{jti}")) def store_jwt_identifier(self, jti: str, expiration: int) -> None: """ @@ -235,7 +235,12 @@ def store_jwt_identifier(self, jti: str, expiration: int) -> None: # Defensive: to prevent races, we expire the JTI slightly after # the token expiration date. Thus, the lock will not be # released before the token invalidation. - r.set(jti, exat=expiration + 5, value="placeholder", nx=True) + r.set( + f"/warehouse/oidc/{self.publisher}/{jti}", + exat=expiration + 5, + value="placeholder", + nx=True, + ) def verify_jwt_signature(self, unverified_token: str) -> SignedClaims | None: try: @@ -306,7 +311,7 @@ def find_publisher( self.db, self.issuer_url, signed_claims, pending=pending ) - jwt_token_identifier: str | None = signed_claims.get("jti", None) + jwt_token_identifier: str | None = signed_claims.get("jti") # jti is in the __preverified_claims__ set, so if it was present, # it was already checked if pending is False and jwt_token_identifier: From d326cf4e4e28f759b1fbd5d4d003e53e7331f10b Mon Sep 17 00:00:00 2001 From: Alexis Date: Mon, 15 Jul 2024 10:24:28 +0200 Subject: [PATCH 10/15] Remove duplication in Github/Gitlab --- warehouse/oidc/models/github.py | 8 ++------ warehouse/oidc/models/gitlab.py | 8 ++------ warehouse/oidc/services.py | 2 +- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index d10f7d414bf2..58d0c64af392 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -28,6 +28,7 @@ from warehouse.oidc.models._core import ( CheckClaimCallable, OIDCPublisher, + OIDCPublisherMixin, PendingOIDCPublisher, check_claim_binary, ) @@ -144,12 +145,7 @@ class GitHubPublisherMixin: "environment": _check_environment, } - __preverified_claims__ = { - "iss", - "iat", - "nbf", - "exp", - "aud", + __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { "jti", } diff --git a/warehouse/oidc/models/gitlab.py b/warehouse/oidc/models/gitlab.py index 2aac4b18c020..43c87bfa0a13 100644 --- a/warehouse/oidc/models/gitlab.py +++ b/warehouse/oidc/models/gitlab.py @@ -22,6 +22,7 @@ from warehouse.oidc.models._core import ( CheckClaimCallable, OIDCPublisher, + OIDCPublisherMixin, PendingOIDCPublisher, ) @@ -122,12 +123,7 @@ class GitLabPublisherMixin: __required_unverifiable_claims__: set[str] = {"ref_path", "sha"} - __preverified_claims__ = { - "iss", - "iat", - "nbf", - "exp", - "aud", + __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { "jti", } diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index 3433332a6fad..d69dfbd9b9fc 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -238,7 +238,7 @@ def store_jwt_identifier(self, jti: str, expiration: int) -> None: r.set( f"/warehouse/oidc/{self.publisher}/{jti}", exat=expiration + 5, - value="placeholder", + value="", # empty value to lower memory usage nx=True, ) From 799b322fcac45b9e2dbf9fac7ba8daa02c8a2303 Mon Sep 17 00:00:00 2001 From: dm Date: Mon, 15 Jul 2024 16:19:34 +0200 Subject: [PATCH 11/15] Rephrase token error message Co-authored-by: William Woodruff --- warehouse/oidc/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 662742fb0f6b..6aa6d843b141 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -243,7 +243,7 @@ def mint_token( errors=[ { "code": "invalid-reuse-token", - "description": "valid token, but already used", + "description": "invalid token: already used", } ], request=request, From a088b7987ffa3eab5f3379f2a98e74ab9321d238 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 16 Jul 2024 09:31:44 +0200 Subject: [PATCH 12/15] Fix test --- tests/unit/oidc/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/oidc/test_views.py b/tests/unit/oidc/test_views.py index 9054c8958a18..02f0a60846fd 100644 --- a/tests/unit/oidc/test_views.py +++ b/tests/unit/oidc/test_views.py @@ -386,7 +386,7 @@ def find_publishers_mockup(_, pending: bool = False): "errors": [ { "code": "invalid-reuse-token", - "description": "valid token, but already used", + "description": "invalid token: already used", } ], } From 31f4417ba049508e5fd13fa6c2592d550b4819c8 Mon Sep 17 00:00:00 2001 From: Alexis Date: Fri, 9 Aug 2024 16:25:33 +0200 Subject: [PATCH 13/15] Check JTI in Publishers with check_existing_jti --- tests/unit/oidc/models/test_activestate.py | 15 +- tests/unit/oidc/models/test_core.py | 35 +++- tests/unit/oidc/models/test_github.py | 22 ++- tests/unit/oidc/models/test_gitlab.py | 22 ++- tests/unit/oidc/models/test_google.py | 24 ++- tests/unit/oidc/test_services.py | 177 ++++++++++----------- warehouse/oidc/models/_core.py | 64 +++++++- warehouse/oidc/models/activestate.py | 2 +- warehouse/oidc/models/github.py | 29 +++- warehouse/oidc/models/gitlab.py | 29 +++- warehouse/oidc/models/google.py | 5 +- warehouse/oidc/services.py | 36 ++--- warehouse/oidc/views.py | 10 +- 13 files changed, 306 insertions(+), 164 deletions(-) diff --git a/tests/unit/oidc/models/test_activestate.py b/tests/unit/oidc/models/test_activestate.py index f03e23fdf20f..dc6d1becde24 100644 --- a/tests/unit/oidc/models/test_activestate.py +++ b/tests/unit/oidc/models/test_activestate.py @@ -161,7 +161,9 @@ def test_activestate_publisher_unaccounted_claims(self, monkeypatch): signed_claims["fake-claim"] = "fake" signed_claims["another-fake-claim"] = "also-fake" - assert publisher.verify_claims(signed_claims=signed_claims) + assert publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert sentry_sdk.capture_message.calls == [ pretend.call( @@ -211,10 +213,17 @@ def test_activestate_publisher_missing_claims( assert claim_to_drop not in signed_claims if valid: - assert publisher.verify_claims(signed_claims=signed_claims) is valid + assert ( + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) + is valid + ) else: with pytest.raises(InvalidPublisherError) as e: - assert publisher.verify_claims(signed_claims=signed_claims) + assert publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == error_msg assert sentry_sdk.capture_message.calls == [ pretend.call( diff --git a/tests/unit/oidc/models/test_core.py b/tests/unit/oidc/models/test_core.py index bc477722d5ea..aa83db4e1dd8 100644 --- a/tests/unit/oidc/models/test_core.py +++ b/tests/unit/oidc/models/test_core.py @@ -51,5 +51,38 @@ def test_oidc_publisher_not_default_verifiable(self): publisher = _core.OIDCPublisher(projects=[]) with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims={}) + publisher.verify_claims(signed_claims={}, publisher_service=pretend.stub()) assert str(e.value) == "No required verifiable claims" + + +def test_check_existing_jti(): + publisher = pretend.stub( + token_identifier_exists=pretend.call_recorder(lambda s: False), + ) + + assert _core.check_existing_jti( + pretend.stub(), + "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", + pretend.stub(), + publisher_service=publisher, + ) + + +def test_check_existing_jti_fails(metrics): + publisher = pretend.stub( + token_identifier_exists=pretend.call_recorder(lambda s: True), + metrics=metrics, + publisher="fakepublisher", + ) + with pytest.raises(errors.ReusedTokenError): + assert _core.check_existing_jti( + pretend.stub(), + "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", + pretend.stub(), + publisher_service=publisher, + ) + + assert ( + pretend.call("warehouse.oidc.reused_token", tags=["publisher:fakepublisher"]) + in metrics.increment.calls + ) diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index 6902f7a771f7..cdece3012c07 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/tests/unit/oidc/models/test_github.py @@ -134,7 +134,9 @@ def test_github_publisher_unaccounted_claims(self, monkeypatch): signed_claims["fake-claim"] = "fake" signed_claims["another-fake-claim"] = "also-fake" with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Check failed for required claim 'sub'" assert sentry_sdk.capture_message.calls == [ pretend.call( @@ -173,7 +175,9 @@ def test_github_publisher_missing_claims(self, monkeypatch, missing): assert missing not in signed_claims assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == f"Missing claim {missing!r}" assert sentry_sdk.capture_message.calls == [ pretend.call(f"JWT for GitHubPublisher is missing claim: {missing}") @@ -192,6 +196,10 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch): sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) monkeypatch.setattr(_core, "sentry_sdk", sentry_sdk) + service_ = pretend.stub( + token_identifier_exists=pretend.call_recorder(lambda s: False), + ) + signed_claims = { claim_name: getattr(publisher, claim_name) for claim_name in github.GitHubPublisher.__required_verifiable_claims__ @@ -201,7 +209,9 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch): signed_claims["job_workflow_ref"] = publisher.job_workflow_ref + "@ref" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=service_ + ) assert str(e.value) == "Check failed for optional claim 'environment'" assert sentry_sdk.capture_message.calls == [] @@ -219,7 +229,7 @@ def test_github_publisher_verifies(self, monkeypatch, environment, missing_claim environment=environment, ) - noop_check = pretend.call_recorder(lambda gt, sc, ac: True) + noop_check = pretend.call_recorder(lambda gt, sc, ac, **kwargs: True) verifiable_claims = { claim_name: noop_check for claim_name in publisher.__required_verifiable_claims__ @@ -240,7 +250,9 @@ def test_github_publisher_verifies(self, monkeypatch, environment, missing_claim for claim_name in github.GitHubPublisher.all_known_claims() if claim_name not in missing_claims } - assert publisher.verify_claims(signed_claims=signed_claims) + assert publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert len(noop_check.calls) == len(verifiable_claims) + len( optional_verifiable_claims ) diff --git a/tests/unit/oidc/models/test_gitlab.py b/tests/unit/oidc/models/test_gitlab.py index ca4e457a8d17..e0c55bc809b4 100644 --- a/tests/unit/oidc/models/test_gitlab.py +++ b/tests/unit/oidc/models/test_gitlab.py @@ -131,7 +131,9 @@ def test_gitlab_publisher_unaccounted_claims(self, monkeypatch): signed_claims["fake-claim"] = "fake" signed_claims["another-fake-claim"] = "also-fake" with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Check failed for required claim 'sub'" assert sentry_sdk.capture_message.calls == [ pretend.call( @@ -169,7 +171,9 @@ def test_gitlab_publisher_missing_claims(self, monkeypatch, missing): assert missing not in signed_claims assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == f"Missing claim {missing!r}" assert sentry_sdk.capture_message.calls == [ pretend.call(f"JWT for GitLabPublisher is missing claim: {missing}") @@ -187,6 +191,10 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch): sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None)) monkeypatch.setattr(_core, "sentry_sdk", sentry_sdk) + service = pretend.stub( + token_identifier_exists=pretend.call_recorder(lambda s: False) + ) + signed_claims = { claim_name: getattr(publisher, claim_name) for claim_name in gitlab.GitLabPublisher.__required_verifiable_claims__ @@ -196,7 +204,9 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch): signed_claims["ci_config_ref_uri"] = publisher.ci_config_ref_uri + "@ref" assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=service + ) assert str(e.value) == "Check failed for optional claim 'environment'" assert sentry_sdk.capture_message.calls == [] @@ -213,7 +223,7 @@ def test_gitlab_publisher_verifies(self, monkeypatch, environment, missing_claim environment="environment", ) - noop_check = pretend.call_recorder(lambda gt, sc, ac: True) + noop_check = pretend.call_recorder(lambda gt, sc, ac, **kwargs: True) verifiable_claims = { claim_name: noop_check for claim_name in publisher.__required_verifiable_claims__ @@ -234,7 +244,9 @@ def test_gitlab_publisher_verifies(self, monkeypatch, environment, missing_claim for claim_name in gitlab.GitLabPublisher.all_known_claims() if claim_name not in missing_claims } - assert publisher.verify_claims(signed_claims=signed_claims) + assert publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert len(noop_check.calls) == len(verifiable_claims) + len( optional_verifiable_claims ) diff --git a/tests/unit/oidc/models/test_google.py b/tests/unit/oidc/models/test_google.py index d0ce98e689d3..ae4d72e98719 100644 --- a/tests/unit/oidc/models/test_google.py +++ b/tests/unit/oidc/models/test_google.py @@ -91,7 +91,9 @@ def test_google_publisher_unaccounted_claims(self, monkeypatch): signed_claims["fake-claim"] = "fake" signed_claims["another-fake-claim"] = "also-fake" with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Check failed for required claim 'email'" assert sentry_sdk.capture_message.calls == [ pretend.call( @@ -127,7 +129,9 @@ def test_google_publisher_missing_claims(self, monkeypatch): assert "email" not in signed_claims assert publisher.__required_verifiable_claims__ with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Missing claim 'email'" assert sentry_sdk.capture_message.calls == [ pretend.call("JWT for GooglePublisher is missing claim: email") @@ -151,10 +155,14 @@ def test_google_publisher_email_verified(self, email_verified, valid): } if valid: # Does not raise - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) else: with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Check failed for required claim 'email_verified'" @pytest.mark.parametrize( @@ -182,10 +190,14 @@ def test_google_publisher_sub_is_optional(self, expected_sub, actual_sub, valid) } if valid: # Does not raise - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) else: with pytest.raises(errors.InvalidPublisherError) as e: - publisher.verify_claims(signed_claims=signed_claims) + publisher.verify_claims( + signed_claims=signed_claims, publisher_service=pretend.stub() + ) assert str(e.value) == "Check failed for optional claim 'sub'" diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 9a19c445e722..f8143a3ed144 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -188,7 +188,7 @@ def test_find_publisher(self, metrics, monkeypatch): token = SignedClaims({}) - publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c, s: True)) find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) monkeypatch.setattr( services, "find_publisher_by_issuer", find_publisher_by_issuer @@ -268,9 +268,9 @@ def test_find_publisher_verify_claims_fails(self, metrics, monkeypatch): tags=["publisher:fakepublisher"], ), ] - assert publisher.verify_claims.calls == [pretend.call(claims)] + assert publisher.verify_claims.calls == [pretend.call(claims, service)] - def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metrics): + def test_find_publisher_reuse_token_fails(self, monkeypatch, mockredis, metrics): service = services.OIDCPublisherService( session=pretend.stub(), publisher="fakepublisher", @@ -280,9 +280,9 @@ def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metric metrics=metrics, ) - monkeypatch.setattr(services.redis, "StrictRedis", mockredis) - - publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + publisher = pretend.stub( + verify_claims=pretend.call_recorder(pretend.raiser(errors.ReusedTokenError)) + ) find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) monkeypatch.setattr( services, "find_publisher_by_issuer", find_publisher_by_issuer @@ -293,8 +293,6 @@ def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metric datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) ).timestamp() ) - jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) claims = SignedClaims( { @@ -303,92 +301,13 @@ def test_find_publisher_prevent_reuse_token(self, monkeypatch, mockredis, metric "nbf": 1516239022, "exp": expiration, "aud": "pypi", - "jti": jwt_token_identifier, + "jti": "fake-token-identifier", } ) with pytest.raises(errors.ReusedTokenError): service.find_publisher(claims, pending=False) - assert ( - pretend.call( - "warehouse.oidc.reused_token", tags=["publisher:fakepublisher"] - ) - in metrics.increment.calls - ) - - def test_find_publisher_store_jti(self, monkeypatch, mockredis, metrics): - service = services.OIDCPublisherService( - session=pretend.stub(), - publisher="fakepublisher", - issuer_url=pretend.stub(), - audience="fakeaudience", - cache_url="redis://fake.example.com", - metrics=metrics, - ) - - monkeypatch.setattr(services.redis, "StrictRedis", mockredis) - - publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) - find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) - monkeypatch.setattr( - services, "find_publisher_by_issuer", find_publisher_by_issuer - ) - - expiration = int( - ( - datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) - ).timestamp() - ) - jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - claims = SignedClaims( - { - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": expiration, - "aud": "pypi", - "jti": jwt_token_identifier, - } - ) - - service.find_publisher(claims, pending=False) - assert service.token_identifier_exists(jwt_token_identifier) is True - - def test_find_publisher_jti_not_stored_if_pending( - self, monkeypatch, mockredis, metrics - ): - service = services.OIDCPublisherService( - session=pretend.stub(), - publisher="fakepublisher", - issuer_url=pretend.stub(), - audience="fakeaudience", - cache_url="redis://fake.example.com", - metrics=metrics, - ) - - monkeypatch.setattr(services.redis, "StrictRedis", mockredis) - - publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) - find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) - monkeypatch.setattr( - services, "find_publisher_by_issuer", find_publisher_by_issuer - ) - jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - claims = SignedClaims( - { - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": int(datetime.datetime.now(tz=datetime.UTC).timestamp()), - "aud": "pypi", - "jti": jwt_token_identifier, - } - ) - - service.find_publisher(claims, pending=True) - assert service.token_identifier_exists(jwt_token_identifier) is False - def test_get_keyset_not_cached(self, monkeypatch, mockredis): service = services.OIDCPublisherService( session=pretend.stub(), @@ -809,6 +728,44 @@ def test_reify_publisher(self, monkeypatch): assert pending_publisher.reify.calls == [pretend.call(service.db)] assert project.oidc_publishers == [publisher] + def test_token_identifier_exists(self, metrics, mockredis, monkeypatch): + service = services.OIDCPublisherService( + session=pretend.stub(), + publisher="fakepublisher", + issuer_url=pretend.stub(), + audience="fakeaudience", + cache_url="redis://fake.example.com", + metrics=metrics, + ) + + monkeypatch.setattr(services.redis, "StrictRedis", mockredis) + + assert service.token_identifier_exists("fake-jti-token") is False + + def test_token_identifier_exists_find_duplicate( + self, metrics, mockredis, monkeypatch + ): + service = services.OIDCPublisherService( + session=pretend.stub(), + publisher="fakepublisher", + issuer_url=pretend.stub(), + audience="fakeaudience", + cache_url="redis://fake.example.com", + metrics=metrics, + ) + + monkeypatch.setattr(services.redis, "StrictRedis", mockredis) + + expiration = int( + ( + datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) + ).timestamp() + ) + jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) + + assert service.token_identifier_exists(jwt_token_identifier) is True + class TestNullOIDCPublisherService: def test_interface_matches(self): @@ -935,14 +892,16 @@ def test_verify_jwt_signature_strict_aud(self): assert service.verify_jwt_signature(jwt) is None def test_find_publisher(self, monkeypatch): - claims = { - "iss": "foo", - "iat": 1516239022, - "nbf": 1516239022, - "exp": 9999999999, - "aud": "pypi", - "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", - } + claims = SignedClaims( + { + "iss": "foo", + "iat": 1516239022, + "nbf": 1516239022, + "exp": 9999999999, + "aud": "pypi", + "jti": "6e67b1cb-2b8d-4be5-91cb-757edb2ec970", + } + ) service = services.NullOIDCPublisherService( session=pretend.stub(), @@ -953,7 +912,7 @@ def test_find_publisher(self, monkeypatch): metrics=pretend.stub(), ) - publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c: True)) + publisher = pretend.stub(verify_claims=pretend.call_recorder(lambda c, s: True)) find_publisher_by_issuer = pretend.call_recorder(lambda *a, **kw: publisher) monkeypatch.setattr( services, "find_publisher_by_issuer", find_publisher_by_issuer @@ -1066,6 +1025,30 @@ def test_reify_publisher(self): assert pending_publisher.reify.calls == [pretend.call(service.db)] assert project.oidc_publishers == [publisher] + def test_token_identifier_exists(self): + service = services.NullOIDCPublisherService( + session=pretend.stub(), + publisher="example", + issuer_url="https://example.com", + audience="fakeaudience", + cache_url="rediss://fake.example.com", + metrics=pretend.stub(), + ) + + assert service.token_identifier_exists(pretend.stub()) is False + + def test_store_jwt_identifier(self): + service = services.NullOIDCPublisherService( + session=pretend.stub(), + publisher="example", + issuer_url="https://example.com", + audience="fakeaudience", + cache_url="rediss://fake.example.com", + metrics=pretend.stub(), + ) + + assert service.store_jwt_identifier(pretend.stub(), pretend.stub()) is None + class TestPyJWTBackstop: """ diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 9405b600d422..5ee58f430db0 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -13,7 +13,7 @@ from __future__ import annotations from collections.abc import Callable -from typing import TYPE_CHECKING, Any, TypeVar +from typing import TYPE_CHECKING, Any, TypedDict, TypeVar, Unpack, cast import sentry_sdk @@ -23,17 +23,23 @@ from sqlalchemy.orm import Mapped, mapped_column from warehouse import db -from warehouse.oidc.errors import InvalidPublisherError +from warehouse.oidc.errors import InvalidPublisherError, ReusedTokenError from warehouse.oidc.interfaces import SignedClaims if TYPE_CHECKING: from warehouse.accounts.models import User from warehouse.macaroons.models import Macaroon + from warehouse.oidc.services import OIDCPublisherService from warehouse.packaging.models import Project C = TypeVar("C") -CheckClaimCallable = Callable[[C, C, SignedClaims], bool] + +class CheckNamedArguments(TypedDict, total=False): + publisher_service: OIDCPublisherService + + +CheckClaimCallable = Callable[[C, C, SignedClaims, Unpack[CheckNamedArguments]], bool] def check_claim_binary(binary_func: Callable[[C, C], bool]) -> CheckClaimCallable[C]: @@ -45,7 +51,12 @@ def check_claim_binary(binary_func: Callable[[C, C], bool]) -> CheckClaimCallabl comparison checks like `str.__eq__`. """ - def wrapper(ground_truth: C, signed_claim: C, all_signed_claims: SignedClaims): + def wrapper( + ground_truth: C, + signed_claim: C, + _all_signed_claims: SignedClaims, + **_kwargs: Unpack[CheckNamedArguments], + ) -> bool: return binary_func(ground_truth, signed_claim) return wrapper @@ -59,12 +70,37 @@ def check_claim_invariant(value: C) -> CheckClaimCallable[C]: comparison checks, like "claim x is always the literal `true` value". """ - def wrapper(ground_truth: C, signed_claim: C, all_signed_claims: SignedClaims): + def wrapper( + ground_truth: C, + signed_claim: C, + _all_signed_claims: SignedClaims, + **_kwargs: Unpack[CheckNamedArguments], + ): return ground_truth == signed_claim == value return wrapper +def check_existing_jti( + _ground_truth, + signed_claim, + _all_signed_claims, + **kwargs: Unpack[CheckNamedArguments], +) -> bool: + + publisher_service: OIDCPublisherService = cast( + OIDCPublisherService, kwargs.get("publisher_service") + ) + if publisher_service.token_identifier_exists(signed_claim): + publisher_service.metrics.increment( + "warehouse.oidc.reused_token", + tags=[f"publisher:{publisher_service.publisher}"], + ) + raise ReusedTokenError() + + return True + + class OIDCPublisherProjectAssociation(db.Model): __tablename__ = "oidc_publisher_project_association" @@ -162,7 +198,9 @@ def all_known_claims(cls) -> set[str]: | cls.__unchecked_claims__ ) - def verify_claims(self, signed_claims: SignedClaims): + def verify_claims( + self, signed_claims: SignedClaims, publisher_service: OIDCPublisherService + ): """ Given a JWT that has been successfully decoded (checked for a valid signature and basic claims), verify it against the more specific @@ -211,7 +249,12 @@ def verify_claims(self, signed_claims: SignedClaims): # verifiable claim is correct for claim_name, check in self.__required_verifiable_claims__.items(): signed_claim = signed_claims.get(claim_name) - if not check(getattr(self, claim_name), signed_claim, signed_claims): + if not check( + getattr(self, claim_name), + signed_claim, + signed_claims, + publisher_service=publisher_service, + ): raise InvalidPublisherError( f"Check failed for required claim {claim_name!r}" ) @@ -224,7 +267,12 @@ def verify_claims(self, signed_claims: SignedClaims): # required for a given publisher. signed_claim = signed_claims.get(claim_name) - if not check(getattr(self, claim_name), signed_claim, signed_claims): + if not check( + getattr(self, claim_name), + signed_claim, + signed_claims, + publisher_service=publisher_service, + ): raise InvalidPublisherError( f"Check failed for optional claim {claim_name!r}" ) diff --git a/warehouse/oidc/models/activestate.py b/warehouse/oidc/models/activestate.py index b5939f3b4a82..e004f4fb601f 100644 --- a/warehouse/oidc/models/activestate.py +++ b/warehouse/oidc/models/activestate.py @@ -33,7 +33,7 @@ def _check_sub( - ground_truth: str, signed_claim: str, _all_signed_claims: SignedClaims + ground_truth: str, signed_claim: str, _all_signed_claims: SignedClaims, **_kwargs ) -> bool: # We expect a string formatted as follows: # org::project: diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index 58d0c64af392..aaeccaef26c2 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -31,10 +31,11 @@ OIDCPublisherMixin, PendingOIDCPublisher, check_claim_binary, + check_existing_jti, ) -def _check_repository(ground_truth, signed_claim, all_signed_claims): +def _check_repository(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # Defensive: GitHub should never give us an empty repository claim. if not signed_claim: return False @@ -43,7 +44,7 @@ def _check_repository(ground_truth, signed_claim, all_signed_claims): return signed_claim.lower() == ground_truth.lower() -def _check_job_workflow_ref(ground_truth, signed_claim, all_signed_claims): +def _check_job_workflow_ref(ground_truth, signed_claim, all_signed_claims, **_kwargs): # We expect a string formatted as follows: # OWNER/REPO/.github/workflows/WORKFLOW.yml@REF # where REF is the value of either the `ref` or `sha` claims. @@ -74,7 +75,7 @@ def _check_job_workflow_ref(ground_truth, signed_claim, all_signed_claims): return True -def _check_environment(ground_truth, signed_claim, all_signed_claims): +def _check_environment(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # When there is an environment, we expect a case-insensitive string. # https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment # For tokens that are generated outside of an environment, the claim will @@ -96,7 +97,7 @@ def _check_environment(ground_truth, signed_claim, all_signed_claims): return ground_truth.lower() == signed_claim.lower() -def _check_sub(ground_truth, signed_claim, _all_signed_claims): +def _check_sub(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # We expect a string formatted as follows: # repo:ORG/REPO[:OPTIONAL-STUFF] # where :OPTIONAL-STUFF is a concatenation of other job context @@ -145,10 +146,6 @@ class GitHubPublisherMixin: "environment": _check_environment, } - __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { - "jti", - } - __unchecked_claims__ = { "actor", "actor_id", @@ -242,6 +239,11 @@ def job_workflow_ref(self): def sub(self): return f"repo:{self.repository}" + @property + def jti(self): + """Placeholder value for JTI.""" + return True + def publisher_url(self, claims=None): base = f"https://github.com/{self.repository}" sha = claims.get("sha") if claims else None @@ -304,6 +306,13 @@ class GitHubPublisher(GitHubPublisherMixin, OIDCPublisher): ), ) + __required_verifiable_claims__ = ( + GitHubPublisherMixin.__required_verifiable_claims__ + | { + "jti": check_existing_jti, + } + ) + id = mapped_column( UUID(as_uuid=True), ForeignKey(OIDCPublisher.id), primary_key=True ) @@ -326,6 +335,10 @@ class PendingGitHubPublisher(GitHubPublisherMixin, PendingOIDCPublisher): UUID(as_uuid=True), ForeignKey(PendingOIDCPublisher.id), primary_key=True ) + __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { + "jti", + } + def reify(self, session): """ Returns a `GitHubPublisher` for this `PendingGitHubPublisher`, diff --git a/warehouse/oidc/models/gitlab.py b/warehouse/oidc/models/gitlab.py index 43c87bfa0a13..1d1857d86571 100644 --- a/warehouse/oidc/models/gitlab.py +++ b/warehouse/oidc/models/gitlab.py @@ -24,10 +24,11 @@ OIDCPublisher, OIDCPublisherMixin, PendingOIDCPublisher, + check_existing_jti, ) -def _check_project_path(ground_truth, signed_claim, all_signed_claims): +def _check_project_path(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # Defensive: GitLab should never give us an empty project_path claim. if not signed_claim: return False @@ -36,7 +37,7 @@ def _check_project_path(ground_truth, signed_claim, all_signed_claims): return signed_claim.lower() == ground_truth.lower() -def _check_ci_config_ref_uri(ground_truth, signed_claim, all_signed_claims): +def _check_ci_config_ref_uri(ground_truth, signed_claim, all_signed_claims, **_kwargs): # We expect a string formatted as follows: # gitlab.com/OWNER/REPO//WORKFLOW_PATH/WORKFLOW_FILE.yml@REF # where REF is the value of the `ref_path` claim. @@ -62,7 +63,7 @@ def _check_ci_config_ref_uri(ground_truth, signed_claim, all_signed_claims): return True -def _check_environment(ground_truth, signed_claim, all_signed_claims): +def _check_environment(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # When there is an environment, we expect a string. # For tokens that are generated outside of an environment, the claim will # be missing. @@ -81,7 +82,7 @@ def _check_environment(ground_truth, signed_claim, all_signed_claims): return ground_truth == signed_claim -def _check_sub(ground_truth, signed_claim, _all_signed_claims): +def _check_sub(ground_truth, signed_claim, _all_signed_claims, **_kwargs): # We expect a string formatted as follows: # project_path:NAMESPACE/PROJECT[:OPTIONAL-STUFF] # where :OPTIONAL-STUFF is a concatenation of other job context @@ -123,10 +124,6 @@ class GitLabPublisherMixin: __required_unverifiable_claims__: set[str] = {"ref_path", "sha"} - __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { - "jti", - } - __optional_verifiable_claims__: dict[str, CheckClaimCallable[Any]] = { "environment": _check_environment, } @@ -226,6 +223,11 @@ def ci_config_ref_uri(self): def publisher_name(self): return "GitLab" + @property + def jti(self): + """Placeholder value for JTI.""" + return True + def publisher_url(self, claims=None): base = f"https://gitlab.com/{self.project_path}" return f"{base}/commit/{claims['sha']}" if claims else base @@ -251,6 +253,13 @@ class GitLabPublisher(GitLabPublisherMixin, OIDCPublisher): ), ) + __required_verifiable_claims__ = ( + GitLabPublisherMixin.__required_verifiable_claims__ + | { + "jti": check_existing_jti, + } + ) + id = mapped_column( UUID(as_uuid=True), ForeignKey(OIDCPublisher.id), primary_key=True ) @@ -269,6 +278,10 @@ class PendingGitLabPublisher(GitLabPublisherMixin, PendingOIDCPublisher): ), ) + __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { + "jti", + } + id = mapped_column( UUID(as_uuid=True), ForeignKey(PendingOIDCPublisher.id), primary_key=True ) diff --git a/warehouse/oidc/models/google.py b/warehouse/oidc/models/google.py index acd660f7d355..9fc14e355bf3 100644 --- a/warehouse/oidc/models/google.py +++ b/warehouse/oidc/models/google.py @@ -27,7 +27,10 @@ def _check_sub( - ground_truth: str, signed_claim: str, all_signed_claims: SignedClaims + ground_truth: str, + signed_claim: str, + _all_signed_claims: SignedClaims, + **_kwargs, ) -> bool: # If we haven't set a subject for the publisher, we don't need to check # this claim. diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index ae97d4cc219e..e78d431b03ce 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -11,7 +11,6 @@ # limitations under the License. import json -import typing import warnings import jwt @@ -22,7 +21,7 @@ from zope.interface import implementer from warehouse.metrics.interfaces import IMetricsService -from warehouse.oidc.errors import InvalidPublisherError, ReusedTokenError +from warehouse.oidc.errors import InvalidPublisherError from warehouse.oidc.interfaces import IOIDCPublisherService, SignedClaims from warehouse.oidc.models import OIDCPublisher, PendingOIDCPublisher from warehouse.oidc.utils import find_publisher_by_issuer @@ -80,6 +79,20 @@ def reify_pending_publisher(self, pending_publisher, project): project.oidc_publishers.append(new_publisher) return new_publisher + def token_identifier_exists(self, jti: str) -> bool: + """ + The NullOIDCPublisherService does not provide a mechanism to store used tokens + before their expiration. + """ + return False + + def store_jwt_identifier(self, jti: str, expiration: int) -> None: + """ + The NullOIDCPublisherService does not provide a mechanism to store used tokens + before their expiration. + """ + return + @implementer(IOIDCPublisherService) class OIDCPublisherService: @@ -311,29 +324,12 @@ def find_publisher( self.db, self.issuer_url, signed_claims, pending=pending ) - jwt_token_identifier: str | None = signed_claims.get("jti") - # jti is in the __preverified_claims__ set, so if it was present, - # it was already checked - if pending is False and jwt_token_identifier: - if self.token_identifier_exists(jwt_token_identifier): - self.metrics.increment( - "warehouse.oidc.reused_token", - tags=metrics_tags, - ) - raise ReusedTokenError("JWT Token already used to mint a token.") - - publisher.verify_claims(signed_claims) + publisher.verify_claims(signed_claims, self) self.metrics.increment( "warehouse.oidc.find_publisher.ok", tags=metrics_tags, ) - if pending is False and jwt_token_identifier: - # Of note, exp is coming from a trusted source here, - # so we don't validate it - expiration = typing.cast(int, signed_claims.get("exp")) - self.store_jwt_identifier(jwt_token_identifier, expiration) - return publisher except InvalidPublisherError as e: self.metrics.increment( diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 75233bd829a0..3f05dc5a6026 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -13,7 +13,7 @@ import time from datetime import datetime -from typing import TypedDict +from typing import TypedDict, cast import jwt import sentry_sdk @@ -284,6 +284,14 @@ def mint_token( oidc_publisher_id=str(publisher.id), additional={"oidc": publisher.stored_claims(claims)}, ) + + # We have used the given JWT to mint a new token. Let now store it to prevent + # its reuse if the claims contain a JTI. Of note, exp is coming from a trusted + # source here, so we don't validate it + if jwt_token_identifier := claims.get("jti"): + expiration = cast(int, claims.get("exp")) + oidc_service.store_jwt_identifier(jwt_token_identifier, expiration) + for project in publisher.projects: project.record_event( tag=EventTag.Project.ShortLivedAPITokenAdded, From 35e50e26c93d955ac9441404d883c7074da5309f Mon Sep 17 00:00:00 2001 From: Alexis Date: Mon, 12 Aug 2024 10:10:38 +0200 Subject: [PATCH 14/15] Renmame token_identifier_exists and move jti token check --- tests/unit/oidc/models/test_core.py | 4 ++-- tests/unit/oidc/models/test_github.py | 2 +- tests/unit/oidc/models/test_gitlab.py | 2 +- tests/unit/oidc/test_services.py | 12 ++++++------ warehouse/oidc/models/_core.py | 10 +++++----- warehouse/oidc/models/github.py | 17 +++-------------- warehouse/oidc/models/gitlab.py | 17 +++-------------- warehouse/oidc/services.py | 4 ++-- 8 files changed, 23 insertions(+), 45 deletions(-) diff --git a/tests/unit/oidc/models/test_core.py b/tests/unit/oidc/models/test_core.py index aa83db4e1dd8..fc888c380478 100644 --- a/tests/unit/oidc/models/test_core.py +++ b/tests/unit/oidc/models/test_core.py @@ -57,7 +57,7 @@ def test_oidc_publisher_not_default_verifiable(self): def test_check_existing_jti(): publisher = pretend.stub( - token_identifier_exists=pretend.call_recorder(lambda s: False), + jwt_identifier_exists=pretend.call_recorder(lambda s: False), ) assert _core.check_existing_jti( @@ -70,7 +70,7 @@ def test_check_existing_jti(): def test_check_existing_jti_fails(metrics): publisher = pretend.stub( - token_identifier_exists=pretend.call_recorder(lambda s: True), + jwt_identifier_exists=pretend.call_recorder(lambda s: True), metrics=metrics, publisher="fakepublisher", ) diff --git a/tests/unit/oidc/models/test_github.py b/tests/unit/oidc/models/test_github.py index cdece3012c07..3fc818b1d31b 100644 --- a/tests/unit/oidc/models/test_github.py +++ b/tests/unit/oidc/models/test_github.py @@ -197,7 +197,7 @@ def test_github_publisher_missing_optional_claims(self, monkeypatch): monkeypatch.setattr(_core, "sentry_sdk", sentry_sdk) service_ = pretend.stub( - token_identifier_exists=pretend.call_recorder(lambda s: False), + jwt_identifier_exists=pretend.call_recorder(lambda s: False), ) signed_claims = { diff --git a/tests/unit/oidc/models/test_gitlab.py b/tests/unit/oidc/models/test_gitlab.py index e0c55bc809b4..d1016b017bbb 100644 --- a/tests/unit/oidc/models/test_gitlab.py +++ b/tests/unit/oidc/models/test_gitlab.py @@ -192,7 +192,7 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch): monkeypatch.setattr(_core, "sentry_sdk", sentry_sdk) service = pretend.stub( - token_identifier_exists=pretend.call_recorder(lambda s: False) + jwt_identifier_exists=pretend.call_recorder(lambda s: False) ) signed_claims = { diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index f8143a3ed144..8737728730c1 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -728,7 +728,7 @@ def test_reify_publisher(self, monkeypatch): assert pending_publisher.reify.calls == [pretend.call(service.db)] assert project.oidc_publishers == [publisher] - def test_token_identifier_exists(self, metrics, mockredis, monkeypatch): + def test_jwt_identifier_exists(self, metrics, mockredis, monkeypatch): service = services.OIDCPublisherService( session=pretend.stub(), publisher="fakepublisher", @@ -740,9 +740,9 @@ def test_token_identifier_exists(self, metrics, mockredis, monkeypatch): monkeypatch.setattr(services.redis, "StrictRedis", mockredis) - assert service.token_identifier_exists("fake-jti-token") is False + assert service.jwt_identifier_exists("fake-jti-token") is False - def test_token_identifier_exists_find_duplicate( + def test_jwt_identifier_exists_find_duplicate( self, metrics, mockredis, monkeypatch ): service = services.OIDCPublisherService( @@ -764,7 +764,7 @@ def test_token_identifier_exists_find_duplicate( jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) - assert service.token_identifier_exists(jwt_token_identifier) is True + assert service.jwt_identifier_exists(jwt_token_identifier) is True class TestNullOIDCPublisherService: @@ -1025,7 +1025,7 @@ def test_reify_publisher(self): assert pending_publisher.reify.calls == [pretend.call(service.db)] assert project.oidc_publishers == [publisher] - def test_token_identifier_exists(self): + def test_jwt_identifier_exists(self): service = services.NullOIDCPublisherService( session=pretend.stub(), publisher="example", @@ -1035,7 +1035,7 @@ def test_token_identifier_exists(self): metrics=pretend.stub(), ) - assert service.token_identifier_exists(pretend.stub()) is False + assert service.jwt_identifier_exists(pretend.stub()) is False def test_store_jwt_identifier(self): service = services.NullOIDCPublisherService( diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py index 5ee58f430db0..9bcb619aec85 100644 --- a/warehouse/oidc/models/_core.py +++ b/warehouse/oidc/models/_core.py @@ -13,7 +13,7 @@ from __future__ import annotations from collections.abc import Callable -from typing import TYPE_CHECKING, Any, TypedDict, TypeVar, Unpack, cast +from typing import TYPE_CHECKING, Any, TypedDict, TypeVar, Unpack import sentry_sdk @@ -87,11 +87,11 @@ def check_existing_jti( _all_signed_claims, **kwargs: Unpack[CheckNamedArguments], ) -> bool: + """Returns True if the checks passes or raises an exception.""" - publisher_service: OIDCPublisherService = cast( - OIDCPublisherService, kwargs.get("publisher_service") - ) - if publisher_service.token_identifier_exists(signed_claim): + publisher_service: OIDCPublisherService = kwargs["publisher_service"] + + if publisher_service.jwt_identifier_exists(signed_claim): publisher_service.metrics.increment( "warehouse.oidc.reused_token", tags=[f"publisher:{publisher_service.publisher}"], diff --git a/warehouse/oidc/models/github.py b/warehouse/oidc/models/github.py index aaeccaef26c2..f3cc7016733a 100644 --- a/warehouse/oidc/models/github.py +++ b/warehouse/oidc/models/github.py @@ -28,7 +28,6 @@ from warehouse.oidc.models._core import ( CheckClaimCallable, OIDCPublisher, - OIDCPublisherMixin, PendingOIDCPublisher, check_claim_binary, check_existing_jti, @@ -138,6 +137,7 @@ class GitHubPublisherMixin: "repository_owner": check_claim_binary(str.__eq__), "repository_owner_id": check_claim_binary(str.__eq__), "job_workflow_ref": _check_job_workflow_ref, + "jti": check_existing_jti, } __required_unverifiable_claims__: set[str] = {"ref", "sha"} @@ -240,9 +240,9 @@ def sub(self): return f"repo:{self.repository}" @property - def jti(self): + def jti(self) -> str: """Placeholder value for JTI.""" - return True + return "placeholder" def publisher_url(self, claims=None): base = f"https://github.com/{self.repository}" @@ -306,13 +306,6 @@ class GitHubPublisher(GitHubPublisherMixin, OIDCPublisher): ), ) - __required_verifiable_claims__ = ( - GitHubPublisherMixin.__required_verifiable_claims__ - | { - "jti": check_existing_jti, - } - ) - id = mapped_column( UUID(as_uuid=True), ForeignKey(OIDCPublisher.id), primary_key=True ) @@ -335,10 +328,6 @@ class PendingGitHubPublisher(GitHubPublisherMixin, PendingOIDCPublisher): UUID(as_uuid=True), ForeignKey(PendingOIDCPublisher.id), primary_key=True ) - __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { - "jti", - } - def reify(self, session): """ Returns a `GitHubPublisher` for this `PendingGitHubPublisher`, diff --git a/warehouse/oidc/models/gitlab.py b/warehouse/oidc/models/gitlab.py index 1d1857d86571..4897b2ed4c45 100644 --- a/warehouse/oidc/models/gitlab.py +++ b/warehouse/oidc/models/gitlab.py @@ -22,7 +22,6 @@ from warehouse.oidc.models._core import ( CheckClaimCallable, OIDCPublisher, - OIDCPublisherMixin, PendingOIDCPublisher, check_existing_jti, ) @@ -120,6 +119,7 @@ class GitLabPublisherMixin: "sub": _check_sub, "project_path": _check_project_path, "ci_config_ref_uri": _check_ci_config_ref_uri, + "jti": check_existing_jti, } __required_unverifiable_claims__: set[str] = {"ref_path", "sha"} @@ -224,9 +224,9 @@ def publisher_name(self): return "GitLab" @property - def jti(self): + def jti(self) -> str: """Placeholder value for JTI.""" - return True + return "placeholder" def publisher_url(self, claims=None): base = f"https://gitlab.com/{self.project_path}" @@ -253,13 +253,6 @@ class GitLabPublisher(GitLabPublisherMixin, OIDCPublisher): ), ) - __required_verifiable_claims__ = ( - GitLabPublisherMixin.__required_verifiable_claims__ - | { - "jti": check_existing_jti, - } - ) - id = mapped_column( UUID(as_uuid=True), ForeignKey(OIDCPublisher.id), primary_key=True ) @@ -278,10 +271,6 @@ class PendingGitLabPublisher(GitLabPublisherMixin, PendingOIDCPublisher): ), ) - __preverified_claims__ = OIDCPublisherMixin.__preverified_claims__ | { - "jti", - } - id = mapped_column( UUID(as_uuid=True), ForeignKey(PendingOIDCPublisher.id), primary_key=True ) diff --git a/warehouse/oidc/services.py b/warehouse/oidc/services.py index e78d431b03ce..06628c8dd60b 100644 --- a/warehouse/oidc/services.py +++ b/warehouse/oidc/services.py @@ -79,7 +79,7 @@ def reify_pending_publisher(self, pending_publisher, project): project.oidc_publishers.append(new_publisher) return new_publisher - def token_identifier_exists(self, jti: str) -> bool: + def jwt_identifier_exists(self, jti: str) -> bool: """ The NullOIDCPublisherService does not provide a mechanism to store used tokens before their expiration. @@ -233,7 +233,7 @@ def _get_key_for_token(self, token): unverified_header = jwt.get_unverified_header(token) return self._get_key(unverified_header["kid"]) - def token_identifier_exists(self, jti: str) -> bool: + def jwt_identifier_exists(self, jti: str) -> bool: """ Check if a JWT Token Identifier has already been used. """ From 99b90e021286bb8e79f30e9197478cc0faba5d8c Mon Sep 17 00:00:00 2001 From: Alexis Date: Mon, 12 Aug 2024 10:17:22 +0200 Subject: [PATCH 15/15] Renmame jwt_token_identifier --- tests/unit/oidc/test_services.py | 6 +++--- warehouse/oidc/views.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/oidc/test_services.py b/tests/unit/oidc/test_services.py index 8737728730c1..f00f5003d4c9 100644 --- a/tests/unit/oidc/test_services.py +++ b/tests/unit/oidc/test_services.py @@ -761,10 +761,10 @@ def test_jwt_identifier_exists_find_duplicate( datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(minutes=15) ).timestamp() ) - jwt_token_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" - service.store_jwt_identifier(jwt_token_identifier, expiration=expiration) + jwt_identifier = "6e67b1cb-2b8d-4be5-91cb-757edb2ec970" + service.store_jwt_identifier(jwt_identifier, expiration=expiration) - assert service.jwt_identifier_exists(jwt_token_identifier) is True + assert service.jwt_identifier_exists(jwt_identifier) is True class TestNullOIDCPublisherService: diff --git a/warehouse/oidc/views.py b/warehouse/oidc/views.py index 3f05dc5a6026..84ba6bd5c68e 100644 --- a/warehouse/oidc/views.py +++ b/warehouse/oidc/views.py @@ -288,9 +288,9 @@ def mint_token( # We have used the given JWT to mint a new token. Let now store it to prevent # its reuse if the claims contain a JTI. Of note, exp is coming from a trusted # source here, so we don't validate it - if jwt_token_identifier := claims.get("jti"): + if jwt_identifier := claims.get("jti"): expiration = cast(int, claims.get("exp")) - oidc_service.store_jwt_identifier(jwt_token_identifier, expiration) + oidc_service.store_jwt_identifier(jwt_identifier, expiration) for project in publisher.projects: project.record_event(