[plaso] Refactored preprocess plugins to directly use knowledge base #241

[plaso] Refactored preprocess plugins to directly use knowledge base #241

[onager, 2016-08-14 13:53:42]

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface.py
File plaso/preprocessors/interface.py (left):

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:50: WEIGHT = 3
How do you plan to deal with dependencies in preprocessing in the future?

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:23: """Class that defines the file system
preprocess plugin interface."""
-Class that

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:67: """Runs the plugin to determine the value
of the preprocessing attribute.
This sets the value in the KB - determine to me sounds like it returns
something.

"Determines the value of the preprocessing attributes and sets it in the
knowledge base"?

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:76: """Class that defines the file preprocess
plugin interface."""
-Class that
or even
-Class that defines the

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/manager.p...
plaso/preprocessors/manager.py:193: u'Unable to run preproces plugin: {0:s} with
error: {1:s}'.format(
preprocess

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/manager.p...
plaso/preprocessors/manager.py:216: u'Unable to run preproces plugin: {0:s} with
error: {1:s}'.format(
preprocess

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:135: """Parses Windows Registry value data for a
preprocessing attribute.
Make docstring more specific.

Also, this method sets a value in the KB, which is an import side-effect to
note.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
Make docstring more specific.

Also, this method sets a value in the KB, which is an import side-effect to
note.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:211: """Parses a Windows Registry key for a
preprocessing attribute.
Make docstring more specific.

Also, this method sets a value in the KB, which is an import side-effect to
note.

[Joachim Metz, 2016-08-14 14:05:58]

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface.py
File plaso/preprocessors/interface.py (left):

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:50: WEIGHT = 3
Calculating the pre-processing order.

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:23: """Class that defines the file system
preprocess plugin interface."""
On 2016/08/14 13:53:41, onager wrote:
> -Class that

Done.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:67: """Runs the plugin to determine the value
of the preprocessing attribute.
> "Determines the value of the preprocessing attributes and sets it in the
> knowledge base"?

too long.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/interface...
plaso/preprocessors/interface.py:76: """Class that defines the file preprocess
plugin interface."""
On 2016/08/14 13:53:41, onager wrote:
> -Class that
> or even
> -Class that defines the

Done.

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/manager.p...
plaso/preprocessors/manager.py:193: u'Unable to run preproces plugin: {0:s} with
error: {1:s}'.format(
On 2016/08/14 13:53:41, onager wrote:
> preprocess

Done.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/manager.p...
plaso/preprocessors/manager.py:216: u'Unable to run preproces plugin: {0:s} with
error: {1:s}'.format(
On 2016/08/14 13:53:41, onager wrote:
> preprocess

Done.

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:135: """Parses Windows Registry value data for a
preprocessing attribute.
Not sure what you mean.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
Not sure what you mean.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:211: """Parses a Windows Registry key for a
preprocessing attribute.
Note sure what you mean.

[Joachim Metz, 2016-08-14 14:08:25]

Code updated.

[onager, 2016-08-14 14:09:24]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:135: """Parses Windows Registry value data for a
preprocessing attribute.
On 2016/08/14 14:05:58, Joachim Metz wrote:
> Not sure what you mean.

the "preprocessing attribute" here you already know, it's
operating_system_version.

This method calls knowledge_base.SetValue(), which is setting a value, not
parsing something.

[Joachim Metz, 2016-08-14 14:26:59]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:135: """Parses Windows Registry value data for a
preprocessing attribute.
Isn't the function parsing the value from the Registry key?

Please provide an example of what you mean, this will save roundtrips of trying
to explain what you mean.

[onager, 2016-08-14 16:58:17]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:25: value_data (object): Windows Registry value
data.
str, not object

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:52: """Parses Windows Registry value data for a
preprocessing attribute.
"Parses hostname value data and sets knowledge base value."

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:56: value_data (object): Windows Registry value
data.
str, not object

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:105: value_data (object): Windows Registry value
data.
str, not object

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:139: value_data (object): Windows Registry value
data.
This looks to actually require str, not object.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
On 2016/08/14 14:05:57, Joachim Metz wrote:
> Not sure what you mean.

"Parses time zone data and sets knowledge base value."

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:166: value_data (object): Windows Registry value
data.
requires str, not object.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:211: """Parses a Windows Registry key for a
preprocessing attribute.
On 2016/08/14 14:05:58, Joachim Metz wrote:
> Note sure what you mean.

"Parses a user account key and sets knowledge base values."

[Joachim Metz, 2016-08-14 17:15:32]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:25: value_data (object): Windows Registry value
data.
No value_data is an object, it can be int, str, bytes, list[str].

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:52: """Parses Windows Registry value data for a
preprocessing attribute.
what is "hostname value data" ?

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:56: value_data (object): Windows Registry value
data.
same comments as before

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:105: value_data (object): Windows Registry value
data.
same comment as before

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:139: value_data (object): Windows Registry value
data.
we want an str, but that is not necessarily what we get

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
what is "time zone data" ?

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:166: value_data (object): Windows Registry value
data.
same comment as before

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:211: """Parses a Windows Registry key for a
preprocessing attribute.
It parses the profile list key not user account keys which are stored in the
SAM.

[onager, 2016-08-14 17:38:37]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:25: value_data (object): Windows Registry value
data.
On 2016/08/14 17:15:31, Joachim Metz wrote:
> No value_data is an object, it can be int, str, bytes, list[str].

It can, but then we get an exception straight away, so it looks to me like this
function requires a string to work properly. Per the style guide: "A docstring
should give enough information to write a call to the function without reading
the function's code" it's not possible to write a call to this function without
reading the code to see what's going to trigger the fail exception.

Given this is called almost like a callback, you could perhaps change the
Raises: section to indicate that the function will raise if value_data is not a
string. "not supported" doesn't hint at what type is required.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:112: u'Unsupported Registry key: {0:s}, value:
{1:s} type.'.format(
It would be helpful to mention the desired and found types here as well.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
On 2016/08/14 17:15:31, Joachim Metz wrote:
> what is "time zone data" ?

A suggestion ;)

"Parses the time zone value data and updates knowledge base"
"Parses Windows Registry value data for the time zone attribute"
"Sets the time zone preprocess attribute from Registry Value data"
"Processes Registry value data and sets time zone attribute"

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:218: errors.PreProcessFail: if the preprocessing
fails.
Where is this raised?

[Joachim Metz, 2016-08-14 17:54:35]

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

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:25: value_data (object): Windows Registry value
data.
> Per the style guide: "A docstring should give enough information
> to write a call to the function without reading the function's
> code"

This is a callback not an public method. The docstring is providing information
in that context, str would be providing too limited information.

> section to indicate that the function will raise if value_data is
> not a string

That would be accurate. I'll add some text.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:112: u'Unsupported Registry key: {0:s}, value:
{1:s} type.'.format(
On 2016/08/14 17:38:37, onager wrote:
> It would be helpful to mention the desired and found types here as well.

Done.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:161: def _ParseValueData(self, knowledge_base,
value_data):
I find these more confusing then the boilerplate docstring.

So I'm keeping it as-is. These are likely to change when the preprocessing
becomes artifact driven.

https://codereview.appspot.com/306070043/diff/1/plaso/preprocessors/windows.p...
plaso/preprocessors/windows.py:218: errors.PreProcessFail: if the preprocessing
fails.
On 2016/08/14 17:38:37, onager wrote:
> Where is this raised?

Done.

[Joachim Metz, 2016-08-14 17:57:04]

Code updated.

[Joachim Metz, 2016-08-27 05:35:13]

Code updated.

[Joachim Metz, 2016-09-24 10:26:45]

Code updated.

[Joachim Metz, 2016-10-16 15:58:02]

Code updated.

[Joachim Metz, 2016-10-24 19:10:37]

Code updated.

[onager, 2016-10-25 15:41:56]

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

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:49: """Parses a hostname file-like object.
time zone

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:63: class
LinuxUserAccountsPreprocessPlugin(interface.FilePreprocessPlugin):
LinuxPasswdPreprocessPlugin is better here - it's consistent with the other
preprocessors

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
File plaso/preprocessors/manager.py (right):

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
plaso/preprocessors/manager.py:193: u'Unable to run preprocess plugin: {0:s}
with error: {1:s}'.format(
-run +complete, or something similar

We were able to run (start) the plugin, but it blew up and didn't finish
properly

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
plaso/preprocessors/manager.py:216: u'Unable to run preprocess plugin: {0:s}
with error: {1:s}'.format(
-run +complete

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

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:183: timezone =
time_zones.TIME_ZONES.get(lookup_key, value_data)
Please add an error message if we can't map the timezone.

try:
timezone  = time_zones.time_zones.TIME_ZONES.get(lookup_key)
except:
time_zone = value_data
logging.warn("Unable to map {0:s} to a timezone".format(value_data)

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
File tests/preprocessors/test_lib.py (right):

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:39: """Runs the Windows Registry preprocess
plugin.
-the +a

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:63: """Runs the Windows Registry preprocess
plugin on a SOFTWARE file.
-the +a

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:83: """Runs the Windows Registry preprocess
plugin on a SYSTEM file.
-the +a

[Joachim Metz, 2016-10-25 18:49:10]

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

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:49: """Parses a hostname file-like object.
On 2016/10/25 15:41:55, onager wrote:
> time zone

Done.

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/linu...
plaso/preprocessors/linux.py:63: class
LinuxUserAccountsPreprocessPlugin(interface.FilePreprocessPlugin):
On 2016/10/25 15:41:55, onager wrote:
> LinuxPasswdPreprocessPlugin is better here - it's consistent with the other
> preprocessors

I picked LinuxUserAccountsPreprocessPlugin since that is more consistent with
macOS and Windows ;)

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
File plaso/preprocessors/manager.py (right):

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
plaso/preprocessors/manager.py:193: u'Unable to run preprocess plugin: {0:s}
with error: {1:s}'.format(
On 2016/10/25 15:41:55, onager wrote:
> -run +complete, or something similar
> 
> We were able to run (start) the plugin, but it blew up and didn't finish
> properly

Done.

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/mana...
plaso/preprocessors/manager.py:216: u'Unable to run preprocess plugin: {0:s}
with error: {1:s}'.format(
On 2016/10/25 15:41:55, onager wrote:
> -run +complete

Done.

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

https://codereview.appspot.com/306070043/diff/120001/plaso/preprocessors/wind...
plaso/preprocessors/windows.py:183: timezone =
time_zones.TIME_ZONES.get(lookup_key, value_data)
On 2016/10/25 15:41:55, onager wrote:
> Please add an error message if we can't map the timezone.
> 
> try:
> timezone  = time_zones.time_zones.TIME_ZONES.get(lookup_key)
> except:
> time_zone = value_data
> logging.warn("Unable to map {0:s} to a timezone".format(value_data)

Good point

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
File tests/preprocessors/test_lib.py (right):

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:39: """Runs the Windows Registry preprocess
plugin.
On 2016/10/25 15:41:55, onager wrote:
> -the +a

Done.

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:63: """Runs the Windows Registry preprocess
plugin on a SOFTWARE file.
On 2016/10/25 15:41:55, onager wrote:
> -the +a

Done.

https://codereview.appspot.com/306070043/diff/120001/tests/preprocessors/test...
tests/preprocessors/test_lib.py:83: """Runs the Windows Registry preprocess
plugin on a SYSTEM file.
On 2016/10/25 15:41:55, onager wrote:
> -the +a

Done.

[Joachim Metz, 2016-10-25 18:53:26]

Code updated.

[Joachim Metz, 2016-10-26 06:28:51]

Code updated.

[onager, 2016-11-01 15:08:06]

LGTM

[Joachim Metz, 2016-11-01 19:28:22]

Code updated.

[Joachim Metz, 2016-11-01 19:36:24]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: python ./utils/review.py close preprocess1