-
Notifications
You must be signed in to change notification settings - Fork 53
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 Acquire collection loader #935
base: main
Are you sure you want to change the base?
Conversation
First of all, thanks for you contribution @Matthijsy. I think this is a good first attempt :)! As you kind of pointed out, the Let me know what you think about this! |
dissect/target/loaders/zip.py
Outdated
@@ -0,0 +1,97 @@ | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add tests for this new functionality? This could be done later, if you decide to go on the AcquireLoader
route.
@Horofic Thank you for the feedback, that indeed sounds like a good plan. I have now changed it to a dedicated
I suspect that the Furthermore, I suspect this will not work anymore for the legacy acquire format as described in the |
Posting this comment for documentation purposes. @Matthijsy and I had a discussion outside of this PR regarding possible approaches for this @Matthijsy one possible approach we haven’t gone over is described and implemented in PR #700 by @Zawadidone. I believe some changes have to be made to the current |
I made the @Horofic If you have time could you have a look? Would be great to see if this now covers all the different cases |
I have also added (or mainly re-used) a few test cases now. Only the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a initial look. Some of the uncommented tests still seem to fail, could you fix those? Additionally the test that are commented are preferred to stay enabled. I provided some context per test, so adding those cases to the AcquireLoader
logic might be easier. Ill take another look later this week.
Again, if you have any questions, please shoot!
if not root: | ||
return False | ||
|
||
return root.joinpath(FILESYSTEMS_ROOT).exists() or root.joinpath(FILESYSTEMS_LEGACY_ROOT).exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be very slow on large tar files, where there will be a double performance penalty. First the entire tar file will be parsed just for this check, then if it exists, it will be parsed again in __init__
. However, if it doesn't exist, it will still be parsed again in the actual tar loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, but is there a way to prevent that? As we do need to check if the fs
directory exists in the file, since otherwise the tar is not an acquire collect and thus need to be handled by the TarLoader
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ideally" (not really, since it's only for performance and the code path itself will be more confusing) it's a subpath in the zip
and tar
loaders. So you piggy back on the detection logic of those, and upon mapping you do a fast check to see if you need to divert logic to Acquire logic (which could exist in loaders/acquire.py
.
At least, that's the "best" idea I can come up with right now.
Co-authored-by: Stefan de Reuver <[email protected]>
Co-authored-by: Stefan de Reuver <[email protected]>
Co-authored-by: Stefan de Reuver <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this file is not added to the git LFS. Could you add it please?
if p.name == "sysvol": | ||
dirs.append(('c', p)) | ||
else: | ||
dirs.append((p.name[0], p)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes what the function returns. Not bad necessarily, but it might break some other loaders that use this function. So those would need to be changed.
Co-authored-by: Stefan de Reuver <[email protected]>
This PR adds a loader for ZIP files, which is mainly useful for ZIP based acquire collects.
I don't have experience with creating loaders, and mainly took the logic from the TarLoader. So if there are things that can be done better would be happy to improve!
fixes #934