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

fix: skip tmpfs mounts only for select paths #2918

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Conversation

willmurphyscode
Copy link
Contributor

@willmurphyscode willmurphyscode commented May 31, 2024

Syft originally attempted to use path based exclusion for known problematic paths to scan (e.g. /sys), then eventually moved to a filesystem-based detection to achieve identifying parts of the system to skip. This has caused a few different problems:

  • Syft currently skips everything under a tmpfs mount, see

    case "proc", "procfs", "sysfs", "devfs", "devtmpfs", "udev", "tmpfs":
    . This is the root cause of the linked issues

  • An especially bad case of this bug is syft does not find anything in archives if /tmp is a tmpfs #2894 - when pointed at an archive, syft will untar the archive to temp directory, and then ignore the directory it just made if the directory is on a tmpfs mount.

  • Syft currently ignores files that are on a non-ignored mount type that's mounted at a path under an ignored mount type, e.g. in the nixOS case where / is tmpfs, but /home/permanent is ext4 or something, /home/permanent is incorrectly ignored because it's under ` tmpfs mount. This is resolved in this PR by only considering the longest prefix match between the mount infos and the path under consideration, instead of every prefix match.

The main observation out of the above problems is that ignoring tmpfs globally is problematic. The goal of this PR is to combine the best of bother pervious approaches -- path based exclusion and filesystem type exclusion -- in order to avoid problematic areas of the filesystem. These should be combined in a way such that we are allowing tmpfs to be scanned in areas outside of known problematic paths, thereby allowing syft to scan more areas of the system.

This PR tries to change tries to make the ignoring of paths more cleanly in terms of 3 2 business rules:

  1. Continue to ignore existing non-tmpfs filesystem types (e.g. sysfs, devfs, proc, etc.)
  2. Ignore tmpfs type mounts if they're at certain paths.

Originally this PR had a third rule: Special case: never ignore the entire directory being scanned ... however, this opens the door for syft to infinity scan a forbidden path, which isn't the intention. So syft scan / should still ignore /proc and other forbidden paths, just because it's within the scan target doesn't mean we should drop all ignore rules. But this rules doesn't make sense since if a user attempts syft scan /proc we would honor it, but it would still appear to be an empty directory (since we ignored all of the contents of /proc). For these reasons I've elected to drop this rule.

Additionally this PR adds a CLI test to show that archive scans function as expected (that at least one cataloger is wired, and a result is found). This is a smoke-like test for this general functionality in the context of the temp dir being the destination for the archive.

Note

This PR also bumps the minimum required go version from 1.21.0 to 1.22.0 in order to use new functionality

Fixes #2894
Fixes #2847
See for additional context: anchore/grype#1822

@wagoodman wagoodman force-pushed the fix-stop-ignoring-tmpfs branch 2 times, most recently from 5eac666 to a18e307 Compare June 3, 2024 19:55
@wagoodman
Copy link
Contributor

I've pushed extra tests, a refactor, and updated the PR description to reflect the current state.

@wagoodman
Copy link
Contributor

I'm using the new tar.AddFS capability, which is only available in 1.22. For this reason I bumped the go version in both the workflows and the minimum go version required (though it's not required at runtime, it is technically required for tests).

@wagoodman wagoodman marked this pull request as ready for review June 3, 2024 20:51
@wagoodman
Copy link
Contributor

This output uses a snapshot build (/syft-new) with the conditions outlined in #2847 to show that the functionality has been restored:

root@ac9b6235f233:/# /syft /tmp/app
 ✔ Indexed file system                                                                                                                                                                                /tmp/app
 ✔ Cataloged contents                                                                                                                         d75b6c3b0007e70845b739c2c7afe797a80311811e7a74948ef4b989486b07af
   ├── ✔ Packages                        [0 packages]
   └── ✔ Executables                     [0 executables]
[0000]  WARN no explicit name and version provided for directory source, deriving artifact ID from the given path (which is not ideal)
No packages discovered

root@ac9b6235f233:/# /syft-new /tmp/app
 ✔ Indexed file system                                                                                                                                                                                /tmp/app
 ✔ Cataloged contents                                                                                                                         d75b6c3b0007e70845b739c2c7afe797a80311811e7a74948ef4b989486b07af
   ├── ✔ Packages                        [16 packages]
   └── ✔ Executables                     [0 executables]
[0000]  WARN no explicit name and version provided for directory source, deriving artifact ID from the given path (which is not ideal)
NAME                                                            VERSION                             TYPE
actions/cache                                                   v3                                  github-action
actions/checkout                                                v3                                  github-action
actions/setup-go                                                v3                                  github-action
anchore/workflows/.github/workflows/oss-project-board-add.yaml  main                                github-action-workflow
github.com/davecgh/go-spew                                      v1.1.1                              go-module
github.com/google/uuid                                          v1.3.0                              go-module
github.com/mattn/go-colorable                                   v0.1.12                             go-module
github.com/mattn/go-isatty                                      v0.0.14                             go-module
github.com/mgutz/ansi                                           v0.0.0-20200706080929-d51e80ef957d  go-module
github.com/pmezard/go-difflib                                   v1.0.0                              go-module
github.com/scylladb/go-set                                      v1.0.2                              go-module
github.com/sirupsen/logrus                                      v1.8.1                              go-module
github.com/stretchr/testify                                     v1.5.1                              go-module
golang.org/x/sys                                                v0.1.0                              go-module
golang.org/x/term                                               v0.0.0-20210927222741-03fcf44c2211  go-module
gopkg.in/yaml.v2                                                v2.4.0                              go-module

root@ac9b6235f233:/# df -h
Filesystem      Size  Used Avail Use% Mounted on
overlay          88G   80G  3.6G  96% /
tmpfs            64M     0   64M   0% /dev
shm              64M     0   64M   0% /dev/shm
tmpfs           3.9G  276K  3.9G   1% /tmp
/dev/vda1        88G   80G  3.6G  96% /etc/hosts
tmpfs           3.9G     0  3.9G   0% /sys/firmware

willmurphyscode and others added 5 commits June 4, 2024 13:49
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Copy link
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

🟢 Thanks for the pairing and checking on this one this AM!

@wagoodman wagoodman merged commit 557ad73 into main Jun 4, 2024
11 checks passed
@wagoodman wagoodman deleted the fix-stop-ignoring-tmpfs branch June 4, 2024 19:21
@wagoodman wagoodman changed the title fix: only skip tmpfs mounts for some paths fix: skip tmpfs mounts only for select paths Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants