intel / ccc-linux-guest-hardening

Linux Security Hardening for Confidential Compute
https://intel.github.io/ccc-linux-guest-hardening-docs
MIT License
63 stars 13 forks source link

Update fast matcher dependencies, fix clippy issues, migrate to Clap 4 #128

Closed novafacing closed 9 months ago

Wenzel commented 9 months ago

Thanks @novafacing It's weird that remote job was failing on the first attempt at compiling fast_matcher 🤔 : image

Reruning this jobs and now Github is giving me troubles because it can't find a runner 15 minutes after the launch... image

Something's broken on Github side here, i've opened a support ticket, will keep you updated.

sirmc commented 9 months ago

@novafacing The changes look good to me! I haven't tested them, but assuming it still works with our existing setup (i.e., if you did a quick run and it still generates the correct summary), I think this can be merged.

Just for my curiosity, is there any particular reason for rewriting the for X in Y to Y.for_each()? (To me this seems very JavaScripty to me, but granted I'm not up-to-speed with the latest Rust style best practices).

novafacing commented 9 months ago

@sirmc no particular reason, clippy just wants that style and I always give clippy what it wants!

I'm out until Wednesday, so I'm going to test then. I'll ping afterwards when it's tested.

Wenzel commented 9 months ago

@novafacing the CI fails because the remote deployement is based on Ubuntu 22.04 and cargo is installed via the package manager, so it pulls cargo 1.66.1.

If we switch to clap_lex 0.5, it should require Rust v1.64 :) image

Is it possible to downgrade that crate @novafacing ?

novafacing commented 9 months ago

@Wenzel should be possible, I'll look into it!

novafacing commented 9 months ago

@Wenzel clap_lex is just an internal dependency of clap, so it's not actually possible to use it instead (since it's not the same thing).

But we shouldn't be using a cargo version that old because of this CVE is it ok if I just update the version in CI?

Wenzel commented 9 months ago

the version in the CI that's blocking is the image i use for SSH and remote install

if you want to update it, sure

novafacing commented 9 months ago

Ok, added that fix so deploy now installs cargo using sh.rustup.rs instead of using apt. I'll add a message once this passes the "does fast_matcher work" smell test, it shouldn't be merged until then :)

novafacing commented 9 months ago

Ok, tested the updates to fast_matcher -- there was an issue where smatch_warns.txt wasn't parsing as fully valid UTF-8, so I switched over to using bytes regex instead. Did a run with ~100 traces and it worked no problem, identified the expected locations. I think this is OK to merge if it looks good to everyone. I split fast_matcher into lib and bin in case we want to add more tests in the future, unfortunately all the files needed to test it are quite big (500M+ for vmlinux + smatch_warns.txt + a few traces) so I didn't add tests in this PR.

sirmc commented 9 months ago

Ok, tested the updates to fast_matcher -- there was an issue where smatch_warns.txt wasn't parsing as fully valid UTF-8, so I switched over to using bytes regex instead. Did a run with ~100 traces and it worked no problem, identified the expected locations. I think this is OK to merge if it looks good to everyone. I split fast_matcher into lib and bin in case we want to add more tests in the future, unfortunately all the files needed to test it are quite big (500M+ for vmlinux + smatch_warns.txt + a few traces) so I didn't add tests in this PR.

Ok, I think this is more than safe to merge. Thanks for taking the time to push this + test @novafacing!

@Wenzel agree? Can you merge this?

Wenzel commented 9 months ago

@sirmc a little blocker here when I tried to replicate on my machine:

image

This is because the temporary file created previously by rowan is still there under his id, so I can't write my own. @novafacing what you need todo

novafacing commented 9 months ago

Fixed by using temp dir downloads and removing the directory afterward, thanks @Wenzel for the note :)

Wenzel commented 9 months ago

LGTM

ereshetova commented 9 months ago

This looks good now and tested, merging!