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

Add support for docker containers as children #441

Merged
merged 46 commits into from
Apr 18, 2024

Conversation

JSCU-CNI
Copy link
Contributor

@JSCU-CNI JSCU-CNI commented Nov 13, 2023

This PR adds child target support to Dissect for docker containers and depends on #507.

To demonstrate:

$ target-query -f os,version --children example.img
<Target example.img> linux Ubuntu 22.04.3 LTS (Jammy Jellyfish)
<Target /var/lib/docker/image/overlay2/layerdb/mounts/852d704264f1078107bd593e261b79c7f5f3b53243be2f3b26f9189c7b8cbf75> linux Alpine Linux 3.18.4
<Target /var/lib/docker/image/overlay2/layerdb/mounts/53ad2edfc7474c3122e601b9f23fca705eae85b405c7c52b9b53d400618a9bd4> linux Debian GNU/Linux 12 (bookworm)
<Target /var/lib/docker/image/overlay2/layerdb/mounts/96171c133b33b6db82fe59ca91d45a5dc2803bba19501f00e06d243514da1f76> linux CentOS Linux 8

Commands such as target-shell's enter also work when provided the path to the overlay2 mount folder.

Uses the LayerFilesystem functionality of #575 to map every overlay2 layer on top of each other in a single Overlay2Filesystem. Deleted or moved files will still exist in the final reconstructed filesystem.

@JSCU-CNI JSCU-CNI force-pushed the feature/docker-children branch from 18aa10b to 87ab5e8 Compare November 14, 2023 12:28
@JSCU-CNI JSCU-CNI marked this pull request as ready for review November 27, 2023 12:33
Copy link
Contributor

@Horofic Horofic left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response. Internally we are looking at creating a separate LayerFilesystem (name pending :)) that can be used independent of the RootFilesystem. Which is a different direction of what this PR is trying to achieve, since it builds on top of the RootFilesystem.

The benefit of having a separate LayerFilesystem is that it would give us the ability to more cleanly map files to different paths in loaders, instead of how it is done currently.

In order to accomplish this, we need to do some refactoring of the current VirtualFilesystem implementation, too able to use layers. After that we are going to look and see if that approach is what we want.

With that said, I think the best course of action would be to keep this PR open (unmerged). So that improvements can still be made and be used by other users, until we decided on how to move forward with this.

@JSCU-CNI
Copy link
Contributor Author

Thanks for the consideration @Horofic. In the meantime, could you review the new log parsing functions in this PR? I can split the children functionality in a separate PR if needed.

@Horofic Horofic self-requested a review January 15, 2024 09:34
@Schamper
Copy link
Member

Can you split that into a different PR, please? That would aid the review, and likely get that merged in sooner!

@JSCU-CNI JSCU-CNI mentioned this pull request Jan 17, 2024
@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Jan 17, 2024

@Schamper Done, see #507.

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 78.76712% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 74.00%. Comparing base (a0c6c9f) to head (988753f).

❗ Current head 988753f differs from pull request most recent head b34beec. Consider uploading reports for the commit b34beec to get more accurate results

Files Patch % Lines
dissect/target/filesystems/overlay.py 68.04% 31 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   74.02%   74.00%   -0.02%     
==========================================
  Files         285      287       +2     
  Lines       23578    23627      +49     
==========================================
+ Hits        17454    17486      +32     
- Misses       6124     6141      +17     
Flag Coverage Δ
unittests 74.00% <78.76%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Feb 21, 2024

@Horofic Can you share any update in regards to this PR and/or the LayerFilesystem you have in mind? Is this prioritized or on the backlog? Note that we restructured this PR to include a LayerFilesystem in November.

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 77.96610% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 75.09%. Comparing base (bf82f59) to head (75fa7e3).

Files Patch % Lines
dissect/target/filesystems/overlay.py 63.79% 21 Missing ⚠️
dissect/target/loaders/overlay.py 84.21% 3 Missing ⚠️
dissect/target/filesystems/dir.py 83.33% 1 Missing ⚠️
dissect/target/plugins/apps/container/docker.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
+ Coverage   75.05%   75.09%   +0.04%     
==========================================
  Files         288      291       +3     
  Lines       24588    24700     +112     
==========================================
+ Hits        18454    18549      +95     
- Misses       6134     6151      +17     
Flag Coverage Δ
unittests 75.09% <77.96%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Schamper
Copy link
Member

Schamper commented Mar 15, 2024

In light of #575 it might be nice to base this on top of that. Additionally, I think any logic that is currently in the loader should be transferred to a overlay2 filesystem implementation. The logic for it should be contained within the filesystem implementation as much as possible.

Other thought I had was if our current "overlay filesystem" behaves 1:1 with overlay2? Is there no additional logic necessary to deal with edge cases from overlay2? Something like deleted files, for example. I'm not sure if I ever wrote this on GitHub before, but we plan to migrate to a "DirectoryEntry" kind of approach in the future, where concepts like deleted files can more easily be integrated. It might be fine to ignore dealing with deleted files for now in overlay2, but and keep that as something to deal with in the future.

@JSCU-CNI JSCU-CNI requested a review from Schamper April 15, 2024 09:19
Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Last small bits, good to go after.

Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Can you check why the tests are failing? I think it may have to do with the change in dir.py.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Apr 16, 2024

Two things break the tests:

  • changes in the readlink function for DirectoryFilesystemEntry do not work on Windows
  • TargetPath.parents seems to be broken on python3.9

I believe we previously reported TargetPath.parents to be broken but I can't remember where. Edit: #488

@Schamper
Copy link
Member

After some experimentation I've decided to revert the readlink behavior. Using the pathlib variant doesn't give us the "original" result from the readlink, essentially tainting it with the OS specific Path flavor. Underwater, all the pathlib version does it wrap the result of os.readlink anyway.

I'll take a look at the 3.9 behavior.

@JSCU-CNI
Copy link
Contributor Author

Thanks for looking into this.

After some experimentation I've decided to revert the readlink behavior.

Unfortunately this will break the overlay filesystem implementation as os.readlink tries to resolve paths on the host dissect is running on (instead of inside the target), which is why you get the following errors:

FAILED tests/filesystems/test_overlay.py::test_overlay_filesystem_docker_container - PermissionError: [Errno 13] Permission denied: '/var/lib/docker/overlay2/80...

@Schamper
Copy link
Member

Schamper commented Apr 18, 2024

Tried to do a ninja edit but you beat me to it. I noticed the failure too, but I still think the readlink change is not the right approach. I'll take a look at the current failure and come up with a solution. Probably a small variation on it.

@JSCU-CNI
Copy link
Contributor Author

Perhaps we need an Overlay2FilesystemEntry subclassed from DirectoryFilesystemEntry after all :)

@Schamper
Copy link
Member

Perhaps we need an Overlay2FilesystemEntry subclassed from DirectoryFilesystemEntry after all :)

Unfortunately that doesn't solve the underlying problem that DirectoryFilesystem should work on both pathlib.Path and TargetPath. I've made it a bit more explicit right now.

@JSCU-CNI
Copy link
Contributor Author

The failing 3.9 tests might be related to these issues: https://bugs.python.org/issue41511, https://bugs.python.org/issue21041 (and this fix in 3.10: https://github.com/python/cpython/pull/21799/files#diff-fa525485738fc33d05b06c159172ff1f319c26e88d8c6bb39f7dbaae4dc4105cR636).

@Schamper
Copy link
Member

Nice, that was indeed it. I've backported a fix for 3.9.

Schamper
Schamper previously approved these changes Apr 18, 2024
@Schamper Schamper merged commit 0993094 into fox-it:main Apr 18, 2024
16 checks passed
@JSCU-CNI JSCU-CNI deleted the feature/docker-children branch April 18, 2024 12:33
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.

4 participants