Issue #306: Having entries from mactime parser match those of filestat.

Issue #306: Having entries from mactime parser match those of filestat.

[Joachim Metz, 2015-08-20 03:41:56]

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py#newcode228
plaso/lib/event.py:228: # TODO: Review this (after 1.1.0 release). Is there a
better/more clean
Add an issue to track this TODO and implement it.

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py#newcode243
plaso/lib/event.py:243: # In addition to the timestamp desc we need to remove
the offset
desc (don't use abbreviations)

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

https://codereview.appspot.com/263830043/diff/1/plaso/output/interface.py#new...
plaso/output/interface.py:223: if getattr(event_a, u'parser', u'') in
(u'filestat', u'mactime'):
This logic is now in multiple places in the codebase and thus harder to
maintain. I opt looking for an alternative approach here. At minimum defining
the (u'filestat', u'mactime') as a constant.

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:121: value, key, row_offset, row)
Why deviate from the rest of the code base?

[kiddi, 2015-08-21 21:29:50]

Made few changes here.

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py#newcode228
plaso/lib/event.py:228: # TODO: Review this (after 1.1.0 release). Is there a
better/more clean
On 2015/08/20 03:41:56, Joachim Metz wrote:
> Add an issue to track this TODO and implement it.

shouldn't be a need at the time

https://codereview.appspot.com/263830043/diff/1/plaso/lib/event.py#newcode243
plaso/lib/event.py:243: # In addition to the timestamp desc we need to remove
the offset
On 2015/08/20 03:41:56, Joachim Metz wrote:
> desc (don't use abbreviations)

reworded

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

https://codereview.appspot.com/263830043/diff/1/plaso/output/interface.py#new...
plaso/output/interface.py:223: if getattr(event_a, u'parser', u'') in
(u'filestat', u'mactime'):
On 2015/08/20 03:41:56, Joachim Metz wrote:
> This logic is now in multiple places in the codebase and thus harder to
> maintain. I opt looking for an alternative approach here. At minimum defining
> the (u'filestat', u'mactime') as a constant.

Done.

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:121: value, key, row_offset, row)
On 2015/08/20 03:41:56, Joachim Metz wrote:
> Why deviate from the rest of the code base?

Well... in this case we are following the standard derived from the filestat
parser. In order to treat all file based events the same they'll need to have
the same representation. Whether that is the current form of the filestat parser
or the form used here.

In this case I chose to go with the original filestat parser representation.

Also since these events will be merged when they are found to be the same the
timestamp description tends to be really long if we use the full eventdata
description, eg: "Metadata Modification Time;Last Access Time;Content
Modification Time", vs: "mtime;atime;ctime", etc.

But I'm really not attached to either one of these representations. 

Although what we are doing is here is a 1:1 map between mactime and plaso, as in
we take atime as atime, instead of ctime as "Metadata Modification Time", which
may not be correct depending on the underlying filesystem that was parsed using
fls.... fls reports it as ctime, we are using ctime as the description....and
leaving it to the analyst to interpret that correctly.

[kiddi, 2015-08-21 21:36:17]

Code updated.

[kiddi, 2015-08-21 21:41:41]

Code updated.

[kiddi, 2015-08-21 21:42:49]

.

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:139: FILE_BASED_DATA_TYPES = frozenset([u'fs:stat',
u'fs:mactime:line'])
maybe it is better to import the filestat and mactime formatter here and use the
DATA_TYPE there directly to define this, to avoid potentially this getting
broken if those data types would be changed at any point....

[Joachim Metz, 2015-08-28 10:27:01]

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:121: value, key, row_offset, row)
> description tends to be really long

This is a representation related concern and should be handled by either the
output or the formatter not the parser (separation of concern !!!)

> which may not be correct depending on the underlying filesystem that was
parsed using fls

Then we need to address this in the parser, either we have the user provide
information necessary or provide a big warning.

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:139: FILE_BASED_DATA_TYPES = frozenset([u'fs:stat',
u'fs:mactime:line'])
Yes or define it in lib.definitions and then use that in both the parser and
this file.

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:139: FILE_BASED_DATA_TYPES = frozenset([u'fs:stat',
u'fs:mactime:line'])
Make this protected?

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:257: if data_type == u'fs:mactime:line':
I opt to define this as constant as well

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:276: # In the case of a file comparison we need the inode
value to be included.
Elaborate why you need the inode, note that with the upcoming NTFS there will be
no inode value

https://codereview.appspot.com/263830043/diff/40001/plaso/output/interface.py
File plaso/output/interface.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/output/interface.py...
plaso/output/interface.py:222: if getattr(event_a, u'data_type', u'') in
event_a.FILE_BASED_DATA_TYPES:
Turn this into a method:

if event_a.IsFileBasedDataType():

https://codereview.appspot.com/263830043/diff/40001/plaso/output/mediator.py
File plaso/output/mediator.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/output/mediator.py#...
plaso/output/mediator.py:199: if data_type in
event_object.FILE_BASED_DATA_TYPES:
if event_object.IsFileBasedDataType():

[kiddi, 2015-08-28 20:09:05]

done

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:139: FILE_BASED_DATA_TYPES = frozenset([u'fs:stat',
u'fs:mactime:line'])
On 2015/08/28 10:27:01, Joachim Metz wrote:
> Yes or define it in lib.definitions and then use that in both the parser and
> this file.

Done.

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:257: if data_type == u'fs:mactime:line':
On 2015/08/28 10:27:01, Joachim Metz wrote:
> I opt to define this as constant as well

Done.

https://codereview.appspot.com/263830043/diff/40001/plaso/lib/event.py#newcod...
plaso/lib/event.py:276: # In the case of a file comparison we need the inode
value to be included.
On 2015/08/28 10:27:01, Joachim Metz wrote:
> Elaborate why you need the inode, note that with the upcoming NTFS there will
be
> no inode value

eloaborated in a comment

https://codereview.appspot.com/263830043/diff/40001/plaso/output/interface.py
File plaso/output/interface.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/output/interface.py...
plaso/output/interface.py:222: if getattr(event_a, u'data_type', u'') in
event_a.FILE_BASED_DATA_TYPES:
On 2015/08/28 10:27:01, Joachim Metz wrote:
> Turn this into a method:
> 
> if event_a.IsFileBasedDataType():

Done.

https://codereview.appspot.com/263830043/diff/40001/plaso/output/mediator.py
File plaso/output/mediator.py (right):

https://codereview.appspot.com/263830043/diff/40001/plaso/output/mediator.py#...
plaso/output/mediator.py:199: if data_type in
event_object.FILE_BASED_DATA_TYPES:
On 2015/08/28 10:27:01, Joachim Metz wrote:
> if event_object.IsFileBasedDataType():

Done.

[kiddi, 2015-08-28 20:11:28]

Code updated.

[Joachim Metz, 2015-08-30 07:53:48]

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/definitions.py
File plaso/lib/definitions.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/definitions.py#...
plaso/lib/definitions.py:4: FILE_DATA_TYPE_FS_STAT = u'fs:stat'
I opt to add a comment what these types are supposed to represent. 

Also maybe rename them to:
FILE_SYSTEM_EVENT_DATA_TYPES, FILE_DATA is IMO not very descriptive.

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/event.py#newcod...
plaso/lib/event.py:348: 
+ white line

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:118: value = row.get(description, None)
value => timestamp or time_value

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:122: value, description, row_offset, row)
As indicated in previous comments handle description correctly.

[kiddi, 2015-09-14 23:24:30]

done

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/definitions.py
File plaso/lib/definitions.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/definitions.py#...
plaso/lib/definitions.py:4: FILE_DATA_TYPE_FS_STAT = u'fs:stat'
On 2015/08/30 07:53:48, Joachim Metz wrote:
> I opt to add a comment what these types are supposed to represent. 
> 
> Also maybe rename them to:
> FILE_SYSTEM_EVENT_DATA_TYPES, FILE_DATA is IMO not very descriptive.

Done.

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/event.py
File plaso/lib/event.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/lib/event.py#newcod...
plaso/lib/event.py:348: 
On 2015/08/30 07:53:48, Joachim Metz wrote:
> + white line

Done.

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:118: value = row.get(description, None)
On 2015/08/30 07:53:48, Joachim Metz wrote:
> value => timestamp or time_value

Done.

https://codereview.appspot.com/263830043/diff/60001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:122: value, description, row_offset, row)
On 2015/08/30 07:53:48, Joachim Metz wrote:
> As indicated in previous comments handle description correctly.

Done.

[kiddi, 2015-09-14 23:33:44]

Code updated.

[Joachim Metz, 2015-09-15 05:41:40]

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:117: for timestamp_description in self._TIMESTAMP_DESC:
Still needs correct mapping to plaso descriptions.

[kiddi, 2015-09-15 22:43:49]

done

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:117: for timestamp_description in self._TIMESTAMP_DESC:
On 2015/09/15 05:41:40, Joachim Metz wrote:
> Still needs correct mapping to plaso descriptions.

you prefer usage?

[kiddi, 2015-09-15 22:45:36]

Code updated.

[Joachim Metz, 2015-09-16 05:43:59]

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/80001/plaso/parsers/mactime.py#...
plaso/parsers/mactime.py:117: for timestamp_description in self._TIMESTAMP_DESC:
No undo these last changes I'm referring to changes you made in patch set 1.

[Joachim Metz, 2015-09-16 05:54:21]

https://codereview.appspot.com/263830043/diff/100001/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/100001/plaso/parsers/mactime.py...
plaso/parsers/mactime.py:72: _TIMESTAMP_DESC = frozenset([
We should not be using mactime mappings in plaso.
So you'll need to map these to the correct plaso values.

https://codereview.appspot.com/263830043/diff/100001/plaso/parsers/mactime.py...
plaso/parsers/mactime.py:117: for usage in self._TIMESTAMP_DESC:
Undo the changes in patch set 6.

[Joachim Metz, 2015-09-16 05:58:19]

https://codereview.appspot.com/263830043/diff/100001/plaso/output/mediator.py
File plaso/output/mediator.py (right):

https://codereview.appspot.com/263830043/diff/100001/plaso/output/mediator.py...
plaso/output/mediator.py:199: if event_object.IsFileBasedDataType():
What if you have 8 timestamps or more from both $STANDARD_INFORMATION and
$FILE_NAME?

I opt to rethink this whole approach.

[Joachim Metz, 2015-09-29 06:42:49]

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py
File plaso/parsers/mactime.py (right):

https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py#newc...
plaso/parsers/mactime.py:121: value, key, row_offset, row)
Check this list:
http://wiki.sleuthkit.org/index.php?title=Mactime_output

We need to parse mactime output accordingly to this.

Not sure what your concerns are?

[onager, 2015-12-04 12:11:50]

On 2015/09/29 06:42:49, Joachim Metz wrote:
> https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py
> File plaso/parsers/mactime.py (right):
> 
>
https://codereview.appspot.com/263830043/diff/1/plaso/parsers/mactime.py#newc...
> plaso/parsers/mactime.py:121: value, key, row_offset, row)
> Check this list:
> http://wiki.sleuthkit.org/index.php?title=Mactime_output
> 
> We need to parse mactime output accordingly to this.
> 
> Not sure what your concerns are?

Ping for kiddi - please close this if it's no longer relevant