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 target-diff #664

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

JSCU-CNI
Copy link
Contributor

@JSCU-CNI JSCU-CNI commented Apr 4, 2024

This PR adds the command target-diff, which can be used to compare two or more targets against one another:

$ target-diff --help

target-diff

positional arguments:
  {shell,fs,query}      Mode for differentiating targets
    shell               Open an interactive shell to compare two or more targets.
    fs                  Yield records about differences between target filesystems.
    query               Differentiate plugin outputs between two or more targets.

options:
  -d, --deep            Compare file contents even if metadata suggests they have been left unchanged (default: False)
  -l LIMIT, --limit LIMIT
                        How many bytes to compare before assuming a file is left unchanged (0 for no limit) (default:
                        32768)

fs mode outputs records denoting filesystem changes from one target to the other:

$ target-diff --deep fs src.tar dst.tar

<differential/file/created hostname=None domain=None src_target='src.tar' dst_target='dst.tar' path='/changes/only_on_dst'>
<differential/file/deleted hostname=None domain=None src_target='src.tar' dst_target='dst.tar' path='/changes/only_on_src'>
<differential/file/modified hostname=None domain=None src_target='src.tar' dst_target='dst.tar' path='/changes/changed' diff=[b'--- \n', b'+++ \n', b'@@ -1 +1 @@\n', b'-SRC', b'+DST']>

Using query mode, you can compare plugin outputs from one target to the other:

$ target-diff query -f users src.tar dst.tar

<differential/record/unchanged hostname=None domain=None src_target='src.tar' dst_target='dst.tar' record=<unix/user hostname='dst_target' domain=None name='root' passwd='x' uid=0 gid=0 gecos='root' home='/root' shell='/bin/bash' source='/etc/passwd'>>
<differential/record/unchanged hostname=None domain=None src_target='src.tar' dst_target='dst.tar' record=<unix/user hostname='dst_target' domain=None name='user' passwd='x' uid=1000 gid=1000 gecos='user' home='/home/user' shell='/bin/bash' source='/etc/passwd'>>
<differential/record/created hostname=None domain=None src_target='src.tar' dst_target='dst.tar' record=<unix/user hostname='dst_target' domain=None name='dst_user' passwd='x' uid=1001 gid=1001 gecos='dst_user' home='/home/dst_user' shell='/bin/bash' source='/etc/passwd'>>
<differential/record/deleted hostname=None domain=None src_target='src.tar' dst_target='dst.tar' record=<unix/user hostname='src_target' domain=None name='src_user' passwd='x' uid=1001 gid=1001 gecos='src_user' home='/home/src_user' shell='/bin/bash' source='/etc/passwd'>>

In shell mode, you can browse the target filesystems like in target-shell, where directory listings will show which files / directories have been changed, added or deleted. Using the plugin command, plugin outputs can be compared from within the shell context.

$ target-diff shell src.tar dst.tar

(dst_target/src_target)/diff />help

Target Diff
==========


Documented commands (type help <topic>):
=================================================================
cat  clear  diff   exit  help  ls    plugin  previous  set
cd   cyber  enter  find  list  next  prev    python  

(dst_target/src_target)/diff />cd changes
(dst_target/src_target)/diff /changes>ls
changed
only_on_dst (deleted)
only_on_src (created)
subdirectory_both
subdirectory_dst (deleted)
subdirectory_src (created)
unchanged

target-diff depends on fox-it/flow.record#107. To allow tests to run for this PR we've temporarily bumped flow.record to 3.15.dev10 in pyproject.toml

When three or more targets are provided, you can choose between treating every target as a 'delta' or compare every target against one 'absolute' target. Treating targets as 'deltas' is useful if you have multiple snapshots of the same target from different points in time. Treating targets as 'absolutes' can be useful in situations where you have a 'golden image' that you want to compare different targets against.

To keep code duplication low between tools/diff.py and tools/shell.py, this PR adds a superclass ExtendedCmd to shell.py that contains most of the functionality that is shared between the two. Both TargetCmd and DifferentialCli inherit from this class.

Copy link
Member

@yunzheng yunzheng left a comment

Choose a reason for hiding this comment

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

I did not review the whole PR, just added some small flow.record quality of life suggestions since the merge of fox-it/flow.record#115

Available in flow.record==3.15.

pyproject.toml Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Outdated Show resolved Hide resolved
@JSCU-CNI JSCU-CNI force-pushed the feature/add-target-diff branch 2 times, most recently from 131e6b5 to aac31af Compare June 12, 2024 09:02
@JSCU-CNI
Copy link
Contributor Author

Thanks for your suggestions on the flow.record context manager addition. We were wondering if there is an ETA on a review of the PR as a whole, considering it's been over 2 months.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Aug 1, 2024

We understand this PR will take some time to review, but we do think it might be worthwhile to incorporate the changes to shell.py (the ExtendedCmd class, some fixes to target-shell, moving some functions outside of Cmd classes so they can be re-used) into main earlier. In its current state, this PR being left open for a long time causes it to increasingly divert from target-shell.

What we could do is move the shell.py changes into a seperate PR that you can then review first. We don't mind making this PR, and while we're at it we can also pick up some open issues along the way, such as #623, #624, #625 and #585. These improvements can be used for target-fs as well, as it only makes sense to re-use as much as possible between target-shell and target-fs when it comes to outputting target filesystem information. This will likely create a merge conflict with #716 if that is not yet merged, but if that ends up happening we'll incorporate those changes as well.

Do you agree with this approach?

@Schamper
Copy link
Member

Schamper commented Aug 1, 2024

I am working slowly through the backlog of long outstanding PRs but had not gotten to this yet. Your proposal of splitting it up sounds like a good idea though, it's always nice if we can split of large PRs into separate smaller ones. I won't complain either if you pick up some of the mentioned improvements along the way 😄.

I've just merged #716 for your convenience so feel free to do with that as you please 😉.

@JSCU-CNI JSCU-CNI mentioned this pull request Aug 5, 2024
@EinatFox EinatFox linked an issue Aug 6, 2024 that may be closed by this pull request
@JSCU-CNI JSCU-CNI mentioned this pull request Aug 8, 2024
* `--hex` can be used to diff binary files in a readable way.
* `--only-changed` can be used to omit unchanged records when comparing plugin outputs
@JSCU-CNI
Copy link
Contributor Author

Target-diff should be more or less on par with the changes made in #812 and should be ready for review @Schamper .

@JSCU-CNI JSCU-CNI requested a review from yunzheng October 7, 2024 09:56
@yunzheng yunzheng removed their request for review October 7, 2024 10:38
@yunzheng
Copy link
Member

yunzheng commented Oct 7, 2024

I will check if someone from the dissect team can review this.

dissect/target/tools/diff.py Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Outdated Show resolved Hide resolved
dissect/target/tools/diff.py Show resolved Hide resolved
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 176 lines in your changes missing coverage. Please review.

Project coverage is 77.73%. Comparing base (8785906) to head (ff4d18f).

Files with missing lines Patch % Lines
dissect/target/tools/diff.py 69.23% 176 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #664      +/-   ##
==========================================
- Coverage   77.90%   77.73%   -0.17%     
==========================================
  Files         324      325       +1     
  Lines       27852    28424     +572     
==========================================
+ Hits        21697    22095     +398     
- Misses       6155     6329     +174     
Flag Coverage Δ
unittests 77.73% <69.23%> (-0.17%) ⬇️

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.


🚨 Try these New Features:

dissect/target/tools/diff.py Outdated Show resolved Hide resolved
@JSCU-CNI
Copy link
Contributor Author

Thanks for the review @cecinestpasunepipe, I've applied your suggestions in 0a522e9.

@JSCU-CNI JSCU-CNI removed their assignment Nov 11, 2024
@cecinestpasunepipe
Copy link
Contributor

cecinestpasunepipe commented Nov 11, 2024

@JSCU-CNI looks good to me, can you just fix the remaining tests? Apart from that, I have no further comments. I also want to add that we really appreciate this feature, nice work!

Copy link
Contributor

@cecinestpasunepipe cecinestpasunepipe left a comment

Choose a reason for hiding this comment

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

Only fix the test please.

@JSCU-CNI
Copy link
Contributor Author

Thanks @cecinestpasunepipe! The tests should be fixed in c3266b9.

@cecinestpasunepipe
Copy link
Contributor

LGTM, I have no further requests.

@JSCU-CNI
Copy link
Contributor Author

The linked issue #795 was closed but the PR is still open. Did something go wrong there?

@yunzheng
Copy link
Member

@JSCU-CNI Can you rebase the branch?

The linked issue #795 was closed but the PR is still open. Did something go wrong there?

@EinatFox do you know why this issue was closed?

@EinatFox
Copy link

@JSCU-CNI Can you rebase the branch?

The linked issue #795 was closed but the PR is still open. Did something go wrong there?

@EinatFox do you know why this issue was closed?

I see this was wrongly marked as done, reopened the issue.

@JSCU-CNI
Copy link
Contributor Author

What is the status of this PR?

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.

Add target-diff PR#664
6 participants