Repository navigation
Revert "Stop trusting compromised eMudhra roots in the build-toolchain images (#9528)" - #9540
eliottness wants to merge 1 commit into
Conversation
eliottness
added
the
area:builds
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The revert also removes the SCLo repository repair, breaking fresh CentOS toolchain builds when package installation reaches the retired mirror service. Cached layers can mask this failure.
🤖 Bits Code Review · Commit e5d422d
| @@ -8,10 +8,6 @@ RUN sed -i s/mirror.centos.org/vault.centos.org/g /etc/yum.repos.d/*.repo \ | |||
| && sed -i s/^mirrorlist=http/#mirrorlist=http/g /etc/yum.repos.d/*.repo \ | |||
| && yum update -y \ | |||
| && yum install -y centos-release-scl \ | |||
There was a problem hiding this comment.
On a fresh CentOS toolchain build, centos-release-scl creates repository files after the initial vault rewrite. Removing the second rewrite leaves those SCLo repositories pointing at retired mirrorlist.centos.org, causing the subsequent yum install of devtoolset-11 to fail. Preserve the SCLo repository repair after centos-release-scl installation independently of the certificate rollback.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
@eliottness I'm unsure if this is now a new issue caused by the prior merge.
There was a problem hiding this comment.
@bouwkast for the record: no, that one predates #9528. mirrorlist.centos.org is gone, so the stock centos7.build.dockerfile no longer built on a cold cache. #9528 fixed it by moving the SCLo repos to vault, and reverting would have brought the break back. Moot now that the VMs are rebuilt and this is closed. Thanks for handling it, and sorry again for the early merge.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: e5d422df5f
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| @@ -2,12 +2,6 @@ | |||
| FROM mcr.microsoft.com/dotnet/runtime-deps:5.0-buster-slim AS base | |||
| ARG DOTNETSDK_VERSION | |||
|
|
|||
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
An attacker with a compromised eMudhra intermediate key and network interception can exploit this when the image performs an uncached download. Deleting the rules here makes the Debian builder trust that chain again; it then downloads and executes https://apt.llvm.org/llvm.sh without a digest, and CI runs the resulting image with the checkout mounted before publishing Linux tracer packages. Restore the system, NSS, and NuGet distrust rules (and the equivalent Alpine/CentOS rules) rather than trading them for cache performance.
Useful? React with 👍 / 👎.
BenchmarksBenchmark execution time: 2026-10-27 20:08:03 Comparing candidate commit e5d422d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 10 known flaky benchmarks, 116 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9540) and master. ✅ No regressions detected |
Summary of changes
Reverts #9528 (squash commit 759ccb4), which changed the four build-toolchain Dockerfiles (
alpine.build,alpine.build.arm64,centos7.build,debian). This PR restores them byte-for-byte to their content before #9528.Reason for change
#9528 was merged before the CI VMs were rebuilt. The VMs carry these images' layers prebaked, so after the change every Linux job rebuilds the
debianimage instead of using the cache. In #9528's own PR builds, the debian image step went from a median of 0.9 min to 3.1 min (max 13 min) per job, roughly 470 extra agent-minutes per full pipeline. Builds still pass; it is a CI-wide slowdown. This revert restores the cached images until apm-dotnet rebuilds and republishes the VMs (blue/green), after which #9528 can be re-landed.Implementation details
git revertof 759ccb4: 4 files, 62 deletions, no other changes. Nothing on master touched these files after #9528.Test coverage
No new tests: this restores the previously cached Dockerfiles.
Other details
Trade-off: until the change is re-landed, the build-toolchain images trust emSign ECC Root CA - G3 and AAA Certificate Services again, as they did before #9528 (incident IR-62343, APPSEC-71282).
Opened for @DataDog/apm-dotnet to merge (or close, if you prefer to rebuild the VMs now). Sorry for merging #9528 before the VM rebuild was coordinated.