dissect.target icon indicating copy to clipboard operation
dissect.target copied to clipboard

Initial commit for plugin internals refactor

Open Schamper opened this issue 1 year ago • 1 comments

Initial commit for the plugin internals refactor. This removes a lot of slow and hard to understand code with (hopefully) faster and easier to understand code. Also fixes some consistency issues I came across while working on this.

There are some things I want to add, but I may do those in a separate PR:

  • Re-add support for https://github.com/fox-it/dissect.target/issues/758 (it was lost in between version 2 and 3)
  • Add support for "plugin directories", similar to _os.py, but then _plugin.py or something similar (https://github.com/fox-it/dissect.target/pull/788)
  • Add helper functions for getting runtime information from functions, e.g. the class object, the record descriptor objects, etc. This will finally allow us to query plugins by e.g. ones that generate records that have a path field, for example.

This should also cover https://github.com/fox-it/dissect.target/pull/759.

Closes https://github.com/fox-it/dissect.target/issues/889.

Schamper avatar Jul 25 '24 14:07 Schamper

Codecov Report

Attention: Patch coverage is 88.29201% with 85 lines in your changes missing coverage. Please review.

Project coverage is 76.83%. Comparing base (f2f50a5) to head (552933d). Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
dissect/target/plugin.py 91.64% 38 Missing :warning:
dissect/target/target.py 70.31% 19 Missing :warning:
dissect/target/tools/query.py 77.35% 12 Missing :warning:
dissect/target/plugins/general/plugins.py 88.00% 6 Missing :warning:
dissect/target/helpers/docs.py 88.09% 5 Missing :warning:
dissect/target/tools/utils.py 81.25% 3 Missing :warning:
dissect/target/plugins/os/windows/lnk.py 0.00% 1 Missing :warning:
dissect/target/plugins/os/windows/registry.py 50.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
- Coverage   77.72%   76.83%   -0.90%     
==========================================
  Files         326      327       +1     
  Lines       28575    28645      +70     
==========================================
- Hits        22210    22009     -201     
- Misses       6365     6636     +271     
Flag Coverage Δ
unittests 76.83% <88.29%> (-0.90%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 25 '24 14:07 codecov[bot]

I noticed plugins/general/example.py and plugins/os/unix/packagemanager.py still have a __findable__ property, which is now no longer needed.

Fixed package manager but we still don't want the example plugin to findable.

Another thing I noticed is that the -l no longer lists the plugins from _os.py.

Should be fixed again.

And a third thing I noticed that if I run a namespace plugin, e.g. webserver.access, which is listed when doing target-query MyTarget -l, before it printed an error like:

Error while parsing sysvol/windows/system32/inetsrv/config/applicationHost.config: ...

but now it errors with:

target-query: error: argument -f/--function contains invalid plugin(s): webserver.access

I had to largely rewrite the NamespacePlugin to properly behave without any of its old side effects, but this is now also fixed.

Schamper avatar Dec 16 '24 15:12 Schamper

Would it be possible to add registered arguments to generate_functions_json?

diff --git a/dissect/target/plugins/general/plugins.py b/dissect/target/plugins/general/plugins.py
index 334830b..6dd5ed9 100644
--- a/dissect/target/plugins/general/plugins.py
+++ b/dissect/target/plugins/general/plugins.py
@@ -59,14 +59,25 @@ def generate_functions_json(functions: list[plugin.FunctionDescriptor] | None =
 
     for desc in functions or _get_default_functions():
         plugincls = plugin.load(desc)
-        docstring = getattr(plugincls, desc.method_name).__doc__
+        func = getattr(plugincls, desc.method_name)
 
         loaded.append(
             {
                 "name": desc.name,
                 "output": desc.output,
-                "description": docstring.split("\n\n", 1)[0].strip() if docstring else None,
+                "description": func.__doc__.split("\n\n", 1)[0].strip() if func.__doc__ else None,
                 "path": desc.path,
+                "arguments": [
+                    {
+                        "name": name[0],
+                        "type": arg.get("type").__name__ if arg.get("type") is not None else None,
+                        "help": arg.get("help"),
+                        "default": arg.get("default"),
+                    }
+                    for name, arg in func.__args__
+                ]
+                if hasattr(func, "__args__")
+                else None,
             }
         )

JSCU-CNI avatar Jan 27 '25 13:01 JSCU-CNI

Additionally could FunctionDescriptor.record be populated with __record__ as discussed in #759?

JSCU-CNI avatar Jan 27 '25 13:01 JSCU-CNI

Additionally could FunctionDescriptor.record be populated with __record__ as discussed in #759?

This is not possible since we can't serialise the record descriptor, so it would just be a string. This functionality will be part of an incremental improvement of this PR as discussed in the main PR body:

  • Add helper functions for getting runtime information from functions, e.g. the class object, the record descriptor objects, etc. This will finally allow us to query plugins by e.g. ones that generate records that have a path field, for example.

The goal of this PR is to agree and establish a better base plugin system, and then incrementally improve that.

Schamper avatar Jan 27 '25 13:01 Schamper

Additionally could FunctionDescriptor.record be populated with record as discussed in https://github.com/fox-it/dissect.target/pull/759?

This is not possible since we can't serialize the record descriptor, so it would just be a string. This functionality will be part of an incremental improvement of this PR as discussed in the main PR body: [...]

It seems like find_functions now supports what we need here. Please disregard our comment :)

JSCU-CNI avatar Jan 30 '25 10:01 JSCU-CNI

And otherwise https://github.com/fox-it/dissect.target/pull/1007 hopefully improves on that 😄

Schamper avatar Jan 30 '25 11:01 Schamper