feat: New tests for image signing and split infra tests to smaller ones#1222
Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom Mar 16, 2026
Merged
feat: New tests for image signing and split infra tests to smaller ones#1222dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/hold', '/cherry-pick', '/lgtm', '/verified', '/wip'} |
dbasunag
force-pushed
the
signing_image_tests
branch
from
a703f77 to
157ee1f
Compare
Collaborator
Author
|
/build-push-pr-image |
Contributor
📝 WalkthroughWalkthroughThis pull request extends model signing infrastructure to support OCI image signing and verification workflows. Changes include adding cosign and skopeo to Dockerfile dependencies, introducing OCI registry test fixtures and restructuring test suites into granular classes with explicit dependency ordering, defining new signing constants, updating namespace references, and implementing a new Route resource class for OpenShift configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
Status of building tag pr-1222: success. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/model_registry/model_registry/python_client/signing/conftest.py (2)
406-423:⚠️ Potential issue | 🟠 MajorUse a fresh download directory instead of
${tmp_base_dir}/model.This fixture reuses a stable path and does not clear it before
snapshot_download(). A stalemodel.sigor partial download from a previous run can maketest_model_signpass without producing a new signature in this run.🔧 Remediation
-@pytest.fixture(scope="class") -def downloaded_model_dir() -> Path: +@pytest.fixture(scope="class") +def downloaded_model_dir(tmp_path_factory) -> Path: """Download a test model from Hugging Face to a temporary directory. @@ - model_dir = Path(py_config["tmp_base_dir"]) / "model" - model_dir.mkdir(exist_ok=True) + model_dir = tmp_path_factory.mktemp("model")As per coding guidelines,
**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around lines 406 - 423, The downloaded_model_dir fixture reuses a stable path (Path(py_config["tmp_base_dir"]) / "model") which can leave stale files (e.g., model.sig) between runs; change downloaded_model_dir to create a fresh temporary directory per test (use tempfile.mkdtemp or tempfile.TemporaryDirectory or append a UUID/timestamp) instead of reusing "model", ensure the directory is empty before calling snapshot_download(repo_id=..., local_dir=...), yield the fresh Path from the fixture (or return after cleanup) and optionally remove the temp dir in teardown so each invocation of downloaded_model_dir produces a clean download directory for test_model_sign and related tests.
426-458:⚠️ Potential issue | 🟠 MajorDo not leave the minted service-account token and mutated env behind (CWE-922).
Medium severity.
set_environment_variableswrites a live cluster token to disk and teardown only removes the env var names; it does not delete the token file or restore any preexistingSIGSTORE_*/ROOT_*values.copied_model_to_oci_registryalso unconditionally dropsCOSIGN_ALLOW_INSECURE_REGISTRY. In a shared CI worker, another test or process can reuse the leftover token, and later tests can lose their original env configuration.🔧 Remediation
`@pytest.fixture`(scope="class") def set_environment_variables(securesign_instance: Securesign) -> Generator[None, Any]: @@ - os.environ["IDENTITY_TOKEN_PATH"] = generate_token(temp_base_folder=py_config["tmp_base_dir"]) + managed_vars = [ + "IDENTITY_TOKEN_PATH", + "SIGSTORE_TUF_URL", + "SIGSTORE_FULCIO_URL", + "SIGSTORE_REKOR_URL", + "SIGSTORE_TSA_URL", + "ROOT_CHECKSUM", + "ROOT_URL", + ] + original_env = {name: os.environ.get(name) for name in managed_vars} + token_path = generate_token(temp_base_folder=py_config["tmp_base_dir"]) + os.environ["IDENTITY_TOKEN_PATH"] = token_path @@ LOGGER.info("Environment variables set for signing tests") yield # Clean up environment variables - for var_name in [ - "IDENTITY_TOKEN_PATH", - "SIGSTORE_TUF_URL", - "SIGSTORE_FULCIO_URL", - "SIGSTORE_REKOR_URL", - "SIGSTORE_TSA_URL", - "ROOT_CHECKSUM", - "ROOT_URL", - ]: - os.environ.pop(var_name, None) + if token_path and os.path.exists(token_path): + os.remove(token_path) + + for var_name, old_value in original_env.items(): + if old_value is None: + os.environ.pop(var_name, None) + else: + os.environ[var_name] = old_value`@pytest.fixture`(scope="class") def copied_model_to_oci_registry( @@ - os.environ["COSIGN_ALLOW_INSECURE_REGISTRY"] = "true" + previous_allow_insecure = os.environ.get("COSIGN_ALLOW_INSECURE_REGISTRY") + os.environ["COSIGN_ALLOW_INSECURE_REGISTRY"] = "true" @@ - os.environ.pop("COSIGN_ALLOW_INSECURE_REGISTRY", None) + if previous_allow_insecure is None: + os.environ.pop("COSIGN_ALLOW_INSECURE_REGISTRY", None) + else: + os.environ["COSIGN_ALLOW_INSECURE_REGISTRY"] = previous_allow_insecureAlso harden
generate_token()to create the token file with owner-only permissions.As per coding guidelines,
**: REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)Also applies to: 559-567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around lines 426 - 458, The fixture set_environment_variables leaves a live service-account token on disk and overwrites ENV without restoring previous values; change it to capture previous SIGSTORE_*/ROOT_* and COSIGN_ALLOW_INSECURE_REGISTRY values, save the token path returned from generate_token(temp_base_folder=...), set the token file to owner-only permissions (use generate_token or post-create chmod 0o600), and on teardown remove the token file (os.remove(token_path) if exists) and restore any preexisting env values (or unset if none); also ensure copied_model_to_oci_registry cleanup similarly removes COSIGN_ALLOW_INSECURE_REGISTRY and any created artifacts. Ensure references to generate_token, set_environment_variables, and copied_model_to_oci_registry are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 25-27: The Dockerfile RUN that downloads the cosign binary
currently writes remote content directly to /usr/bin/cosign without
verification; change the RUN that fetches the cosign release (the curl line that
writes /usr/bin/cosign for v2.4.2) to: 1) use curl --fail/-f so HTTP errors fail
the build, 2) download the corresponding checksum or signature artifact
(SHA256SUMS or cosign-linux-amd64.sum) from the same release, 3) verify the
downloaded binary using shasum/sha256sum (or verify the signature with gpg if a
signed checksum is provided), and 4) only install (move/chmod) the binary into
/usr/bin/cosign after checksum/signature verification succeeds; ensure the
verification step references the same release URL and aborts the build on
mismatch.
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 383-403: The ai_hub_oci_registry_host fixture returns
route.instance.spec.host immediately but OpenShift assigns the host
asynchronously; change ai_hub_oci_registry_host to wait until the Route is
admitted and the host is populated before returning. Poll the Route resource
created by ai_hub_oci_registry_route (use the Route instance object returned by
the Route context manager) and wait for instance.status.ingress[*].conditions
(or equivalent) to show an "Admitted" condition with status "True" and for
instance.spec.host to be non-empty, with a short timeout and retry/backoff;
return the host only after these checks pass so downstream tests don't get None
or unadmitted hosts.
In
`@tests/model_registry/model_registry/python_client/signing/test_signing_infrastructure.py`:
- Around line 125-138: The test is calling requests.get(...).json() for
tags_before and tags_after without checking HTTP status; update the calls that
build tags_before and tags_after (the requests.get against tags_url) to first
verify the response succeeded (e.g., check response.status_code or call
response.raise_for_status()) and log/raise a clear error with
response.status_code and response.text before attempting .json(), so failures
from the registry API are surfaced rather than causing JSON/key errors after
signer.sign_image.
---
Outside diff comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 406-423: The downloaded_model_dir fixture reuses a stable path
(Path(py_config["tmp_base_dir"]) / "model") which can leave stale files (e.g.,
model.sig) between runs; change downloaded_model_dir to create a fresh temporary
directory per test (use tempfile.mkdtemp or tempfile.TemporaryDirectory or
append a UUID/timestamp) instead of reusing "model", ensure the directory is
empty before calling snapshot_download(repo_id=..., local_dir=...), yield the
fresh Path from the fixture (or return after cleanup) and optionally remove the
temp dir in teardown so each invocation of downloaded_model_dir produces a clean
download directory for test_model_sign and related tests.
- Around line 426-458: The fixture set_environment_variables leaves a live
service-account token on disk and overwrites ENV without restoring previous
values; change it to capture previous SIGSTORE_*/ROOT_* and
COSIGN_ALLOW_INSECURE_REGISTRY values, save the token path returned from
generate_token(temp_base_folder=...), set the token file to owner-only
permissions (use generate_token or post-create chmod 0o600), and on teardown
remove the token file (os.remove(token_path) if exists) and restore any
preexisting env values (or unset if none); also ensure
copied_model_to_oci_registry cleanup similarly removes
COSIGN_ALLOW_INSECURE_REGISTRY and any created artifacts. Ensure references to
generate_token, set_environment_variables, and copied_model_to_oci_registry are
updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 43998286-07c9-48b6-9fea-1052f9726107
📒 Files selected for processing (6)
Dockerfiletests/model_registry/model_registry/python_client/signing/conftest.pytests/model_registry/model_registry/python_client/signing/constants.pytests/model_registry/model_registry/python_client/signing/test_signing_infrastructure.pytests/model_registry/model_registry/python_client/signing/utils.pyutilities/resources/route.py
Comment on lines
+25
to
+27
| # Install cosign | ||
| RUN curl -sSL "https://github.com/sigstore/cosign/releases/download/v2.4.2/cosign-linux-amd64" --output /usr/bin/cosign \ | ||
| && chmod +x /usr/bin/cosign |
Contributor
There was a problem hiding this comment.
Verify the downloaded cosign binary before installing it (CWE-494).
High severity. This curl -sSL writes whatever the remote endpoint returns straight into /usr/bin/cosign, and the build never verifies a checksum or even fails on HTTP 4xx/5xx. A compromised release asset, proxy, or transient GitHub error can bake arbitrary content into the image that CI later executes.
🔧 Remediation
+# Pin and verify the release asset before installing it.
+ARG COSIGN_VERSION=2.4.2
+ARG COSIGN_SHA256=<published-sha256>
-# Install cosign
-RUN curl -sSL "https://github.com/sigstore/cosign/releases/download/v2.4.2/cosign-linux-amd64" --output /usr/bin/cosign \
- && chmod +x /usr/bin/cosign
+# Install cosign
+RUN curl -fsSLo /tmp/cosign "https://github.com/sigstore/cosign/releases/download/v${COSIGN_VERSION}/cosign-linux-amd64" \
+ && echo "${COSIGN_SHA256} /tmp/cosign" | sha256sum -c - \
+ && install -m 0755 /tmp/cosign /usr/bin/cosign \
+ && rm -f /tmp/cosignAs per coding guidelines, **: REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 25 - 27, The Dockerfile RUN that downloads the
cosign binary currently writes remote content directly to /usr/bin/cosign
without verification; change the RUN that fetches the cosign release (the curl
line that writes /usr/bin/cosign for v2.4.2) to: 1) use curl --fail/-f so HTTP
errors fail the build, 2) download the corresponding checksum or signature
artifact (SHA256SUMS or cosign-linux-amd64.sum) from the same release, 3) verify
the downloaded binary using shasum/sha256sum (or verify the signature with gpg
if a signed checksum is provided), and 4) only install (move/chmod) the binary
into /usr/bin/cosign after checksum/signature verification succeeds; ensure the
verification step references the same release URL and aborts the build on
mismatch.
Comment on lines
+383
to
+403
| @pytest.fixture(scope="class") | ||
| def ai_hub_oci_registry_route(admin_client: DynamicClient, oci_registry_service: Service) -> Generator[Route, Any]: | ||
| """Override the default Route with edge TLS termination. | ||
|
|
||
| Cosign requires HTTPS. Edge termination lets the OpenShift router handle TLS | ||
| and forward plain HTTP to the Zot backend. | ||
| """ | ||
| with Route( | ||
| client=admin_client, | ||
| name=OCIRegistry.Metadata.NAME, | ||
| namespace=oci_registry_service.namespace, | ||
| to={"kind": "Service", "name": oci_registry_service.name}, | ||
| tls={"termination": "edge", "insecureEdgeTerminationPolicy": "Redirect"}, | ||
| ) as oci_route: | ||
| yield oci_route | ||
|
|
||
|
|
||
| @pytest.fixture(scope="class") | ||
| def ai_hub_oci_registry_host(ai_hub_oci_registry_route: Route) -> str: | ||
| """Get the OCI registry host from the route.""" | ||
| return ai_hub_oci_registry_route.instance.spec.host |
Contributor
There was a problem hiding this comment.
Wait for the Route to be admitted before returning its host.
ai_hub_oci_registry_host reads instance.spec.host immediately after creating the Route. That value is assigned asynchronously by OpenShift, so the next fixture can intermittently build https://None/v2/ or start probing a host that is not admitted yet.
As per coding guidelines, **: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around
lines 383 - 403, The ai_hub_oci_registry_host fixture returns
route.instance.spec.host immediately but OpenShift assigns the host
asynchronously; change ai_hub_oci_registry_host to wait until the Route is
admitted and the host is populated before returning. Poll the Route resource
created by ai_hub_oci_registry_route (use the Route instance object returned by
the Route context manager) and wait for instance.status.ingress[*].conditions
(or equivalent) to show an "Admitted" condition with status "True" and for
instance.spec.host to be non-empty, with a short timeout and retry/backoff;
return the host only after these checks pass so downstream tests don't get None
or unadmitted hosts.
Comment on lines
+125
to
+138
| registry_url = f"https://{ai_hub_oci_registry_host}" | ||
| tags_url = f"{registry_url}/v2/{SIGNING_OCI_REPO_NAME}/tags/list" | ||
|
|
||
| tags_before = requests.get(tags_url, verify=False, timeout=10).json() | ||
| LOGGER.info(f"Tags before signing: {json.dumps(tags_before, indent=2)}") | ||
| assert tags_before["tags"] == ["latest"], ( | ||
| f"Expected only ['latest'] tag before signing, got: {tags_before['tags']}" | ||
| ) | ||
|
|
||
| signer.sign_image(image=str(copied_model_to_oci_registry)) | ||
| LOGGER.info("Model signed successfully") | ||
|
|
||
| tags_after = requests.get(tags_url, verify=False, timeout=10).json() | ||
| LOGGER.info(f"Tags after signing: {json.dumps(tags_after, indent=2)}") |
Contributor
There was a problem hiding this comment.
Raise on registry API failures before calling .json().
Both tag lookups parse JSON without checking the HTTP status first. When the route returns a 4xx/5xx or an HTML error page, the test fails with a JSON/key error and hides the actual registry failure.
🔧 Remediation
- tags_before = requests.get(tags_url, verify=False, timeout=10).json()
+ before_response = requests.get(tags_url, verify=False, timeout=10)
+ before_response.raise_for_status()
+ tags_before = before_response.json()
@@
- tags_after = requests.get(tags_url, verify=False, timeout=10).json()
+ after_response = requests.get(tags_url, verify=False, timeout=10)
+ after_response.raise_for_status()
+ tags_after = after_response.json()As per coding guidelines, **: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| registry_url = f"https://{ai_hub_oci_registry_host}" | |
| tags_url = f"{registry_url}/v2/{SIGNING_OCI_REPO_NAME}/tags/list" | |
| tags_before = requests.get(tags_url, verify=False, timeout=10).json() | |
| LOGGER.info(f"Tags before signing: {json.dumps(tags_before, indent=2)}") | |
| assert tags_before["tags"] == ["latest"], ( | |
| f"Expected only ['latest'] tag before signing, got: {tags_before['tags']}" | |
| ) | |
| signer.sign_image(image=str(copied_model_to_oci_registry)) | |
| LOGGER.info("Model signed successfully") | |
| tags_after = requests.get(tags_url, verify=False, timeout=10).json() | |
| LOGGER.info(f"Tags after signing: {json.dumps(tags_after, indent=2)}") | |
| registry_url = f"https://{ai_hub_oci_registry_host}" | |
| tags_url = f"{registry_url}/v2/{SIGNING_OCI_REPO_NAME}/tags/list" | |
| before_response = requests.get(tags_url, verify=False, timeout=10) | |
| before_response.raise_for_status() | |
| tags_before = before_response.json() | |
| LOGGER.info(f"Tags before signing: {json.dumps(tags_before, indent=2)}") | |
| assert tags_before["tags"] == ["latest"], ( | |
| f"Expected only ['latest'] tag before signing, got: {tags_before['tags']}" | |
| ) | |
| signer.sign_image(image=str(copied_model_to_oci_registry)) | |
| LOGGER.info("Model signed successfully") | |
| after_response = requests.get(tags_url, verify=False, timeout=10) | |
| after_response.raise_for_status() | |
| tags_after = after_response.json() | |
| LOGGER.info(f"Tags after signing: {json.dumps(tags_after, indent=2)}") |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 128-128: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
[error] 137-137: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/model_registry/model_registry/python_client/signing/test_signing_infrastructure.py`
around lines 125 - 138, The test is calling requests.get(...).json() for
tags_before and tags_after without checking HTTP status; update the calls that
build tags_before and tags_after (the requests.get against tags_url) to first
verify the response succeeded (e.g., check response.status_code or call
response.raise_for_status()) and log/raise a clear error with
response.status_code and response.text before attempting .json(), so failures
from the registry API are surfaced rather than causing JSON/key errors after
signer.sign_image.
jonburdo
approved these changes
Mar 16, 2026
Member
jonburdo
left a comment
jonburdo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
I did a quick pass on this and it looks great! I'll try to do a more thorough review, but anything I would have could be addressed in a follow-up
jonburdo
reviewed
Mar 16, 2026
| && mv grpcurl /usr/bin/grpcurl | ||
|
|
||
| # Install cosign | ||
| RUN curl -sSL "https://github.com/sigstore/cosign/releases/download/v2.4.2/cosign-linux-amd64" --output /usr/bin/cosign \ |
Member
There was a problem hiding this comment.
Actually, sorry - I just realized - this should be rh cosign - not upstream sigstore
|
Status of building tag latest: success. |
ssaleem-rh
pushed a commit
to ssaleem-rh/opendatahub-tests
that referenced
this pull request
Mar 23, 2026
…es (opendatahub-io#1222) Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit
New Features
Chores