-
Notifications
You must be signed in to change notification settings - Fork 352
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
Artifact definitions filter helper #1883
Artifact definitions filter helper #1883
Conversation
…om/jnettesheim/plaso into artifact-definitions-filter-helper
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.
Please add tests, there are a couple of complex functions added where I would like to see tests to better understand them.
Besides that various style nits.
plaso/engine/artifact_filters.py
Outdated
from dfwinreg import registry_searcher | ||
|
||
from plaso.engine import path_helper | ||
|
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.
- white line
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.
Done
plaso/engine/artifact_filters.py
Outdated
from plaso.engine import path_helper | ||
|
||
class ArtifactDefinitionsFilterHelper(object): | ||
"""Helper to create filters based on artifact defintions. |
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.
defintions => definitions
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.
Done
plaso/engine/artifact_filters.py
Outdated
class ArtifactDefinitionsFilterHelper(object): | ||
"""Helper to create filters based on artifact defintions. | ||
|
||
Builds extraction and parsing filters from forensic artifact definitions. |
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.
what are "parsing filters" ?
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.
Done.
plaso/engine/artifact_filters.py
Outdated
https://github.com/ForensicArtifacts/artifacts/blob/master/docs/Artifacts%20definition%20format%20and%20style%20guide.asciidoc | ||
""" | ||
|
||
ARTIFACT_FILTERS = 'ARTIFACT_FILTERS' |
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.
If these are only used by this class please prefix with an underscore.
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.
Removed several of these and added underscore to one only used in this class.
plaso/engine/artifact_filters.py
Outdated
self._knowledge_base = knowledge_base | ||
|
||
def BuildFindSpecs(self, environment_variables=None): | ||
"""Build find specification from a forensic artifact definitions. |
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.
Build => Builds
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.
Done.
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.
Done.
plaso/engine/path_helper.py
Outdated
@@ -133,3 +147,26 @@ def GetRelativePathForPathSpec(cls, path_spec, mount_path=None): | |||
location = location[len(mount_path):] | |||
|
|||
return location | |||
|
|||
@classmethod | |||
def ExpandUserHomeDirPath(cls, path, user_accounts): |
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.
Windows calls this profile directory not home directory, also don's use abbreviations such as Dir
. This is more clear for an international audience.
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 could be for any operating system. Removed abbreviation.
tests/engine/artifact_filters.py
Outdated
|
||
@shared_test_lib.skipUnlessHasTestFile(['artifacts']) | ||
@shared_test_lib.skipUnlessHasTestFile(['SYSTEM']) | ||
def testBuildRegistryFindSpecs(self): |
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.
testBuildRegistryFindSpecs => testBuildFindSpecsWithRegistry or testBuildFindSpecsOnRegistry
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.
Done.
tests/engine/artifact_filters.py
Outdated
@shared_test_lib.skipUnlessHasTestFile(['artifacts']) | ||
@shared_test_lib.skipUnlessHasTestFile(['SYSTEM']) | ||
def testBuildRegistryFindSpecs(self): | ||
"""Tests the BuildFindSpecs function for registry artifacts.""" |
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.
for => on
registry => Registry
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.
Done.
tests/engine/artifact_filters.py
Outdated
@shared_test_lib.skipUnlessHasTestFile(['System.evtx']) | ||
@shared_test_lib.skipUnlessHasTestFile(['testdir', 'filter_1.txt']) | ||
@shared_test_lib.skipUnlessHasTestFile(['testdir', 'filter_3.txt']) | ||
def testBuildFileFindSpecs(self): |
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.
testBuildFileFindSpecs => testBuildFindSpecsWithFileSystem or testBuildFindSpecsOnFileSystem
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.
Done.
tests/engine/artifact_filters.py
Outdated
|
||
# Three key paths found | ||
self.assertEqual(len(key_paths), 3) | ||
|
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.
Please add tests for BuildFindSpecsFromFileArtifact
, _CheckKeyCompatibility
, _ExpandRecursiveGlobs
, _BuildRecursivePaths
. The last 2 methods I have a hard time understanding from the code directly.
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.
Done.
Codecov Report
@@ Coverage Diff @@
## master #1883 +/- ##
==========================================
+ Coverage 84.03% 84.05% +0.01%
==========================================
Files 435 436 +1
Lines 30998 31174 +176
==========================================
+ Hits 26049 26202 +153
- Misses 4949 4972 +23
Continue to review full report at Codecov.
|
@joachimmetz - PTAL. |
@jnettesheim I'm having a look this weekend. |
plaso/engine/artifact_filters.py
Outdated
dynamically populate environment variables in key. | ||
user_accounts (list[str]): identified user accounts stored in the | ||
knowledge base. | ||
find_specs (dict[artifacts.artifact_types]): Dictionary containing |
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.
find_specs (dict[artifacts.artifact_types]): Dictionary containing ...
I had a brief look at this but this arg looks weird to me. Find specs are a dfVFS class, but type information says artifacts.artifact_types ?
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.
returning a list of dfvfs.FindSpect will simplify the interface.
@jnettesheim I've started looking at this. I've made some minor changes, mainly nits about docstrings and style formatting. I'll continue as soon as time permits |
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.
more comments and changes
plaso/engine/artifact_filters.py
Outdated
|
||
from __future__ import unicode_literals | ||
|
||
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.
Use from plaso.engine import logger
instead of logging
plaso/engine/artifact_filters.py
Outdated
""" | ||
find_specs = {} | ||
|
||
artifact_definitions = [] |
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 loop can be merged with the next loop and remove the need for the artifact_definitions list
plaso/engine/artifact_filters.py
Outdated
dynamically populate environment variables in key. | ||
user_accounts (list[str]): identified user accounts stored in the | ||
knowledge base. | ||
find_specs (dict[artifacts.artifact_types]): Dictionary containing |
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.
returning a list of dfvfs.FindSpect will simplify the interface.
tests/engine/path_helper.py
Outdated
'\\Windows\\Test\\*\\*\\*\\*\\*\\*\\*\\*', | ||
'\\Windows\\Test\\*\\*\\*\\*\\*\\*\\*\\*\\*', | ||
'\\Windows\\Test\\*\\*\\*\\*\\*\\*\\*\\*\\*\\*'] | ||
self.assertItemsEqual(paths, check_paths) |
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.
not portable to python 3
Instance of 'PathHelperTest' has no 'assertItemsEqual' member (no-member)
plaso/engine/path_helper.py
Outdated
@@ -3,6 +3,9 @@ | |||
|
|||
from __future__ import unicode_literals | |||
|
|||
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.
user engine.logger instead
plaso/engine/path_helper.py
Outdated
@@ -42,9 +49,19 @@ def ExpandWindowsPath(cls, path, environment_variables): | |||
not path_segment.endswith('%')): | |||
continue | |||
|
|||
lookup_key = path_segment.upper()[1:-1] | |||
check_for_drive_letter = False | |||
if path_segment.upper().startswith('%%ENVIRON_'): |
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.
store upper case path in a variable seeing you're reusing it
plaso/engine/path_helper.py
Outdated
_, _, new_path = new_path.rpartition(':') | ||
user_paths.append(new_path) | ||
else: | ||
user_paths = [path] |
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.
drive letter is not stripped here
tests/engine/artifact_filters.py
Outdated
path_segments = ['Users', 'testuser2', 'Documents', 'WindowsPowerShell', | ||
'profile\\.ps1'] | ||
self.assertEqual( | ||
find_specs[artifact_types.TYPE_INDICATOR_FILE][2]._location_segments, |
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.
seeing file_specs is a dict this test will be flaky
Yes pending completion of CI tests, then I'll merge |
One line description of pull request
Issue 1313, plaso engine updates to add forensic artifact filtering capabilities.
Description:
Per discussion in #1732 adding plaso engine changes in separate PR.
Related issue (if applicable): fixes #
Notes:
All contributions to Plaso undergo code
review. This makes sure
that the code has appropriate test coverage and conforms to the Plaso style
guide.
One of the maintainers will examine your code, and may request changes.
Checklist:
If new dependencies are required: