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 --resolve to target-query #485

Merged
merged 18 commits into from
Jan 3, 2024
Merged

Conversation

Miauwkeru
Copy link
Contributor

  • Moves all the record modifier code to helpers/modifier.py

  • Adds modifier logic, to allow for more modifiers to get added later (currently only paths get used)

  • open the target_path during hashing:

When opened in a DirLoader, the OS will "open" the file
However, the file never got closed, so on linux you'd get
an error that the maximum open files threshold was reached

  • Add tests for resolve

@Miauwkeru Miauwkeru requested a review from pyrco December 18, 2023 12:22
Comment on lines 837 to 838
return hashutil.custom(fd, algos)
return hashutil.common(fd)
Copy link
Member

Choose a reason for hiding this comment

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

Where is fd coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from a debug artifact that I didn't revert correctly

Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on it, but could you maybe also replace all instances of List in this file with list?

@@ -67,7 +67,7 @@ def firewall(self):
data[fname] = value

if "app" in data:
data["app"] = self.target.resolve(data["app"])
data["app"] = str(self.target.resolve(data["app"]))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the app field in the record become a path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

techically it should, i did see some instances it wasn't a path tho. but some other information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems I was mistaken on that front, that was another changed app

Comment on lines 354 to 357
modifier = None

if args.resolve:
modifier = modifier.Modifier.RESOLVE
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this conflict with the import name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That it did, that was an oversight

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (ae79ef5) 74.05% compared to head (8941d3b) 74.07%.

Files Patch % Lines
dissect/target/tools/query.py 62.96% 10 Missing ⚠️
dissect/target/filesystems/dir.py 50.00% 5 Missing ⚠️
dissect/target/helpers/record_modifier.py 98.30% 1 Missing ⚠️
dissect/target/plugins/apps/webserver/iis.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
+ Coverage   74.05%   74.07%   +0.02%     
==========================================
  Files         260      261       +1     
  Lines       21070    21110      +40     
==========================================
+ Hits        15603    15638      +35     
- Misses       5467     5472       +5     
Flag Coverage Δ
unittests 74.07% <85.71%> (+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.

@Miauwkeru Miauwkeru requested review from Schamper and pyrco December 22, 2023 09:12
@Miauwkeru Miauwkeru requested a review from pyrco January 3, 2024 08:00
@Miauwkeru Miauwkeru requested a review from pyrco January 3, 2024 09:35
@Miauwkeru Miauwkeru force-pushed the DIS-2624-create-resolve-command branch from 55e3592 to c28c323 Compare January 3, 2024 09:35
@Miauwkeru Miauwkeru dismissed Schamper’s stale review January 3, 2024 10:13

Changes were implemented

@Miauwkeru Miauwkeru merged commit 47c83a3 into main Jan 3, 2024
@Miauwkeru Miauwkeru deleted the DIS-2624-create-resolve-command branch January 3, 2024 10:15
Poeloe pushed a commit that referenced this pull request Feb 29, 2024
* Changes the target.resolve() functionality to return a TargetPath
* Create modifiers that can change records before printing them.

(DIS-2624)
Zawadidone pushed a commit to Zawadidone/dissect.target that referenced this pull request Apr 5, 2024
* Changes the target.resolve() functionality to return a TargetPath
* Create modifiers that can change records before printing them.

(DIS-2624)
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.

3 participants