[plaso] Made preprocessing artifact driven #155

[plaso] Made preprocessing artifact driven #155

Dependent on:
* plaso https://codereview.appspot.com/326740043/

[Joachim Metz, 2014-09-27 09:15:57]

Very early state of the CL, put this out there for discussion purposes.

[kiddi, 2014-11-04 05:28:33]

I've got few comments here, let's just send them away so that you at least have
something to start with.

Then we can discuss this a bit further.

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows.yaml
File data/artifacts/windows.yaml (right):

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows.yaml#n...
data/artifacts/windows.yaml:12: supported_os: [Windows]
does Windows not have to be wrapped up in a quote?

supported_os: ['Windows']

Or is that not necessary? Can I do

supported_os: [Windows,Linux]
?

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows_paths....
File data/artifacts/windows_paths.yaml (right):

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows_paths....
data/artifacts/windows_paths.yaml:24: name: '%ProgramFiles%'
so we are going with % to denote the attribute, vs {}, vs something else?

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py
File plaso/artifacts/definition.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:43: self.providers = []
alpha ordering?

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:115: """The type indicator."""
do we not need a Raises section here?

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:148: return type_indicator
this seems just like the provider type, why not have a common parent here? to
reduce code re-use

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:174: 
alpha ordering

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:226: self.key = value
I guess that was supposed to be 

self.value = value

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py
File plaso/artifacts/registry.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:51: artifact_definition.name))
I guess that is supposed to be artifact_definition_name (not definition.name)

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:86: """
I would at least do some sort of checks to see if this file even exists prior to
opening it.

Also check if you can open it (or if some os.error comes up, for instance due to
permission errors)

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:87: file_object = open(filename, 'rb')
I would also opt here to do a

yaml_generator = None
with open(filename, 'rb') as file_object:
  try:
    yaml_generator = yaml.safe_lo.....


if not yaml_generator:
  return

that way you at least have a certainty that you close the file if things get bad

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:89: # TODO: add try/catch.
I think this todo is simple enough to warrant an implementation before launch

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:95: logging.warning(u'Invalid YAML artifact
definition missing name.')
should we not include some more debugging information? to make it easier to fix
issues in the yaml file later on

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:144: artifact_definition.name))
same here, definition_name vs definition.name

https://codereview.appspot.com/150170043/diff/1/plaso/preprocessors/artifacts.py
File plaso/preprocessors/artifacts.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/preprocessors/artifacts...
plaso/preprocessors/artifacts.py:51: location_string))
shouldn't there be a continue then here?

[Joachim Metz, 2014-11-23 09:29:33]

Merging with work on stand-alone artifact definition Python module:
https://github.com/joachimmetz/artifacts

[kiddi, 2014-12-13 03:37:01]

What is the current status of this? Are we hoping to push this in before the
release or pause until after release?

[Joachim Metz, 2014-12-13 07:28:44]

After

On Sat, Dec 13, 2014 at 4:37 AM, <kiddi@kiddaland.net> wrote:
>
> What is the current status of this? Are we hoping to push this in before
> the release or pause until after release?
>
> https://codereview.appspot.com/150170043/
>
> --
> remember the documentation site: plaso.kiddaland.net
> ---You received this message because you are subscribed to the Google
> Groups "log2timeline-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to log2timeline-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to log2timeline-dev@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[Joachim Metz, 2017-04-09 13:53:50]

Code updated.

[Joachim Metz, 2017-05-14 06:17:02]

Code updated.

[Joachim Metz, 2017-05-14 06:41:54]

Code updated.

[Joachim Metz, 2017-05-20 17:43:09]

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows.yaml
File data/artifacts/windows.yaml (right):

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows.yaml#n...
data/artifacts/windows.yaml:12: supported_os: [Windows]
not necessary

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows_paths....
File data/artifacts/windows_paths.yaml (right):

https://codereview.appspot.com/150170043/diff/1/data/artifacts/windows_paths....
data/artifacts/windows_paths.yaml:24: name: '%ProgramFiles%'
not sure what you mean, but CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py
File plaso/artifacts/definition.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:43: self.providers = []
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:115: """The type indicator."""
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:148: return type_indicator
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:174: 
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/definition.py...
plaso/artifacts/definition.py:226: self.key = value
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py
File plaso/artifacts/registry.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:51: artifact_definition.name))
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:86: """
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:87: file_object = open(filename, 'rb')
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:89: # TODO: add try/catch.
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:95: logging.warning(u'Invalid YAML artifact
definition missing name.')
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/artifacts/registry.py#n...
plaso/artifacts/registry.py:144: artifact_definition.name))
CL has changed

https://codereview.appspot.com/150170043/diff/1/plaso/preprocessors/artifacts.py
File plaso/preprocessors/artifacts.py (right):

https://codereview.appspot.com/150170043/diff/1/plaso/preprocessors/artifacts...
plaso/preprocessors/artifacts.py:51: location_string))
CL has changed

[Joachim Metz, 2017-05-20 19:25:53]

Code updated.

[Joachim Metz, 2017-05-21 19:35:12]

Code updated.

[Joachim Metz, 2017-05-22 17:48:34]

PTAL

[onager, 2017-06-01 02:38:19]

https://codereview.appspot.com/150170043/diff/120001/plaso/cli/helpers/artifa...
File plaso/cli/helpers/artifact_definitions.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/cli/helpers/artifa...
plaso/cli/helpers/artifact_definitions.py:59: data_location =
getattr(configuration_object, u'_data_location', None)
Why is this a protected attribute, if its needed by other objects? Please
refactor.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
File plaso/preprocessors/interface.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:13: class ArtifactMapping(object):
Having read the rest of the CL, I think these are actually mappers, not
mappings. They map a filesystem and an artifact definition to a knowledge base
attribute. 

Mapping to me sounds like the value object that would hold the tuple of
(artifact name,  knowledge base attribute name, value), or similar.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:14: """The artifact mapping interface."""
Please add a description of what an artifact mapping is, and does.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:15: 
Child classes all seem to have a Collect() method, so please define it in the
interface.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:16: ARTIFACT_DEFINTION_NAME = None
Please add a docstring describing the meaning of this constant. It seems to
indicate which artifact should be processed by the mapper, so please add that.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:20: """File system artifact mapping."""
This looks like an interface, not a class that should be instantiated. Please
mention this.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:26: """Parses artifact file system data for a
preprocessing attribute.
What's "artifact file system data"? Please clarify

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:50: artifact_definition
(artifacts.ArtifactDefinition): artifact defintition.
definition

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:65: if source.separator !=
file_system.PATH_SEPARATOR:
Please add a comment about why this code is here - so that separators used in
the artifact source definition aren't meaningful?
If this is the case, perhaps this should actually be something done by the
findspec, ultimately?

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:135: """File artifact mapping."""
+interface?

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:224: # CurrentControl set so we correct it here
for now.
CurrentControlSet

Can you reference an issue in the artifacts project here too, please?

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:247: """Windows Registry value artifact
mapping."""
+interface

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
File plaso/preprocessors/linux.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:17: ARTIFACT_DEFINTION_NAME = u'LinuxHostnameFile'
definition

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:20: """Parses artifact file data for a
preprocessing attribute.
Please rewrite docstring. This change makes it less clear what the method does.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:66: bool: True if preprocessing is complete.
This does seem to be what this return value means. It seems to mean "the
artifact was processed correctly" or something. Please reword.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
File plaso/preprocessors/macos.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
plaso/preprocessors/macos.py:16: """Plist file artifact mapping.
This looks like an interface too, right? and shouldn't be instantiated?

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
plaso/preprocessors/macos.py:237: The method GetKeys fails if the supplied key
does not exist within the
There doesn't seem to be a GetKeys method any more - remove this comment.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
File plaso/preprocessors/windows.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:34: self.ARTIFACT_DEFINTION_NAME,
type(value_data)))
Looks like the parameters are swapped around here.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:124: self.ARTIFACT_DEFINTION_NAME,
type(value_data)))
Params swapped here? Or maybe this is meant to be "Artifact {0:s} has
unsupported type {1:s}"?

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:204: errors.PreProcessFail: if the preprocessing
fails.
How about "...: if the artifact cannot be mapped" or something similar.

[Joachim Metz, 2017-06-04 05:34:49]

Code updated.

[Joachim Metz, 2017-06-04 06:08:12]

Code updated.

[Joachim Metz, 2017-06-04 06:30:32]

https://codereview.appspot.com/150170043/diff/120001/plaso/cli/helpers/artifa...
File plaso/cli/helpers/artifact_definitions.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/cli/helpers/artifa...
plaso/cli/helpers/artifact_definitions.py:59: data_location =
getattr(configuration_object, u'_data_location', None)
leaving as-is for now, seeing clean up is ongoing in different CLs, left a
reminder in https://github.com/log2timeline/plaso/issues/160

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
File plaso/preprocessors/interface.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:13: class ArtifactMapping(object):
for me they are pretty much the same, but I'll reword to PreprocessPlugin. I
just needed a different term for the refactor.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:14: """The artifact mapping interface."""
On 2017/06/01 02:38:18, onager wrote:
> Please add a description of what an artifact mapping is, and does.

Acknowledged.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:15: 
the args are not consistent, so not for now.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:16: ARTIFACT_DEFINTION_NAME = None
On 2017/06/01 02:38:18, onager wrote:
> Please add a docstring describing the meaning of this constant. It seems to
> indicate which artifact should be processed by the mapper, so please add that.

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:20: """File system artifact mapping."""
On 2017/06/01 02:38:18, onager wrote:
> This looks like an interface, not a class that should be instantiated. Please
> mention this.

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:26: """Parses artifact file system data for a
preprocessing attribute.
reworded

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:50: artifact_definition
(artifacts.ArtifactDefinition): artifact defintition.
On 2017/06/01 02:38:18, onager wrote:
> definition

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:65: if source.separator !=
file_system.PATH_SEPARATOR:
On 2017/06/01 02:38:18, onager wrote:
> Please add a comment about why this code is here - so that separators used in
> the artifact source definition aren't meaningful?
> If this is the case, perhaps this should actually be something done by the
> findspec, ultimately?

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:135: """File artifact mapping."""
On 2017/06/01 02:38:18, onager wrote:
> +interface?

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:224: # CurrentControl set so we correct it here
for now.
On 2017/06/01 02:38:18, onager wrote:
> CurrentControlSet
> 
> Can you reference an issue in the artifacts project here too, please?

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:247: """Windows Registry value artifact
mapping."""
On 2017/06/01 02:38:18, onager wrote:
> +interface

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
File plaso/preprocessors/linux.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:17: ARTIFACT_DEFINTION_NAME = u'LinuxHostnameFile'
On 2017/06/01 02:38:18, onager wrote:
> definition

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:20: """Parses artifact file data for a
preprocessing attribute.
On 2017/06/01 02:38:18, onager wrote:
> Please rewrite docstring. This change makes it less clear what the method
does.

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:66: bool: True if preprocessing is complete.
The current wording is correct. The bool indicates when the preprocessor should
stop looking for the preprocessing information.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
File plaso/preprocessors/macos.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
plaso/preprocessors/macos.py:16: """Plist file artifact mapping.
if by your comment to change the docstring then done, otherwise unclear what you
mean

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/maco...
plaso/preprocessors/macos.py:237: The method GetKeys fails if the supplied key
does not exist within the
On 2017/06/01 02:38:19, onager wrote:
> There doesn't seem to be a GetKeys method any more - remove this comment.

Done.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
File plaso/preprocessors/windows.py (right):

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:34: self.ARTIFACT_DEFINTION_NAME,
type(value_data)))
not sure what you mean, the parameters are in the correct order.

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:124: self.ARTIFACT_DEFINTION_NAME,
type(value_data)))
changed it, if you think that is more clear

https://codereview.appspot.com/150170043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:204: errors.PreProcessFail: if the preprocessing
fails.
not changing, changed mapping to preprocessing instead

[Joachim Metz, 2017-06-04 08:36:02]

Code updated.

[onager, 2017-06-05 05:53:01]

LGTM - two minor comments

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/inte...
File plaso/preprocessors/interface.py (right):

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:250: # CurrentControl set so we correct it here
for now.
CurrentControlSet

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/wind...
File plaso/preprocessors/windows.py (right):

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:126: u'Unsupported artifact: {0:s} value type:
{1:s}.'.format(
This (and the similar exception above) is still unclear to me. The problem is
that the data for an attribute is an unexpected type, correct? The artifact
itself is supported, but the type is unexpected. 

Please reword both this exception message, and the other, similar ones.

[Joachim Metz, 2017-07-02 07:08:52]

Code updated.

[Joachim Metz, 2017-07-02 07:15:29]

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/inte...
File plaso/preprocessors/interface.py (right):

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/inte...
plaso/preprocessors/interface.py:250: # CurrentControl set so we correct it here
for now.
On 2017/06/05 05:53:01, onager wrote:
> CurrentControlSet

Done.

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/wind...
File plaso/preprocessors/windows.py (right):

https://codereview.appspot.com/150170043/diff/180001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:126: u'Unsupported artifact: {0:s} value type:
{1:s}.'.format(
On 2017/06/05 05:53:01, onager wrote:
> This (and the similar exception above) is still unclear to me. The problem is
> that the data for an attribute is an unexpected type, correct? The artifact
> itself is supported, but the type is unexpected. 
> 
> Please reword both this exception message, and the other, similar ones.

Acknowledged.

[Joachim Metz, 2017-07-02 07:17:50]

Code updated.

[Joachim Metz, 2017-07-02 08:39:16]

Code updated.

[Joachim Metz, 2017-07-02 09:39:22]

Code updated.

[Joachim Metz, 2017-07-08 17:28:30]

Code updated.

[Joachim Metz, 2017-07-27 06:41:04]

merged via github due to pylint 1.7 linting conflicts