Repository navigation
PULP-2642: upgrade pulp_python to 3.38.0 + per-domain ATTESTATION_VERIFICATION_KEYS (PULP-2643 + PULP-2647) - #1671
Conversation
Reviewer's GuideThis PR upgrades pulp-python to 3.38.0 with Python 3.12, removes the downstream attestation-verification patch that is now upstream, and migrates deployment, development, CI, and functional tests to pulp-python’s per-domain ATTESTATION_VERIFICATION_KEYS configuration while preserving the vendored signing key for the supported production domains. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
2a3bace to
84df3f1
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="deploy/clowdapp.yaml" line_range="390" />
<code_context>
value: ${{PULP_DB_ENCRYPTION_KEY}}
- - name: PULP_ATTESTATION_VERIFICATION_KEY
- value: ${{PULP_ATTESTATION_VERIFICATION_KEY}}
+ - name: PULP_ATTESTATION_VERIFICATION_KEYS
+ value: ${{PULP_ATTESTATION_VERIFICATION_KEYS}}
- name: PULP_CACHE_ENABLED
</code_context>
<issue_to_address>
**Attestation tests reject uploads**
When the pull-request functional tests run with the generated test signing key, the test deployment provides only `PULP_ATTESTATION_VERIFICATION_KEY` and the fixture reads `ATTESTATION_VERIFICATION_KEY`, while `pulp-python` 3.38.0 verifies with the per-domain `ATTESTATION_VERIFICATION_KEYS` map. The generated key is therefore unavailable to verification, so the attestation tests fail.
Update the functional-test deployment and fixture to configure and read the per-domain verification-key map.
Also at `deploy/clowdapp.yaml:391`, `deploy/clowdapp.yaml:752`, `pulp_service/requirements.txt:4`.
</issue_to_address>
### Comment 2
<location path="Dockerfile" line_range="2" />
<code_context>
FROM registry.access.redhat.com/ubi9/ubi
-ARG PYTHON_VERSION=3.11
+ARG PYTHON_VERSION=3.12
ENV PYTHONUNBUFFERED=0
ENV DJANGO_SETTINGS_MODULE=pulpcore.app.settings
</code_context>
<issue_to_address>
**Functional tests cannot import bindings**
When the pull-request Tekton pipeline installs bindings and runs tests against the Python 3.12 image, the Tekton setup still writes namespace-package shims and sets `PYTHONPATH` to Python 3.11 site-packages, but the image uses Python 3.12. The generated bindings are not found and functional-test imports fail.
Update the Tekton shim paths and `PYTHONPATH` to use the Python 3.12 site-packages directory.
Also at `pulp_service/requirements.txt:4`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if a domain is mapped to an overly trusted verification key, attestations signed by that key could be accepted for artifacts in that domain. Those artifacts and their provenance records can remain available after a revert, so reverting alone would not undo the trust decision's effects.
Blocking findings: deploy/clowdapp.yaml:390, Dockerfile:2
Addresses the Sourcery review on pulp#1671. Attestation functional tests: migrate the test deployment (.tekton), the dev-container settings, and the test fixture from the single ATTESTATION_VERIFICATION_KEY to the per-domain ATTESTATION_VERIFICATION_KEYS map (default domain -> test key), matching pulp_python 3.38.0. Update the content-upload assertion to the 3.38.0 publisher ("Pulp User"), since the SLSA v0.2 publisher enrichment was removed together with patch 0048. Bindings / test runner: the Tekton steps run via `oc exec` inside the pulp image, which is now Python 3.12, so update the namespace-package shim paths and PYTHONPATH from python3.11 to python3.12 site-packages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".tekton/pulp-deploy-and-test.yaml" line_range="608" />
<code_context>
"name": "pulp-api",
"env": [
- {"name": "PULP_ATTESTATION_VERIFICATION_KEY", "value": "/etc/pki/attestation/test-key.pem"}
+ {"name": "PULP_ATTESTATION_VERIFICATION_KEYS", "value": "@json {\"default\": \"/etc/pki/attestation/test-key.pem\"}"}
],
"volumeMounts": [
</code_context>
<issue_to_address>
**Attestation tests fail to start**
When tekton applies the deployment patch directly with `oc`, `oc` preserves the `@json` prefix in `PULP_ATTESTATION_VERIFICATION_KEYS`, so the setting is not a dictionary and the fixture's `.get("default")` fails; the attestation functional tests do not run successfully.
Pass a JSON mapping as the environment value without the Clowder-specific `@json` marker.
Also at `.tekton/pulp-deploy-and-test.yaml:638-639`.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/tests/functional/test_attestation_verification.py" line_range="246" />
<code_context>
publisher = prov_obj.provenance["attestation_bundles"][0]["publisher"]
- assert publisher["builder_id"] == "https://konflux-ci.dev/calunga"
- assert publisher["build_type"] == "https://konflux-ci.dev/PythonWheelBuild@v1"
- assert publisher["kind"] == "konflux-ci.dev"
+ assert publisher["kind"] == "Pulp User"
</code_context>
<issue_to_address>
**Dropped attestations pass upload test**
When the content-upload handler creates the provenance bundle but omits its attestations, `test_konflux_attestation_via_content_upload` checks only the publisher kind at line 245, so an upload path that creates a Pulp User provenance bundle but drops the submitted attestation still passes.
Assert that the stored provenance contains the submitted attestation, including its envelope.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and the per-domain key map defines which signing key is trusted to verify attestations for the configured domains. If that mapping grants trust to the wrong key or domains, attestations can be accepted under the wrong trust boundary; reverting stops future acceptance but does not undo provenance already recorded.
Blocking findings: .tekton/pulp-deploy-and-test.yaml:608, pulp_service/pulp_service/tests/functional/test_attestation_verification.py:246
…keys Atomic upgrade combining the version bump (PULP-2643) and the per-domain attestation key config (PULP-2647), which must ship together: pulp_python 3.36.2 reads ATTESTATION_VERIFICATION_KEY (single) while 3.38.0 reads ATTESTATION_VERIFICATION_KEYS (per-domain map), and stage auto-deploys main. PULP-2643 (bump + drop patch): - Bump pulp-python 3.36.2 -> 3.38.0 (requirements + version table in CLAUDE.md). - Remove patch 0048 (upstreamed in pulp/pulp_python#1327): the .patch file, its Dockerfile COPY/RUN, and its docs entry; the vendored key copy is retained. - Bump image Python 3.11 -> 3.12 (3.38.0 requires >=3.12). PULP-2647 (per-domain config): - Replace the single PULP_ATTESTATION_VERIFICATION_KEY with the per-domain ATTESTATION_VERIFICATION_KEYS map in the api/content/worker containers and the template parameter. The map covers the three prod domains that use key-signed attestations (public-trusted-libraries, lightwell, lightwell-stage), each mapped to the existing vendored key path /etc/pki/sigstore/SIGSTORE-redhat-release3. Functional tests: - Migrate the test deployment (.tekton), dev-container settings, and the attestation test fixture to the per-domain map (default domain), and update the content-upload assertions to 3.38.0 behavior (Pulp User publisher; assert the submitted attestation is stored). - Update the Tekton binding shims and PYTHONPATH from python3.11 to python3.12. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1b44417 to
fb1ac2b
Compare
| - name: PULP_ATTESTATION_VERIFICATION_KEY | ||
| description: Path to attestation verification key | ||
| value: "/etc/pki/sigstore/SIGSTORE-redhat-release3" | ||
| - name: PULP_ATTESTATION_VERIFICATION_KEYS |
There was a problem hiding this comment.
Let's have a default value here, but we should keep the actual values for stage and prod deployments in app-interface.
There was a problem hiding this comment.
Thanks, good point. I moved it to app-interface as suggested here https://gitlab.cee.redhat.com/service/app-interface/-/merge_requests/217576
and then keep here only the default value in this repository
Per review feedback: the clowdapp template parameter carries a safe empty
default ('@JSON {}', matching pulp_python's own ATTESTATION_VERIFICATION_KEYS
default) instead of hardcoding the prod domain map. The real per-environment
maps (stage and prod) are set as app-interface saas parameters, following the
same per-target override pattern used for the other PULP_* settings.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pulp-python 3.37.0+ requires Python >=3.12. Since #1671 pinned pulp-python==3.38.0, the dev container image (still on Python 3.11) fails to build at `pip install /tmp/pulp_service`. The production Dockerfile already uses 3.12. - dev-container/Dockerfile: PYTHON_VERSION 3.11 -> 3.12 - pulp-add-patch / pulp-remove-patch: update site-packages fallback - setup.py: python_requires >=3.12, drop 3.11 classifier Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Why this is one atomic PR
The pulp_python bump and the attestation config must ship together because the running image and the config have to agree in every environment: pulp_python 3.36.2 reads
ATTESTATION_VERIFICATION_KEY(one global key) while 3.38.0 readsATTESTATION_VERIFICATION_KEYS(a per-domain map). Shipping only the bump (new code, old config) or only the config (new env var, old code) leaves key-signed attestation uploads with no usable key and rejects them for the domains that rely on them. So the bump (PULP-2643) and the per-domain config (PULP-2647) land as a single change.PULP-2643 — bump + drop patch
pulp-pythonpin3.36.2 -> 3.38.0(pulp_service/requirements.txt+ version table inimages/assets/patches/CLAUDE.md).0048(upstreamed in pulp/pulp_python#1327): the.patch, its COPY/RUN in theDockerfile, and its docs entry. The vendored-key COPY is retained (the map points at the key copied fromimages/assets/keys/SIGSTORE-redhat-release3.pemto/etc/pki/sigstore/SIGSTORE-redhat-release3).3.11 -> 3.12(DockerfileARG PYTHON_VERSION) — 3.38.0 requires>=3.12; cascades via${PYTHON_VERSION}.PULP-2647 — per-domain config
Replaces the single
PULP_ATTESTATION_VERIFICATION_KEYwith the per-domain map indeploy/clowdapp.yaml(api/content/worker containers + template parameter):Domains derived from a full prod inventory -> blast radius (stored provenance): only these 3 use the key-signed path (all
Pulp User, no cert); the other 22 python domains have 0 attestations.Validation
oc process --localrenders the map; noPULP_ATTESTATION_VERIFICATION_KEYreferences remain; clowdapp YAML parses.images/assets/keys/SIGSTORE-redhat-release3.pem.Refs: PULP-2643, PULP-2647, epic PULP-2642.
🤖 Generated with Claude Code
Summary by Sourcery
Upgrade pulp_python and migrate attestation verification to per-domain key configuration while aligning the image, deployment, development, and CI environments.
New Features:
Bug Fixes:
Enhancements:
CI:
Deployment:
Documentation:
Tests: