Skip to content

Commit a7c3920

Browse files
committed
Validate issuer/URL by scheme and host instead of string prefix
is_issuer_known accepted any issuer that started with the expected string "https://ci.eclipse.org". A string-prefix check treats the issuer as opaque text, so URLs that only begin with those characters but resolve to a different host were accepted -- e.g. a userinfo form ("https://ci.eclipse.org@other.host") or a suffix form ("https://ci.eclipse.org.other.host"). The issuer is then used for OIDC discovery, so it must genuinely point at the expected host. Match issuers/URLs on their parsed scheme and host (f"{scheme}://{hostname}") against a base-URL constant: - models.is_issuer_known: compare against JENKINS_ISSUER_BASE_URL, mirroring how the GitHub issuer is matched against GITHUB_ISSUER. - cli add-workload: apply the same scheme+host comparison to both the GitHub (GITHUB_BASE_URL) and Jenkins (JENKINS_ISSUER_BASE_URL) cases, replacing the bare netloc equality and startswith checks. This also enforces the https scheme on the GitHub repo URL. Rename the constant JENKINS_ISSUER_PREFIX to JENKINS_ISSUER_BASE_URL and add GITHUB_BASE_URL. Add tests for the userinfo/suffix host constructions, non-https and malformed issuers, and the disguised/non-https URLs in the CLI (asserting no network call or DB write happens for a rejected URL). Update the authentication flow in DESIGN.md to describe host-based validation. Signed-off-by: Lukas Puehringer <lukas.puehringer@eclipse-foundation.org> Assisted-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c8f57af commit a7c3920

5 files changed

Lines changed: 79 additions & 12 deletions

File tree

docs/DESIGN.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,8 @@ sequenceDiagram
154154
2. Extract issuer
155155
3. Verify issuer is known
156156
- GitHub: Full match: `https://token.actions.githubusercontent.com`
157-
- Jenkins: Prefix match: `https://ci.eclipse.org`
157+
- Jenkins: Parse the issuer as a URL and require an `https` scheme with
158+
the host exactly equal to `ci.eclipse.org`
158159
3. Token Key Discovery
159160
1. Fetch OIDC configuration from `{issuer}/.well-known/openid-configuration`
160161
2. Extract `jwks_uri` from configuration

pia/cli.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@
4242
from sqlalchemy.orm import Session, sessionmaker
4343

4444
from .models import (
45-
JENKINS_ISSUER_PREFIX,
45+
GITHUB_BASE_URL,
46+
JENKINS_ISSUER_BASE_URL,
4647
DependencyTrackProject,
4748
EclipseFoundationProject,
4849
GitHubWorkload,
@@ -130,11 +131,12 @@ def add_workload(ef_project_id: str, url: str, dry_run: bool) -> None:
130131
"""Register a GitHub or Jenkins workload for an Eclipse Foundation project.
131132
132133
URL type is determined by its value: github.com URLs create a GitHubWorkload,
133-
URLs starting with the Jenkins issuer prefix create a JenkinsWorkload with the
134-
URL as issuer. Any other URL is rejected.
134+
ci.eclipse.org URLs create a JenkinsWorkload with the URL as issuer. Both are
135+
matched on the URL's scheme and host. Any other URL is rejected.
135136
"""
136137
parsed = urlparse(url)
137-
if parsed.netloc == "github.com":
138+
base_url = f"{parsed.scheme}://{parsed.hostname}"
139+
if base_url == GITHUB_BASE_URL:
138140
path_parts = parsed.path.strip("/").split("/")
139141
if len(path_parts) != 2 or not all(path_parts):
140142
raise click.ClickException(f"GitHub URL must include owner/repo: {url}")
@@ -150,15 +152,15 @@ def add_workload(ef_project_id: str, url: str, dry_run: bool) -> None:
150152
f"Prepared GitHubWorkload(ef_project_id={ef_project_id!r}, "
151153
f"repo_owner={owner!r}, repo_name={repo!r}, repo_owner_id={owner_id})"
152154
)
153-
elif url.startswith(JENKINS_ISSUER_PREFIX):
155+
elif base_url == JENKINS_ISSUER_BASE_URL:
154156
workload = JenkinsWorkload(ef_project_id=ef_project_id, issuer=url)
155157
logger.info(
156158
f"Prepared JenkinsWorkload(ef_project_id={ef_project_id!r}, issuer={url!r})"
157159
)
158160
else:
159161
raise click.ClickException(
160-
f"URL must be a GitHub repo URL or start with {JENKINS_ISSUER_PREFIX!r}: "
161-
f"{url}"
162+
f"URL must be a {GITHUB_BASE_URL} repo URL or a "
163+
f"{JENKINS_ISSUER_BASE_URL} issuer URL: {url}"
162164
)
163165

164166
with _make_session() as session:

pia/models.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import logging
44
from typing import Any
5+
from urllib.parse import urlparse
56

67
from pydantic import BaseModel, ConfigDict, Field, HttpUrl
78
from sqlalchemy import ForeignKey, Select, String, UniqueConstraint, select
@@ -13,8 +14,11 @@
1314
GITHUB_ISSUER = "https://token.actions.githubusercontent.com"
1415
"""OIDC issuer for GitHub Actions tokens. Constant across all GitHub workloads."""
1516

16-
JENKINS_ISSUER_PREFIX = "https://ci.eclipse.org"
17-
"""Prefix used for early validation of Jenkins issuer URLs."""
17+
GITHUB_BASE_URL = "https://github.com"
18+
"""Scheme and host of the GitHub repo URLs accepted when registering workloads."""
19+
20+
JENKINS_ISSUER_BASE_URL = "https://ci.eclipse.org"
21+
"""Scheme and host every Jenkins issuer URL must have."""
1822

1923
GITHUB_ALLOWED_EVENT_NAMES = frozenset({"push", "workflow_dispatch"})
2024
"""Allowed values for the GitHub OIDC token's `event_name` claim.
@@ -131,12 +135,14 @@ def is_issuer_known(issuer: str) -> bool:
131135
"""Check if issuer is generally known to PIA.
132136
133137
GitHub: issuer must equal GITHUB_ISSUER
134-
Jenkins: issuer must start with JENKINS_ISSUER_PREFIX
138+
Jenkins: issuer's scheme and host must equal JENKINS_ISSUER_BASE_URL
139+
135140
"""
136141
if issuer == GITHUB_ISSUER:
137142
return True
138143

139-
return issuer.startswith(JENKINS_ISSUER_PREFIX)
144+
parsed = urlparse(issuer)
145+
return f"{parsed.scheme}://{parsed.hostname}" == JENKINS_ISSUER_BASE_URL
140146

141147

142148
def find_workload_by_claims(

tests/test_cli.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,38 @@ def test_add_workload_invalid_github_url(runner):
141141
assert "owner/repo" in result.output
142142

143143

144+
@pytest.mark.parametrize(
145+
"url",
146+
[
147+
# GitHub host disguised via userinfo / suffix, or wrong scheme.
148+
"https://github.com@evil.example/eclipse-foo/repo",
149+
"https://github.qkg1.top.evil.example/eclipse-foo/repo",
150+
"http://github.com/eclipse-foo/repo",
151+
# Jenkins host disguised via userinfo / suffix, or wrong scheme.
152+
"https://ci.eclipse.org@evil.example/eclipse-bar/oidc",
153+
"https://ci.eclipse.org.evil.example/eclipse-bar/oidc",
154+
"http://ci.eclipse.org/eclipse-bar/oidc",
155+
],
156+
)
157+
def test_add_workload_rejects_disguised_or_non_https_host(
158+
runner, session_factory, monkeypatch, url
159+
):
160+
# A disguised host must be rejected during URL validation, before any
161+
# network call (e.g. resolving the GitHub owner id) or DB write.
162+
def fail(*a, **kw):
163+
raise AssertionError("network must not be touched for a rejected URL")
164+
165+
monkeypatch.setattr(cli_module.requests, "get", fail)
166+
167+
result = runner.invoke(cli_module.cli, ["add-workload", "eclipse-foo", url])
168+
169+
assert result.exit_code != 0
170+
assert "URL must be" in result.output
171+
with session_factory() as s:
172+
assert s.query(GitHubWorkload).count() == 0
173+
assert s.query(JenkinsWorkload).count() == 0
174+
175+
144176
def test_missing_database_url_fails_early(runner, monkeypatch):
145177
"""Without PIA_DATABASE_URL the cli group exits before any subcommand runs."""
146178
monkeypatch.delenv("PIA_DATABASE_URL", raising=False)

tests/test_models.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,35 @@ def test_github_issuer_known(self):
2424
def test_jenkins_issuer_known(self):
2525
assert is_issuer_known("https://ci.eclipse.org/eclipse-other/oidc")
2626

27+
def test_jenkins_issuer_bare_host_known(self):
28+
assert is_issuer_known("https://ci.eclipse.org")
29+
2730
def test_arbitrary_issuer_unknown(self):
2831
assert not is_issuer_known("https://attacker.com")
2932

33+
@pytest.mark.parametrize(
34+
"issuer",
35+
[
36+
# Userinfo before "@": the real host is other.host, not ci.eclipse.org.
37+
"https://ci.eclipse.org@other.host",
38+
"https://ci.eclipse.org@127.0.0.1:8888",
39+
# Suffix on the host: ci.eclipse.org.other.host is a different host.
40+
"https://ci.eclipse.org.other.host",
41+
"https://ci.eclipse.org.other.host/eclipse-x/oidc",
42+
# Host is only a prefix substring, not the whole host.
43+
"https://ci.eclipse.org.evil.example/.well-known/openid-configuration",
44+
],
45+
)
46+
def test_issuer_resolving_to_other_host_unknown(self, issuer):
47+
assert not is_issuer_known(issuer)
48+
49+
def test_non_https_jenkins_issuer_unknown(self):
50+
assert not is_issuer_known("http://ci.eclipse.org/eclipse-other/oidc")
51+
52+
def test_malformed_issuer_unknown(self):
53+
assert not is_issuer_known("ci.eclipse.org")
54+
assert not is_issuer_known("not a url")
55+
3056

3157
class TestFindWorkloadByClaims:
3258
def test_github_match(self, seed_db):

0 commit comments

Comments
 (0)