ci(APMSP-3083): fix security issue by splitting the job - #2420
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6824093 | Docs | View more details | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2026-08-26 10:27:30 Comparing candidate commit 6824093 in PR branch Found 1 performance improvements and 8 performance regressions! Performance is the same for 159 metrics, 10 unstable metrics.
|
ekump
left a comment
There was a problem hiding this comment.
Code is LGTM, but could you do an editorial pass on the comments before merging?
| # Pin policy to proper guards: | ||
| # - Code went through review on main so 'ref' reflect that. | ||
| # - The reference is protected. | ||
| # - Assure the workflow provenance. |
There was a problem hiding this comment.
Should this comment be here? It reads more like AI explaining what it did.
| # 1.`rustfmt` checks out the pull request and runs cargo fmt with no token and no `id-token` | ||
| # permission. | ||
| # 2. `rustfmt-commit` only applies the resulting patch and pushes it only using verified tools | ||
| # from the image, in this case 'git'. Not that --force-with-lease is used which makes the push |
There was a problem hiding this comment.
| # from the image, in this case 'git'. Not that --force-with-lease is used which makes the push | |
| # from the image, in this case 'git'. Note that --force-with-lease is used which makes the push |
| @@ -1,42 +1,145 @@ | |||
| name: Rustfmt Auto | |||
|
|
|||
| # Security model: | |||
There was a problem hiding this comment.
Can we make this comment block more concise? It reads like AI explaining what it did, rather than something useful for future readers.
What does this PR do?
This PR hardens the workflow by improving its security model:
Use
pull_request_targetwhich runs the definition from the default branch instead, which also pins the OIDCrefandjob_workflow_refclaims torefs/heads/main, so the trust policy can require a protected ref like the repository's other STS policies do.The trade-off of
pull_request_targetis that the job context is privileged, so the work is split in two:1.
rustfmtchecks out the pull request and runs cargo fmt with no token and noid-tokenpermission.2.
rustfmt-commitonly applies the resulting patch and pushes it only using verified tools from the image, in this case 'git'. Note that--force-with-leaseis used which makes the push a no-op if the branch moved on from the revision that was formatted. Not using third-party tools in this job is deliberate as it prevents using uncontrolled code with elevated permissions.