refactor(core)!: Use the shared ca-trust library and stop baking CA certificates into images. - #2451
Open
jackluo923 wants to merge 1 commit into
Open
refactor(core)!: Use the shared ca-trust library and stop baking CA certificates into images.#2451jackluo923 wants to merge 1 commit into
jackluo923 wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces automatic corporate CA staging with opt-in CA trust propagation. Shared Docker Buildx helpers, package-manager mirror scripts, Dockerfile trust stages, packaging runtime mounts, workflow rules, and documentation now support this workflow. ChangesCA trust and Docker build migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PackagingBuild
participant DockerImageBuild
participant DockerBuildx
participant PackageContainer
PackagingBuild->>DockerImageBuild: parse --with-ca-certs
DockerImageBuild->>DockerBuildx: build with CA arguments
DockerBuildx->>PackageContainer: mount optional CA trust data
PackageContainer->>PackageContainer: run package commands through ca-trust-run.sh
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/tools/scripts/utils/run-in-container.sh (1)
27-62: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
ca_trust_cmd_prefixis computed but never used;--with-ca-certssilently does nothing here.Line 49 builds
ca_trust_cmd_prefix=("bash" "${CA_TRUST_CONTAINER_DIR}/container-exec.sh"), and the comment at lines 43-46 explains that this prefix is required because "something inside the container has to source container.sh to export the trust environment." The finaldocker runcommand at lines 55-62 splices in${ca_trust_args[@]+"${ca_trust_args[@]}"}(the mount) but never referencesca_trust_cmd_prefix, so the user's command runs directly, withoutcontainer-exec.shever sourcingcontainer.sh.As a result, when
--with-ca-certsis passed, the host CA bundle is mounted into the container, but no trust environment variable (SSL_CERT_FILE,CURL_CA_BUNDLE, etc.) is ever exported for the command running inside the container. The flag has no observable effect on the run command actually executed, which contradicts the script's own inline documentation and defeats its purpose behind a TLS-intercepting proxy.packaging/build.sh(line 353) shows the correct pattern for the same variable.🐛 Proposed fix
docker run \ -i \ --rm \ -u"$(id -u):$(id -g)" \ --mount "type=bind,src=$(readlink -f "$component_root"),dst=$container_component_root" \ ${ca_trust_args[@]+"${ca_trust_args[@]}"} \ -w "$container_component_root" \ - ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main "$@" + ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main \ + ${ca_trust_cmd_prefix[@]+"${ca_trust_cmd_prefix[@]}"} "$@"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/core/tools/scripts/utils/run-in-container.sh` around lines 27 - 62, Use the computed ca_trust_cmd_prefix in the final docker run command so that, when --with-ca-certs is enabled, container-exec.sh sources the trust configuration before executing the user’s command. Preserve direct execution of "$@" when the flag is disabled, following the established pattern used by packaging/build.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile`:
- Around line 19-34: Introduce a shared ca-trust wrapper script, such as
ca-trust-run.sh, that encapsulates the existing CA_TRUST_DIR setup and optional
container-exec invocation, then replace both duplicated wrapper blocks in
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile#L19-34,
components/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfile#L21-34,
components/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfile#L21-34,
and
components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile#L19-34
with calls to that shared script, passing each target script as its argument.
In `@components/core/tools/packaging/build.sh`:
- Around line 68-102: Extract the shared CA-trust staging, validation,
run-argument, and container-exec-prefix setup from
components/core/tools/packaging/build.sh lines 68-102 and
components/core/tools/scripts/utils/run-in-container.sh lines 27-50 into one
helper, preferably in the CA-trust library or docker-image-build.sh. Update both
callers to use that helper, retaining ca_trust_add_build_args only in build.sh
and ensuring run-in-container.sh applies the resulting ca_trust_cmd_prefix when
invoking the container.
In `@components/core/tools/packaging/universal-deb/Dockerfile`:
- Around line 23-27: Update the Dockerfile RUN step that installs epel-release
to execute through /run/ca-trust/container-exec.sh by setting runner="bash
/run/ca-trust/container-exec.sh". Source the dnf CA options from
ca-trust-pkg-opts.sh and pass the resulting CLP_DNF_CA_OPTS array to every dnf
invocation in this step, replacing the current container.sh-only setup.
In `@components/core/tools/scripts/lib_install/ca-trust-pkg-opts.sh`:
- Around line 23-26: The staged bundle filename in _clp_ca_trust_bundle must
remain aligned with the library’s CA_TRUST_BUNDLE_FILENAME used by
package-manager options. Reuse the shared constant if available inside this
script; otherwise add a focused validation that compares the configured filename
with ca-bundle.pem and fails loudly on mismatch.
In `@docs/src/dev-docs/tooling-containers.md`:
- Around line 192-193: Update the library name in the documentation sentence
referencing the `docker/build` and `docker/ca-trust` libraries by removing the
stray apostrophe after `yscope-dev-utils`, while preserving the existing link
and wording.
- Around line 219-220: Update the CA-bundle locality statement in the BuildKit
documentation to apply only when using a local builder without cache export. Add
a warning before the --with-ca-certs usage explaining that remote builders or
cache export can transmit the bundle outside the invoking machine, especially
when the builder is untrusted.
In `@tools/yscope-dev-utils`:
- Line 1: Update the tools/yscope-dev-utils submodule pointer from
de62d090d6e5b7933f67aead3ea6c9ea030dc53f to the merged upstream commit for
yscope-dev-utils#119 after it lands, then rerun the CA-trust build validation.
---
Outside diff comments:
In `@components/core/tools/scripts/utils/run-in-container.sh`:
- Around line 27-62: Use the computed ca_trust_cmd_prefix in the final docker
run command so that, when --with-ca-certs is enabled, container-exec.sh sources
the trust configuration before executing the user’s command. Preserve direct
execution of "$@" when the flag is disabled, following the established pattern
used by packaging/build.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1f1ddc6-1904-4cec-b821-48c3fecb4137
📒 Files selected for processing (30)
.github/actions/clp-core-build-containers/action.yaml.github/workflows/clp-artifact-build.yamlcomponents/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfilecomponents/core/tools/docker-images/clp-env-base-centos-stream-9/build.shcomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfilecomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/build.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfilecomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/build.shcomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfilecomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.shcomponents/core/tools/packaging/alpine-apk/Dockerfilecomponents/core/tools/packaging/build.shcomponents/core/tools/packaging/universal-deb/Dockerfilecomponents/core/tools/scripts/.gitignorecomponents/core/tools/scripts/corporate-proxy-container.shcomponents/core/tools/scripts/corporate-proxy-host.shcomponents/core/tools/scripts/docker-image-build.shcomponents/core/tools/scripts/lib_install/.gitignorecomponents/core/tools/scripts/lib_install/ca-trust-pkg-opts.shcomponents/core/tools/scripts/lib_install/centos-stream-9/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/utils/run-in-container.shdocs/src/dev-docs/tooling-containers.mdtools/scripts/deps-download/init.shtools/yscope-dev-utils
💤 Files with no reviewable changes (5)
- components/core/tools/scripts/lib_install/.gitignore
- components/core/tools/scripts/.gitignore
- components/core/tools/scripts/corporate-proxy-container.sh
- .github/workflows/clp-artifact-build.yaml
- components/core/tools/scripts/corporate-proxy-host.sh
3 tasks
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
from
August 2, 2026 18:53
e356fdb to
f6e901b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile (1)
19-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated CA-trust wrapper block across all four base-image Dockerfiles, still unresolved. The same
if [ -e /run/ca-trust/container.sh ]; then export CA_TRUST_DIR=...; runner="bash /run/ca-trust/container-exec.sh"; else runner=""; fi; $runner ...block appears twice per file, in four files. A prior review already raised this exact duplication and proposed a shared script; the duplication is still present unchanged.
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile#L19-L34: replace both wrapper blocks with a call to a shared script, for exampletools/scripts/lib_install/ca-trust-run.sh <target-script>.components/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfile#L21-L34: replace both wrapper blocks with the same shared script call.components/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfile#L21-L34: replace both wrapper blocks with the same shared script call.components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile#L19-L34: replace both wrapper blocks with the same shared script call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile` around lines 19 - 34, Remove the duplicated CA-trust wrapper logic around both installation commands in components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile (lines 19-34), components/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfile (lines 21-34), components/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfile (lines 21-34), and components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile (lines 19-34). Replace each wrapper with a call to the shared tools/scripts/lib_install/ca-trust-run.sh script, passing the existing target script as its argument and preserving both script invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/tools/scripts/docker-image-build.sh`:
- Around line 93-101: Update run_image_build around the ca_trust_dir cleanup so
its EXIT trap does not overwrite the caller’s existing EXIT handler. Scope the
trap to a subshell containing ca_trust_stage_host_bundle and
docker_build_finalize, while preserving their current failure and cleanup
behavior.
In
`@components/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.sh`:
- Around line 21-25: Update the mirror-rewrite logic in
configure-package-mirror.sh to escape APT_MIRROR_URL for use as a sed
replacement, handling &, backslashes, and the | delimiter before the three sed
expressions run. Preserve the existing source URL substitutions while ensuring
configured mirror values cannot alter the replacement expression.
---
Duplicate comments:
In `@components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile`:
- Around line 19-34: Remove the duplicated CA-trust wrapper logic around both
installation commands in
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile
(lines 19-34),
components/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfile
(lines 21-34),
components/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfile (lines
21-34), and
components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile (lines
19-34). Replace each wrapper with a call to the shared
tools/scripts/lib_install/ca-trust-run.sh script, passing the existing target
script as its argument and preserving both script invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60bfef68-8ccf-48f4-a97c-386c63b9e31b
📒 Files selected for processing (30)
.github/actions/clp-core-build-containers/action.yaml.github/workflows/clp-artifact-build.yamlcomponents/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfilecomponents/core/tools/docker-images/clp-env-base-centos-stream-9/build.shcomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfilecomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/build.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfilecomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/build.shcomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfilecomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.shcomponents/core/tools/packaging/alpine-apk/Dockerfilecomponents/core/tools/packaging/build.shcomponents/core/tools/packaging/universal-deb/Dockerfilecomponents/core/tools/scripts/.gitignorecomponents/core/tools/scripts/corporate-proxy-container.shcomponents/core/tools/scripts/corporate-proxy-host.shcomponents/core/tools/scripts/docker-image-build.shcomponents/core/tools/scripts/lib_install/.gitignorecomponents/core/tools/scripts/lib_install/ca-trust-pkg-opts.shcomponents/core/tools/scripts/lib_install/centos-stream-9/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/utils/run-in-container.shdocs/src/dev-docs/tooling-containers.mdtools/scripts/deps-download/init.shtools/yscope-dev-utils
💤 Files with no reviewable changes (5)
- components/core/tools/scripts/.gitignore
- components/core/tools/scripts/lib_install/.gitignore
- components/core/tools/scripts/corporate-proxy-container.sh
- .github/workflows/clp-artifact-build.yaml
- components/core/tools/scripts/corporate-proxy-host.sh
jackluo923
changed the title
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
from
August 2, 2026 22:27
f6e901b to
d51693d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/src/dev-docs/tooling-containers.md (1)
192-193: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stray apostrophe from
yscope-dev-utils.The link text currently renders
yscope-dev-utils'. Remove the apostrophe so the library name displays correctly.Proposed fix
-All base image `build.sh` scripts source `docker-image-build.sh`, which wraps [yscope-dev-utils' +All base image `build.sh` scripts source `docker-image-build.sh`, which wraps [yscope-dev-utils🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/dev-docs/tooling-containers.md` around lines 192 - 193, Remove the stray apostrophe from the `yscope-dev-utils` link text in the documentation, leaving the surrounding link and sentence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile`:
- Line 9: Update the base image reference in the Dockerfile’s FROM directive to
use an approved immutable digest or explicit versioned tag instead of the
mutable latest tag, while preserving the existing base stage name.
In `@tools/yscope-dev-utils`:
- Line 1: Update the tools/yscope-dev-utils submodule pointer to the merge
commit SHA produced by y-scope/yscope-dev-utils PR `#119`, ensuring the PR is
merged rather than draft. After updating the pointer, rerun the CA-trust build
validation and confirm the checked-out SHA matches the merged commit.
---
Duplicate comments:
In `@docs/src/dev-docs/tooling-containers.md`:
- Around line 192-193: Remove the stray apostrophe from the `yscope-dev-utils`
link text in the documentation, leaving the surrounding link and sentence
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f2eefc0-0935-458d-a2df-44afc349fdb9
📒 Files selected for processing (31)
.github/actions/clp-core-build-containers/action.yaml.github/workflows/clp-artifact-build.yamlcomponents/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfilecomponents/core/tools/docker-images/clp-env-base-centos-stream-9/build.shcomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfilecomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/build.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfilecomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/build.shcomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfilecomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.shcomponents/core/tools/packaging/alpine-apk/Dockerfilecomponents/core/tools/packaging/build.shcomponents/core/tools/packaging/universal-deb/Dockerfilecomponents/core/tools/scripts/.gitignorecomponents/core/tools/scripts/corporate-proxy-container.shcomponents/core/tools/scripts/corporate-proxy-host.shcomponents/core/tools/scripts/docker-image-build.shcomponents/core/tools/scripts/lib_install/.gitignorecomponents/core/tools/scripts/lib_install/ca-trust-pkg-opts.shcomponents/core/tools/scripts/lib_install/ca-trust-run.shcomponents/core/tools/scripts/lib_install/centos-stream-9/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/utils/run-in-container.shdocs/src/dev-docs/tooling-containers.mdtools/scripts/deps-download/init.shtools/yscope-dev-utils
💤 Files with no reviewable changes (5)
- components/core/tools/scripts/.gitignore
- components/core/tools/scripts/corporate-proxy-container.sh
- components/core/tools/scripts/lib_install/.gitignore
- .github/workflows/clp-artifact-build.yaml
- components/core/tools/scripts/corporate-proxy-host.sh
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
from
August 2, 2026 22:44
d51693d to
ef67451
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/scripts/deps-download/init.sh`:
- Line 11: Update the YSCOPE_DEV_UTILS_COMMIT_SHA constant in init.sh to the
merged yscope-dev-utils commit from PR `#119`, replacing the current draft-PR SHA
and keeping the downloader aligned with the tools/yscope-dev-utils submodule.
In `@tools/yscope-dev-utils`:
- Line 1: After upstream PR `#119` merges, update the tools/yscope-dev-utils
gitlink to its merge commit SHA and update the dependency reference in
tools/scripts/deps-download/init.sh to that exact same SHA. Ensure both
submodule and archive-download paths resolve the merged dependency revision, not
the current draft commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92efd809-7778-4318-989d-bf6b01cccc58
📒 Files selected for processing (31)
.github/actions/clp-core-build-containers/action.yaml.github/workflows/clp-artifact-build.yamlcomponents/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfilecomponents/core/tools/docker-images/clp-env-base-centos-stream-9/build.shcomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfilecomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/build.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfilecomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/build.shcomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfilecomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.shcomponents/core/tools/packaging/alpine-apk/Dockerfilecomponents/core/tools/packaging/build.shcomponents/core/tools/packaging/universal-deb/Dockerfilecomponents/core/tools/scripts/.gitignorecomponents/core/tools/scripts/corporate-proxy-container.shcomponents/core/tools/scripts/corporate-proxy-host.shcomponents/core/tools/scripts/docker-image-build.shcomponents/core/tools/scripts/lib_install/.gitignorecomponents/core/tools/scripts/lib_install/ca-trust-pkg-opts.shcomponents/core/tools/scripts/lib_install/ca-trust-run.shcomponents/core/tools/scripts/lib_install/centos-stream-9/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/utils/run-in-container.shdocs/src/dev-docs/tooling-containers.mdtools/scripts/deps-download/init.shtools/yscope-dev-utils
💤 Files with no reviewable changes (5)
- components/core/tools/scripts/.gitignore
- .github/workflows/clp-artifact-build.yaml
- components/core/tools/scripts/corporate-proxy-container.sh
- components/core/tools/scripts/lib_install/.gitignore
- components/core/tools/scripts/corporate-proxy-host.sh
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
from
August 2, 2026 23:01
ef67451 to
9569742
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/tools/packaging/universal-deb/Dockerfile`:
- Around line 25-31: Update the Dockerfile shell configuration after FROM
${BASE_IMAGE} so the RUN block sourcing ca-trust-pkg-opts.sh executes explicitly
under Bash and can safely use the CLP_DNF_CA_OPTS array; preserve the existing
package installation commands.
In `@docs/src/dev-docs/tooling-containers.md`:
- Line 197: Update the tooling requirements text to state separately that Docker
23 is the project-supported minimum because build.sh relies on docker buildx,
and describe named build contexts as a separate Buildx requirement rather than
implying they are Docker 23-specific.
In `@tools/scripts/deps-download/init.sh`:
- Line 11: Update YSCOPE_DEV_UTILS_COMMIT_SHA only after yscope-dev-utils PR
`#119` merges, setting it to the verified merge commit SHA; update the
tools/yscope-dev-utils gitlink to that identical SHA so the archive downloader
and submodule resolve the same revision.
In `@tools/yscope-dev-utils`:
- Line 1: After upstream PR `#119` merges, update the tools/yscope-dev-utils
gitlink to its merged commit SHA and update the corresponding dependency
reference in tools/scripts/deps-download/init.sh to that exact same SHA. Ensure
neither reference remains on the draft revision and both submodule and
archive-download paths resolve the merged dependency revision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3991382d-e603-4748-b8e7-f67f9021bfe8
📒 Files selected for processing (31)
.github/actions/clp-core-build-containers/action.yaml.github/workflows/clp-artifact-build.yamlcomponents/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfilecomponents/core/tools/docker-images/clp-env-base-centos-stream-9/build.shcomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/Dockerfilecomponents/core/tools/docker-images/clp-env-base-manylinux_2_28/build.shcomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/Dockerfilecomponents/core/tools/docker-images/clp-env-base-musllinux_1_2/build.shcomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfilecomponents/core/tools/docker-images/clp-env-base-ubuntu-jammy/build.shcomponents/core/tools/packaging/alpine-apk/Dockerfilecomponents/core/tools/packaging/build.shcomponents/core/tools/packaging/universal-deb/Dockerfilecomponents/core/tools/scripts/.gitignorecomponents/core/tools/scripts/corporate-proxy-container.shcomponents/core/tools/scripts/corporate-proxy-host.shcomponents/core/tools/scripts/docker-image-build.shcomponents/core/tools/scripts/lib_install/.gitignorecomponents/core/tools/scripts/lib_install/ca-trust-pkg-opts.shcomponents/core/tools/scripts/lib_install/ca-trust-run.shcomponents/core/tools/scripts/lib_install/centos-stream-9/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/manylinux_2_28/install-prebuilt-packages.shcomponents/core/tools/scripts/lib_install/musllinux_1_2/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/configure-package-mirror.shcomponents/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shcomponents/core/tools/scripts/utils/run-in-container.shdocs/src/dev-docs/tooling-containers.mdtools/scripts/deps-download/init.shtools/yscope-dev-utils
💤 Files with no reviewable changes (5)
- components/core/tools/scripts/lib_install/.gitignore
- .github/workflows/clp-artifact-build.yaml
- components/core/tools/scripts/.gitignore
- components/core/tools/scripts/corporate-proxy-container.sh
- components/core/tools/scripts/corporate-proxy-host.sh
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
2 times, most recently
from
August 2, 2026 23:17
779f967 to
47fac82
Compare
… images.
`components/core` carried its own host-CA propagation for builds behind a
TLS-intercepting corporate gateway, duplicated a third time inside the CI
action. yscope-dev-utils now hosts that library with a build-time surface, so
all of it can go.
The old design copied the host bundle into the build context, overwrote the
container's distro trust store, and baked CURL_CA_BUNDLE/PIP_CERT/
REQUESTS_CA_BUNDLE/SSL_CERT_FILE into the image. Because CI staged the GitHub
runner's bundle to satisfy a hard-fail, every published clp-core-dependencies
image shipped a frozen snapshot of that runner's trust store in place of its own
distro's. The built ubuntu-jammy image's /etc/ssl/certs/ca-certificates.crt is
now byte-identical to a pristine `apt-get install ca-certificates` (same md5,
121 certs), and no CA environment variables or /opt/corp-ca remain.
Host CA trust becomes opt-in via `build.sh --with-ca-certs`, matching
clp-plugin-presto-connector. The bundle is mounted through a named build context
only for the RUN steps that reach the network -- a build context rather than a
BuildKit secret, since secrets are capped at 500KiB and corporate bundles can
exceed that. Each Dockerfile declares `FROM scratch AS ca_trust` as an empty
default, so an unprovided context resolves to an empty mount instead of failing;
BuildKit would otherwise try to pull the context name as an image. That is what
lets the CI action drop its staging step and add nothing in its place.
Point apt and dnf at the bundle explicitly, via
`lib_install/ca-trust-pkg-opts.sh`. Neither reads SSL_CERT_FILE or
CURL_CA_BUNDLE -- verified: apt over https still fails certificate verification
with both set to a valid bundle, and dnf succeeds with both set to /dev/null --
so the environment variables the library exports cover only curl, pip, and apk.
The removed container-side script had installed the bundle into the distro trust
store, which is why it worked for package managers; this restores that coverage
without writing to the trust store. Verified end to end on an image with no
system CA store at all: an https `apt-get update` reports 24 certificate errors
without the staged bundle and 0 with it.
Extract each distro's mirror `sed` block into
`lib_install/<distro>/configure-package-mirror.sh`. Necessary because
`container.sh` needs bash while a Dockerfile RUN uses /bin/sh -- dash on ubuntu,
busybox on musl -- so those steps go through `container-exec.sh`, and the
mirror commands' embedded quoting can't survive the nesting. The scripts keep
the trailing package-manager refresh, so a bad mirror still fails at the mirror
step rather than midway through installation.
Also cover the packaging images. `universal-deb` and `alpine-apk` run networked
installs and previously inherited the baked SSL_CERT_FILE from the base image,
so corporate packaging worked by accident; without this they would have silently
broken. `packaging/build.sh` stages the bundle once and reuses it for the base
image build, the builder image build, and the build container. Both packaging
Dockerfiles keep `ARG BASE_IMAGE` ahead of the first `FROM`: declaring it after
the `ca_trust` stage would scope it to that stage and leave `FROM ${BASE_IMAGE}`
resolving to an empty base name.
Mounting a trust directory is inert on its own -- something inside the container
has to source `container.sh` -- so `run-in-container.sh` and the packaging build
container prefix their commands with `container-exec.sh`. Published images no
longer carry host CAs, so commands that reach the network from inside a
container need this. `CurlDownloadHandler` is unaffected: it reads
CURL_CA_BUNDLE, then SSL_CERT_FILE, then the distro paths, which are now
pristine rather than overwritten.
Co-Authored-By: Claude <noreply@anthropic.com>
jackluo923
force-pushed
the
refactor/ca-trust-lib
branch
from
August 2, 2026 23:24
47fac82 to
4ebcc96
Compare
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
1 participant
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.
Description
Why
Corporate networks often run a TLS-inspecting gateway that re-signs HTTPS traffic with the company's own certificate authority. Your laptop trusts it; a container doesn't. Builds then fail with certificate errors that give no hint of the cause, so each project invents its own fix —
y-scope/clpandy-scope/clp-plugin-presto-connectorboth had one, and they agreed on almost nothing.Ours was the worst. It overwrote the container's trust list permanently, and because CI had to feed that machinery, every published
clp-core-dependenciesimage shipped a frozen GitHub-runner trust store instead of its own — a quiet, long-standing defect. This PR adopts the shared library from yscope-dev-utils#119: certificates are supplied only while a build step runs, never written into the image, and the whole thing is opt-in.What changed
Host CA trust becomes opt-in via
build.sh --with-ca-certs, matching clp-plugin-presto-connector. The bundle is mounted through a named build context only for the RUN steps that reach the network — a build context rather than a BuildKit secret, since secrets are capped at 500KiB and corporate bundles can exceed that. Each Dockerfile declaresFROM scratch AS ca_trustas an empty default, so an unprovided context resolves to an empty mount instead of failing, which is what lets the CI action drop its staging step and add nothing back.apt and dnf read neither
SSL_CERT_FILEnorCURL_CA_BUNDLE, solib_install/ca-trust-pkg-opts.shpoints them at the bundle explicitly (Acquire::https::CaInfo,--setopt=sslcacert). The removed container-side script had installed the bundle into the distro trust store, which is why it worked for package managers; this restores that coverage without writing to the trust store.Each distro's mirror
sedblock moves intolib_install/<distro>/configure-package-mirror.sh, because those steps now go throughcontainer-exec.sh(container.shneeds bash; a Dockerfile RUN uses dash on ubuntu and busybox on musl) and the embedded quoting can't survive the nesting.Breaking: published deps images no longer carry host CA certificates. No in-repo consumer reads the removed variables —
CurlDownloadHandlertreats them as optional overrides and falls back to the distro paths, which are now pristine rather than overwritten. Corporate users who relied on a locally-built image having their CA baked in should pass--with-ca-certsat build time, andrun-in-container.sh --with-ca-certsfor commands that reach the network from inside a container.Depends on yscope-dev-utils#119. The submodule and
tools/scripts/deps-download/init.shboth point at the tip of that stack (y-scope/yscope-dev-utils#120 → #119 → #122) and need re-pointing to the merged SHA before this merges.Checklist
Validation performed
Through a real TLS-intercepting proxy (mitmproxy with its own CA, confirmed intercepting): an https
apt-get updatereports 24 certificate errors without the staged bundle and 0 with it.All four base images built end to end, each verified against its own pristine base:
/opt/corp-caThe
manylinux/musllinuxentries areSSL_CERT_FILE=/opt/_internal/certs.pem, set by the pypa base images themselves; those two Dockerfiles intentionally don't flatten so base env survives.Also verified: the guard falls through correctly with no context under both dash and busybox
sh; exit status propagates so the existing "try a different mirror" hint block still fires; a bad mirror fails at the mirror step rather than midway through installation; and the staging directory is removed on both the success and failure paths.All builds ran on an aarch64 host, so the x86 paths are unexercised here — CI is the first x86 signal. Cross-arch under QEMU was not run.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation