Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions tests/unit/oidc/models/test_gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
),
("gitlab.com/foo/bar//a.yml@/some/ref", "a.yml"),
("gitlab.com/foo/bar//a/b.yml@/some/ref", "a/b.yml"),
# Custom domain.
("gitlab.example.com/foo/bar//example.yml@/some/ref", "example.yml"),
# Malformed `ci_config_ref_uri`s.
("gitlab.com/foo/bar//notnested.wrongsuffix@/some/ref", None),
("gitlab.com/foo/bar//@/some/ref", None),
Expand All @@ -59,6 +61,7 @@ class TestGitLabPublisher:
@pytest.mark.parametrize("environment", [None, "some_environment"])
def test_lookup_fails_invalid_ci_config_ref_uri(self, environment):
signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar",
"ci_config_ref_uri": ("gitlab.com/foo/bar//example/.yml@refs/heads/main"),
}
Expand All @@ -84,6 +87,7 @@ def test_lookup_succeeds_with_mixed_case_project_path(self, db_request):
)

signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar", # different case than stored publisher
"ci_config_ref_uri": ("gitlab.com/foo/bar//.gitlab-ci.yml@refs/heads/main"),
"environment": "some_environment",
Expand Down Expand Up @@ -112,6 +116,7 @@ def test_lookup_succeeds_with_non_lowercase_environment(
)

signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar",
"ci_config_ref_uri": ("gitlab.com/foo/bar//.gitlab-ci.yml@refs/heads/main"),
"environment": environment,
Expand Down Expand Up @@ -140,6 +145,7 @@ def test_lookup_is_case_sensitive_for_environment(self, db_request, environment)
)

signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar",
"ci_config_ref_uri": ("gitlab.com/foo/bar//.gitlab-ci.yml@refs/heads/main"),
"environment": environment,
Expand Down Expand Up @@ -177,6 +183,7 @@ def test_lookup_escapes(

for workflow_filepath in (workflow_filepath_a, workflow_filepath_b):
signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar",
"ci_config_ref_uri": (
f"gitlab.com/foo/bar//{workflow_filepath}@refs/heads/main"
Expand All @@ -195,6 +202,7 @@ def test_lookup_escapes(

def test_lookup_no_matching_publisher(self, db_request):
signed_claims = {
"iss": "https://gitlab.com",
"project_path": "foo/bar",
"ci_config_ref_uri": ("gitlab.com/foo/bar//.gitlab-ci.yml@refs/heads/main"),
}
Expand Down Expand Up @@ -255,6 +263,7 @@ def test_gitlab_publisher_computed_properties(self):
namespace="fakeowner",
workflow_filepath="subfolder/fakeworkflow.yml",
environment="fakeenv",
issuer_url="https://gitlab.com",
)

for claim_name in publisher.__required_verifiable_claims__.keys():
Expand Down Expand Up @@ -342,6 +351,7 @@ def test_gitlab_publisher_missing_claims(self, monkeypatch, missing):
project="fakerepo",
namespace="fakeowner",
workflow_filepath="subfolder/fakeworkflow.yml",
issuer_url="https://gitlab.com",
)

scope = pretend.stub()
Expand Down Expand Up @@ -377,6 +387,7 @@ def test_gitlab_publisher_missing_optional_claims(self, monkeypatch):
namespace="fakeowner",
workflow_filepath="subfolder/fakeworkflow.yml",
environment="some-environment", # The optional claim that should be present
issuer_url="https://gitlab.com",
)

sentry_sdk = pretend.stub(capture_message=pretend.call_recorder(lambda s: None))
Expand Down Expand Up @@ -412,6 +423,7 @@ def test_gitlab_publisher_verifies(self, monkeypatch, environment, missing_claim
namespace="fakeowner",
workflow_filepath="subfolder/fakeworkflow.yml",
environment="environment",
issuer_url="https://gitlab.com",
)

noop_check = pretend.call_recorder(lambda gt, sc, ac, **kwargs: True)
Expand Down Expand Up @@ -644,6 +656,7 @@ def test_gitlab_publisher_ci_config_ref_uri(
project="bar",
namespace="foo",
workflow_filepath="workflows/baz.yml",
issuer_url="https://gitlab.com",
)

check = gitlab.GitLabPublisher.__required_verifiable_claims__[
Expand Down Expand Up @@ -827,6 +840,7 @@ def test_gitlab_publisher_verify_url(
namespace=namespace,
workflow_filepath="workflow_filename.yml",
environment="",
issuer_url="https://gitlab.com",
)
assert publisher.verify_url(url) == expected

Expand All @@ -837,6 +851,7 @@ def test_gitlab_publisher_attestation_identity(self, environment):
namespace="group/subgroup",
workflow_filepath="workflow_filename.yml",
environment=environment,
issuer_url="https://gitlab.com",
)

identity = publisher.attestation_identity
Expand Down
49 changes: 48 additions & 1 deletion tests/unit/oidc/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
GitLabPublisherFactory,
GooglePublisherFactory,
)
from tests.common.db.organizations import OrganizationOIDCIssuerFactory
from warehouse.oidc import errors, utils
from warehouse.oidc.models import (
ActiveStatePublisher,
Expand All @@ -21,13 +22,16 @@
GooglePublisher,
)
from warehouse.oidc.utils import OIDC_PUBLISHER_CLASSES
from warehouse.organizations.models import OIDCIssuerType
from warehouse.utils.security_policy import principals_for


def test_find_publisher_by_issuer_bad_issuer_url():
session = pretend.stub(scalar=lambda *stmt: None)

with pytest.raises(errors.InvalidPublisherError):
utils.find_publisher_by_issuer(
pretend.stub(), "https://fake-issuer.url", pretend.stub()
session, "https://fake-issuer.url", pretend.stub()
)


Expand Down Expand Up @@ -140,6 +144,7 @@ def test_find_publisher_by_issuer_gitlab(db_request, environment, expected_id):

signed_claims.update(
{
"iss": utils.GITLAB_OIDC_ISSUER_URL,
"project_path": "foo/bar",
"ci_config_ref_uri": "gitlab.com/foo/bar//workflows/ci.yml@refs/heads/main",
}
Expand Down Expand Up @@ -318,3 +323,45 @@ def test_oidc_maps_consistent():
# The class mapping for pending and non-pending publisher models
# should be distinct.
assert class_map[True] != class_map[False]


def test_find_publisher_by_issuer_with_custom_issuer(db_request):
"""
A custom OIDC issuer URL is properly resolved to a concrete publisher.
"""
# Create organization and register a custom GitLab issuer URL
custom_issuer_url = "https://gitlab.custom-company.com"
OrganizationOIDCIssuerFactory.create(
issuer_type=OIDCIssuerType.GitLab,
issuer_url=custom_issuer_url,
)

# Create a GitLab publisher that would match the claims
publisher = GitLabPublisherFactory(
namespace="foo",
project="bar",
workflow_filepath="workflows/ci.yml",
environment="",
issuer_url=custom_issuer_url,
)

# Create signed claims that would come from the custom GitLab instance
signed_claims = {
claim_name: "fake" for claim_name in GitLabPublisher.all_known_claims()
}
signed_claims.update(
{
"iss": custom_issuer_url,
"project_path": "foo/bar",
"ci_config_ref_uri": "gitlab.custom-company.com/foo/bar//workflows/ci.yml@refs/heads/main", # noqa: E501
}
)

# This should successfully resolve the custom issuer URL to the GitLab publisher
result = utils.find_publisher_by_issuer(
db_request.db,
utils.GITLAB_OIDC_ISSUER_URL,
signed_claims,
)

assert result.id == publisher.id
4 changes: 3 additions & 1 deletion tests/unit/oidc/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,11 +215,13 @@ def find_service(self, *a, **kw):
assert err["description"] == "malformed JWT"


def test_mint_token_from_oidc_unknown_issuer():
def test_mint_token_from_oidc_unknown_issuer(metrics):
class Request:
def __init__(self):
self.response = pretend.stub(status=None)
self.flags = pretend.stub(disallow_oidc=lambda *a: False)
self.db = pretend.stub(scalar=lambda *stmt: None)
self.metrics = metrics

@property
def body(self):
Expand Down
16 changes: 12 additions & 4 deletions warehouse/oidc/models/gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def _check_ci_config_ref_uri(
**_kwargs,
) -> bool:
# We expect a string formatted as follows:
# gitlab.com/OWNER/REPO//WORKFLOW_PATH/WORKFLOW_FILE.yml@REF
# <gitlab.com>/OWNER/REPO//WORKFLOW_PATH/WORKFLOW_FILE.yml@REF
# where REF is the value of the `ref_path` claim.

# Defensive: GitLab should never give us an empty ci_config_ref_uri,
Expand Down Expand Up @@ -234,6 +234,7 @@ def _get_publisher_for_environment(

@classmethod
def lookup_by_claims(cls, session: Session, signed_claims: SignedClaims) -> Self:
issuer_url = signed_claims["iss"]
project_path = signed_claims["project_path"]
ci_config_ref_uri = signed_claims["ci_config_ref_uri"]
namespace, project = project_path.rsplit("/", 1)
Expand All @@ -249,6 +250,7 @@ def lookup_by_claims(cls, session: Session, signed_claims: SignedClaims) -> Self
func.lower(cls.namespace) == namespace,
func.lower(cls.project) == project,
cls.workflow_filepath == workflow_filepath,
cls.issuer_url == issuer_url,
)
publishers = query.with_session(session).all()
if publisher := cls._get_publisher_for_environment(publishers, environment):
Expand All @@ -266,15 +268,18 @@ def sub(self) -> str:

@property
def ci_config_ref_uri(self) -> str:
return f"gitlab.com/{self.project_path}//{self.workflow_filepath}"
# Extract domain from issuer_url (remove https:// prefix)
domain = self.issuer_url.removeprefix("https://")
return f"{domain}/{self.project_path}//{self.workflow_filepath}"

@property
def publisher_name(self) -> str:
return "GitLab"

@property
def publisher_base_url(self) -> str:
return f"https://gitlab.com/{self.project_path}"
# Use the issuer_url which already includes the scheme (https://)
return f"{self.issuer_url}/{self.project_path}"

@property
def jti(self) -> str:
Expand Down Expand Up @@ -430,6 +435,8 @@ def reify(self, session: Session) -> GitLabPublisher:
GitLabPublisher.project == self.project,
GitLabPublisher.workflow_filepath == self.workflow_filepath,
GitLabPublisher.environment == self.environment,
# TODO (glsm): Implement custom issuers
# GitLabPublisher.issuer_url == self.issuer_url,
)
.one_or_none()
)
Expand All @@ -439,7 +446,8 @@ def reify(self, session: Session) -> GitLabPublisher:
project=self.project,
workflow_filepath=self.workflow_filepath,
environment=self.environment,
issuer_url=GITLAB_OIDC_ISSUER_URL,
# TODO (glsm): Implement custom issuers
# issuer_url=self.issuer_url,
)

session.delete(self)
Expand Down
2 changes: 1 addition & 1 deletion warehouse/oidc/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def verify_jwt_signature(

def find_publisher(
self, signed_claims: SignedClaims, *, pending: bool = False
) -> OIDCPublisher | PendingOIDCPublisher | None:
) -> OIDCPublisher | PendingOIDCPublisher:
# NOTE: We do NOT verify the claims against the publisher, since this
# service is for development purposes only.
return find_publisher_by_issuer(
Expand Down
18 changes: 17 additions & 1 deletion warehouse/oidc/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from dataclasses import dataclass

from pyramid.authorization import Authenticated
from sqlalchemy import select

from warehouse.admin.flags import AdminFlagValue
from warehouse.oidc.errors import InvalidPublisherError
Expand All @@ -27,10 +28,13 @@
PendingGooglePublisher,
PendingOIDCPublisher,
)
from warehouse.organizations.models import OrganizationOIDCIssuer

if typing.TYPE_CHECKING:
from sqlalchemy.orm import Session

from warehouse.organizations.models import OIDCIssuerType


OIDC_ISSUER_SERVICE_NAMES = {
GITHUB_OIDC_ISSUER_URL: "github",
Expand Down Expand Up @@ -66,6 +70,18 @@
}


def lookup_custom_issuer_type(
session: Session, issuer_url: str
) -> OIDCIssuerType | None:
"""
Look up the issuer type for an Organization's OIDC issuer URL.
"""
stmt = select(OrganizationOIDCIssuer.issuer_type).where(
OrganizationOIDCIssuer.issuer_url == issuer_url
)
return session.scalar(stmt)


def find_publisher_by_issuer(
session: Session,
issuer_url: str,
Expand All @@ -79,7 +95,7 @@ def find_publisher_by_issuer(
to one or more projects or a `PendingOIDCPublisher`, varying with the
`pending` parameter.

Returns `None` if no publisher can be found.
Raises if no publisher can be found.
"""

try:
Expand Down
16 changes: 14 additions & 2 deletions warehouse/oidc/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@
from warehouse.oidc.models import GitHubPublisher, OIDCPublisher, PendingOIDCPublisher
from warehouse.oidc.models.gitlab import GitLabPublisher
from warehouse.oidc.services import OIDCPublisherService
from warehouse.oidc.utils import OIDC_ISSUER_ADMIN_FLAGS, OIDC_ISSUER_SERVICE_NAMES
from warehouse.oidc.utils import (
OIDC_ISSUER_ADMIN_FLAGS,
OIDC_ISSUER_SERVICE_NAMES,
lookup_custom_issuer_type,
)
from warehouse.packaging.interfaces import IProjectService
from warehouse.packaging.models import ProjectFactory
from warehouse.rate_limiting.interfaces import IRateLimiter
Expand Down Expand Up @@ -135,8 +139,16 @@ def mint_token_from_oidc(request: Request):
)

# Associate the given issuer claim with Warehouse's OIDCPublisherService.
# First, try the standard issuers
service_name = OIDC_ISSUER_SERVICE_NAMES.get(unverified_issuer)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nobody ever uses a custom issuer, there's no lookup penalty, since it'll match the static dictionary value. This should be the case for most lookups.

# If not in global mapping, check for organization-specific custom issuer
if not service_name:
service_name = lookup_custom_issuer_type(request.db, unverified_issuer)
if not service_name:
request.metrics.increment(
"warehouse.oidc.mint_token_from_oidc.unknown_issuer",
tags={"issuer_url": unverified_issuer},
)
return _invalid(
errors=[
{
Expand All @@ -147,7 +159,7 @@ def mint_token_from_oidc(request: Request):
request=request,
)

if request.flags.disallow_oidc(OIDC_ISSUER_ADMIN_FLAGS[unverified_issuer]):
if request.flags.disallow_oidc(OIDC_ISSUER_ADMIN_FLAGS.get(unverified_issuer)):
return _invalid(
errors=[
{
Expand Down