ci: Catch tests corrupting the source directory #32874
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32874. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
83a51f8 to
8986656
Compare
|
I am a bit confused about the ci failures. I guess it makes sense that chattr doesn't work on zfs, but is it also known to not work on overlayfs? If it doesn't work at all, I wonder how commit 5c2185b makes sense to enable the use of chattr in ci. Is there a single setup where it is known to work? |
4759c48 to
d910b8c
Compare
Changing into the build dir is confusing, and brittle. Avoiding this is required for the next commit.
d910b8c to
2890d3a
Compare
|
so on linux the check doesn't work and on macos it passed: https://github.com/bitcoin/bitcoin/actions/runs/16073560546/job/45363434537?pr=32874#step:7:2812 :( |
|
Closing for now. Probably best to move this to a dedicated nightly ci, than to try to force it into the existing ci infra. |
|
Better not to trust CI to do a security audit for you... One benefit of CMake is that you can build as another user without write access to the source dir (or indeed, in an isolated container). |
Yeah, anything in the CI is more a belt-and-suspenders and a way to catch "sunny-day" violations. Generally, the approach of removing write access can only catch unintentional and accidental violations anyway, because an intentional violation could just guard itself when a write error happens due to missing access. So any violation would have to directly lead to the process being killed, or otherwise logged to mark the tests as failed later on. I am not aware of a tool that allows this without host/root access, so for testing (with or without CI), you'll probably want a full vm with logging/debugging set up (via auditd, AppArmor, SELinux, firejail, strace, ...). Obviously as a user you can still use whatever sandbox you prefer, but for auditing the setup seems more involved. |
fad191f ci: Avoid cd into build dir (MarcoFalke) Pull request description: Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes. The changes are required for stuff like: * cmake presets (see #30871 (comment)) * meta ci tests (like #32874) So remove the `cd` and just make the build dir explicit. ACKs for top commit: hebasto: ACK fad191f, I have reviewed the code and it looks OK. Tree-SHA512: a88a9341445ffe28a0dac3815f235ec8eb0459d10a91a80829fd3184762d3c807d0f68c56243b20c04a6efa5becd8a7fad568f43c2b1e6af1ff8ba07b140ef87
At best it is annoying when tests delete random files in my source dir, or when they leave around temp files. At worst, it is an attempt to inject a backdoor.
So try to catch them in CI.
For example, this should hopefully catch: