Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Apr 27, 2024

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?

@Uzlopak Uzlopak force-pushed the llhttp-integrity branch 2 times, most recently from cf344ec to 5c017dd Compare April 27, 2024 00:10
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.17%. Comparing base (51a97af) to head (7e71031).
Report is 11 commits behind head on main.

❗ Current head 7e71031 differs from pull request most recent head a2371db. Consider uploading reports for the commit a2371db to get more accurate results

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.
📢 Have feedback on the report? Share it here.

.github/workflows/nodejs.yml Outdated Show resolved Hide resolved
id: check_files
run: |
echo "=============== list modified files ==============="
git diff --name-only HEAD^ HEAD
Copy link
Contributor Author

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?

Copy link
Member

@mcollina mcollina left a 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 <aras.abbasi@googlemail.com>
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 27, 2024

Please dont merge yet. I really think the solution is not proper yet. git diff --name-only HEAD^ HEAD will only process one commit. But I need to compare it with the last common commit (hash).

@ronag
Copy link
Member

ronag commented Apr 27, 2024

@Uzlopak please make it a draft PR to avoid merging

@Uzlopak Uzlopak marked this pull request as draft April 27, 2024 10:31
@Uzlopak Uzlopak marked this pull request as ready for review May 1, 2024 21:59
@Uzlopak
Copy link
Contributor Author

Uzlopak commented May 1, 2024

@mcollina
@metcoder95
@mweberxyz

PTAL. Should be correct now.
Maybe @mweberxyz can have a more strict review.

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants