[plaso] Added System Resource Usage Monitor (SRUM) ESE plugin #427

[plaso] Added System Resource Usage Monitor (SRUM) ESE plugin #427

[Joachim Metz, 2018-01-16 07:53:35]

Code updated.

[Joachim Metz, 2018-01-17 22:13:08]

Code updated.

[onager, 2018-01-18 01:46:42]

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:26: application (int): application.
str?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:28: background_bytes_written (int):
background number of bytes written.
Please add a description of what these values mean, or a link to some
documentation.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:47: user_identifier (int): user identifier,
which is a Windows NT security
Isn't this a str?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:81: application (int): application.
str?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:86: user_identifier (int): user identifier,
which is a Windows NT security
str

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:108: application (int): application.
str

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:138: NAME = 'esedb_srum'
Just srum, otherwise the chain is going to look weird - 'esedb/esedb_strum'

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:220: """Retrieves the identifier mappings
from SruDbIdMapTable table.
Please rewrite this docstring. It's not clear what  "the identifiers" are, or
where they come from.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:222: Identifier mappings are stored in the
cache for optimization.
I recommend moving the caching to the calling code, but for reference, this
comment is confusing. It's not clear what or who is storing the mappings in the
cache. Something like "this method will cache the mappings after determining
them" is clearer.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:234: identifier_mappings =
cache.GetResults('SruDbIdMapTable', default_value={})
Please move the caching/retrieval to the calling code, rather than including the
functionality here.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:238: esedb_table =
database.get_table_by_name('SruDbIdMapTable')
This method would be clearer if you passed the table in, instead of the database
handler. Is this method call expensive?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:255: 'IdIndex value missing from parse
table: SruDbIdMapTable')
What's a parse table?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:264: mapping = record_values.get('IdBlob',
None)
Rename "mapping" to something else (name?) because you describe the pairs of
str, str as the "mapping" in the method docstring.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:270: if identifier_type not in (0, 1, 2, 3):
Extract these values to a class constant. Also check the identifier_type value
directly after retrieving it, rather than getting the value of IdBlob and then
aborting.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:306: """Parses a table with a GUID as name.
I don't see how the name of the table is relevant to what this method does.
Rename to _ParseSRUMTable?

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:315: values_map (dict[str, str]): maps table
column names to attribute names.
"mapping of table columns to event data attribute names"

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:319: logging.warning('[{0:s}] invalid
cache'.format(self.NAME))
Raise ValueError, instead of logging. Also, the messages here are confusing,
these args don't seem to be invalid, but not present.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:345: if attribute_name in ('application',
'user_identifier'):
This took me a long time to understand - please add a comment like "comment and
user_identifer are stored as foreign keys with their values in the
SruDbIdMapTable table" or similar.

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
File tests/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
tests/parsers/esedb_plugins/srum.py:22: def testProcess(self):
Please add tests for parsing events from the other tables.

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
tests/parsers/esedb_plugins/srum.py:27:
self.assertEqual(storage_writer.number_of_events, 18543)
Please add a test for the number of different events produced here.

[Joachim Metz, 2018-01-18 06:52:05]

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:26: application (int): application.
ack, thx, I forgot to update this

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:28: background_bytes_written (int):
background number of bytes written.
There is no good documentation at the moment. It is all fragmented or not
publicly accessible. I'm working on this:
https://github.com/libyal/esedb-kb/blob/master/documentation/System%20Resourc...

However I think there should be a broader document for SRUM likely here:
https://github.com/ForensicArtifacts/artifacts-kb

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:47: user_identifier (int): user identifier,
which is a Windows NT security
ack, thx, I forgot to update this

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:81: application (int): application.
On 2018/01/18 01:46:40, onager wrote:
> str?

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:86: user_identifier (int): user identifier,
which is a Windows NT security
On 2018/01/18 01:46:41, onager wrote:
> str

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:108: application (int): application.
On 2018/01/18 01:46:41, onager wrote:
> str

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:138: NAME = 'esedb_srum'
On 2018/01/18 01:46:41, onager wrote:
> Just srum, otherwise the chain is going to look weird - 'esedb/esedb_strum'

Acknowledged. I'll change that for file history as well then

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:220: """Retrieves the identifier mappings
from SruDbIdMapTable table.
I'll not rewrite the text but will add some clarification.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:222: Identifier mappings are stored in the
cache for optimization.
On 2018/01/18 01:46:41, onager wrote:
> I recommend moving the caching to the calling code, but for reference, this
> comment is confusing. It's not clear what or who is storing the mappings in
the
> cache. Something like "this method will cache the mappings after determining
> them" is clearer.

Acknowledged.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:234: identifier_mappings =
cache.GetResults('SruDbIdMapTable', default_value={})
why? what is the advantage/added value?

IMHO moving this to the caller (_ParseGUIDTable) would leak concerns of this
method into the caller

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:238: esedb_table =
database.get_table_by_name('SruDbIdMapTable')
> Is this method call expensive?

Yes? 

moving this to the caller (_ParseGUIDTable) would leak concerns of this method
into the caller

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:255: 'IdIndex value missing from parse
table: SruDbIdMapTable')
On 2018/01/18 01:46:41, onager wrote:
> What's a parse table?

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:264: mapping = record_values.get('IdBlob',
None)
On 2018/01/18 01:46:41, onager wrote:
> Rename "mapping" to something else (name?) because you describe the pairs of
> str, str as the "mapping" in the method docstring.

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:270: if identifier_type not in (0, 1, 2, 3):
> Also check the identifier_type value directly after retrieving it

the intent is to make sure all 3 values are available first

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:306: """Parses a table with a GUID as name.
to distinguish the function from parsing tables with other names, such as
SruDbIdMapTable. Keeping it as-is.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:315: values_map (dict[str, str]): maps table
column names to attribute names.
On 2018/01/18 01:46:41, onager wrote:
> "mapping of table columns to event data attribute names"

Done.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:319: logging.warning('[{0:s}] invalid
cache'.format(self.NAME))
changed this for the other esedb plugins as well

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:345: if attribute_name in ('application',
'user_identifier'):
On 2018/01/18 01:46:41, onager wrote:
> This took me a long time to understand - please add a comment like "comment
and
> user_identifer are stored as foreign keys with their values in the
> SruDbIdMapTable table" or similar.

Done.

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
File tests/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
tests/parsers/esedb_plugins/srum.py:22: def testProcess(self):
On 2018/01/18 01:46:41, onager wrote:
> Please add tests for parsing events from the other tables.

Acknowledged.

https://codereview.appspot.com/340160043/diff/40001/tests/parsers/esedb_plugi...
tests/parsers/esedb_plugins/srum.py:27:
self.assertEqual(storage_writer.number_of_events, 18543)
On 2018/01/18 01:46:42, onager wrote:
> Please add a test for the number of different events produced here.

Acknowledged.

[Joachim Metz, 2018-01-18 07:24:04]

Code updated.

[onager, 2018-01-18 10:46:21]

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:238: esedb_table =
database.get_table_by_name('SruDbIdMapTable')
On 2018/01/18 06:52:05, Joachim Metz wrote:
> > Is this method call expensive?
> 
> Yes? 
> 
> moving this to the caller (_ParseGUIDTable) would leak concerns of this method
> into the caller
> 

This method has too many concerns already - it's checking a cache, reading a
table, validating fields, converting other fields, and then loading a cache and
returning.  

This would be a lot simpler if the caller did the cache management, and this
method just did the parsing, validating and converting.

The calling method is already concerned with the cache, as it has to pass it in
to this method, so anyone reading that method still has to know why it's being
passed in there, and what's it's being used for.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:270: if identifier_type not in (0, 1, 2, 3):
On 2018/01/18 06:52:05, Joachim Metz wrote:
> > Also check the identifier_type value directly after retrieving it
> 
> the intent is to make sure all 3 values are available first

But why? You won't use the value of IdBlob, even if it's there. What's the extra
value in having a "no blob" error message vs a "invalid type" error message?

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:269: 'IdIndex value missing from table:
SruDbIdMapTable')
Remove the redundant : here

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:362: # the GUID table. Obtain their string
representations from the
This is still unclear - these values are keys to the actual values in the other
table. Something like 'DiagTrack' isn't the string representation of '1', it's
the value stored under that key.

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:363: # identifier mappings stored in the
table: SruDbIdMapTable.
Remove redundant :

[Joachim Metz, 2018-01-18 18:00:22]

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:238: esedb_table =
database.get_table_by_name('SruDbIdMapTable')
so then it is better to further split this method into sub methods than
offloading the responsibility to the caller IMHO.

https://codereview.appspot.com/340160043/diff/40001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:270: if identifier_type not in (0, 1, 2, 3):
Ah that is what you meant. Yes I can combine the type and sanity check for
IdBlob. Thought you were talking about the order of the checks.

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:269: 'IdIndex value missing from table:
SruDbIdMapTable')
On 2018/01/18 10:46:21, onager wrote:
> Remove the redundant : here

Keeping as-is for consistency and visual aid

e.g.
https://github.com/log2timeline/plaso/blob/master/plaso/parsers/winlnk.py#L209

If you want to change this, we need to update the style guide and fix it for the
entire codebase.

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:362: # the GUID table. Obtain their string
representations from the
I'll reword, but for context from an application level it is the string
representation. ESE is an application level database.

https://codereview.appspot.com/340160043/diff/60001/plaso/parsers/esedb_plugi...
plaso/parsers/esedb_plugins/srum.py:363: # identifier mappings stored in the
table: SruDbIdMapTable.
On 2018/01/18 10:46:21, onager wrote:
> Remove redundant :

Keeping as-is for consistency and visual aid

[Joachim Metz, 2018-01-18 18:05:36]

Code updated.

[Joachim Metz, 2018-01-18 18:26:08]

Code updated.

[Joachim Metz, 2018-01-18 18:32:18]

Code updated.

[Joachim Metz, 2018-01-18 18:37:54]

Code updated.

[onager, 2018-01-19 09:09:48]

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:307: # mapping. Here we look up the numeric
identifier stored the GUID
+in

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:339: esedb_table (pyesedb.table): table.
The table is unnecessary here, just pass the name.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:340: esedb_record (pyesedb.table): record.
Please fix, this is a record not a table.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:343: tuple[int, str]: numeric identifier and
its string representation.
+Or None, None if the record can't be parsed.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:346: ValueError: if the cache, database or
table value is missing.
Remove, this doesn't raise any more, and has different args

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:403: ValueError: if the cache, database or
table value is missing.
Remove, this no longer raises, and has different args

[Joachim Metz, 2018-01-20 14:00:01]

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:307: # mapping. Here we look up the numeric
identifier stored the GUID
On 2018/01/19 09:09:47, onager wrote:
> +in

Done.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:339: esedb_table (pyesedb.table): table.
On 2018/01/19 09:09:47, onager wrote:
> The table is unnecessary here, just pass the name.

Done.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:340: esedb_record (pyesedb.table): record.
On 2018/01/19 09:09:47, onager wrote:
> Please fix, this is a record not a table.

Done.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:343: tuple[int, str]: numeric identifier and
its string representation.
On 2018/01/19 09:09:47, onager wrote:
> +Or None, None if the record can't be parsed.

Done.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:346: ValueError: if the cache, database or
table value is missing.
On 2018/01/19 09:09:47, onager wrote:
> Remove, this doesn't raise any more, and has different args

Done.

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:403: ValueError: if the cache, database or
table value is missing.
On 2018/01/19 09:09:47, onager wrote:
> Remove, this no longer raises, and has different args

Done.

[Joachim Metz, 2018-01-20 14:04:47]

Code updated.

[baby546833, 2018-01-21 16:49:58]

I love my wife

[baby546833, 2018-01-21 16:53:25]

love my wife

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:403: ValueError: if the cache, database or
table value is missing.
I love my wife

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:403: ValueError: if the cache, database or
table value is missing.
 I love my wife

https://codereview.appspot.com/340160043/diff/140001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:405: identifier_mappings = {}
I love my wife

[onager, 2018-01-24 07:30:24]

Couple of small nits, LGTM

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/file_history.py (right):

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/file_history.py:100: raise ValueError('Missing
database value.')
-value, or and replace with "argument" or "Missing value for database argument",
if you think there's disambiguation between possible things "database" could be.

Also the other, similar exception messages.

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:23: Attributes:
+link to
https://github.com/libyal/esedb-kb/blob/master/documentation/System%20Resourc...
and a comment that the meaning of all these values is not yet documented.

[Joachim Metz, 2018-01-24 07:52:34]

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/file_history.py (right):

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/file_history.py:100: raise ValueError('Missing
database value.')
let's chat about this first and update the style guide if you really think this
is needed. The Python error is Value error. The ambiguity will remain even if we
change it.

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
File plaso/parsers/esedb_plugins/srum.py (right):

https://codereview.appspot.com/340160043/diff/160001/plaso/parsers/esedb_plug...
plaso/parsers/esedb_plugins/srum.py:23: Attributes:
link was already added to the parser, I've moved it to the top.

[Joachim Metz, 2018-01-24 07:57:29]

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