-
Notifications
You must be signed in to change notification settings - Fork 584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: ensure wasm integrity #3173
base: main
Are you sure you want to change the base?
Conversation
cf344ec
to
5c017dd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3173 +/- ##
==========================================
+ Coverage 94.16% 94.17% +0.01%
==========================================
Files 90 90
Lines 24385 24437 +52
==========================================
+ Hits 22962 23014 +52
Misses 1423 1423 ☔ View full report in Codecov by Sentry. |
5c017dd
to
c8d3201
Compare
id: check_files | ||
run: | | ||
echo "=============== list modified files ===============" | ||
git diff --name-only HEAD^ HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mweberxyz can we maybe get somehow the parent commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Co-authored-by: Aras Abbasi <[email protected]>
Please dont merge yet. I really think the solution is not proper yet. |
@Uzlopak please make it a draft PR to avoid merging |
@mcollina PTAL. Should be correct now. |
@@ -181,6 +181,10 @@ jobs: | |||
- name: Run typings tests | |||
run: npm run test:typescript | |||
|
|||
test-llhttp-integrity: | |||
name: Ensure integrity of llhttp wasm files | |||
uses: nodejs/undici/.github/workflows/llhttp-wasm-integrity.yml@main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would run this for every PR. Maybe only if those files were changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a reusable workflow. It has two steps. First check if wasm related files were changed, if so run the integrity check. Maybe the first step can be further optimized by using https://github.com/tj-actions/changed-files so that we can also take care of cases where we directly push into main. Also makes sense to obligatory run the integrity check in the automated release workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just run it in the release workflow then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho we should ensure that the repo has the correct content at any time. So if somebody tries to upstream malicious code into the repo, then it should be imho detected early and not as the last step of the release workflow.
The only issue i see, is that people could force push into main and bypass the ci.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can limit that now that we have automated releases, open a separate issue.
Ensure that the wasm files are not tampered with.
If the llhttp source files or llhttp wasm files are touched, we check also if the wasm files are correct or if they have some tampered binary code. Basically trying to remove the potential attack vector like it was built in into xz.
We can then close
https://github.com/nodejs/undici/security/code-scanning/251
https://github.com/nodejs/undici/security/code-scanning/252
If the code is tampered with, you will see it
https://github.com/nodejs/undici/actions/runs/8915422913/job/24485005620
@mweberxyz
Any Ideas how to improve it?