[plaso] Modify Mac Wifi and Syslog to correctly handle wifi.log #1040

[plaso] Modify Mac Wifi and Syslog to correctly handle wifi.log #1040

[james.nettesheim, 2017-01-25 01:09:33]

On 2017/01/25 01:05:25, james.nettesheim wrote:

The syslog parser will still parse a one line header only "turned over" mac_wifi
wifi.log file.  However, it should not parse any other mac_wifi wifi.log files
as it currently does.
This also removes some of the custom log entry parsing that was implemented as
it no longer appears to be relevant and caused log messages to be parsed in a
difficult to understand format.

[Joachim Metz, 2017-01-25 06:42:23]

Just a heads up: https://codereview.appspot.com/320020043/ will likely affect
your CL

[onager, 2017-01-26 14:33:07]

https://codereview.appspot.com/319180043/diff/1/plaso/formatters/mac_wifi.py
File plaso/formatters/mac_wifi.py (left):

https://codereview.appspot.com/319180043/diff/1/plaso/formatters/mac_wifi.py#...
plaso/formatters/mac_wifi.py:14: u'Action: {action}',
Do we absolutely need to change the content of the event like this? It breaks
backward compatibility, and while we have a pretty relaxed approach to keeping
things compatible, I'd like to make sure we don't do so trivially.

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

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:28: timestamp: the timestamp, contains the number of
microseconds from
Please add type information to args.

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:30: body: The body of the log message.
Why are you killing off the extra fields here? If it's because of the "turned
over" line, just create a new event type with fewer attributes, ala the "syslog
comment" event.

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:160: if key == u'logline' or key == u'header' or key
== u'turned_over_header':
if key in ['u'logline', u'header', u'turned_over_header']

In fact, swap this around:
if key not in [....
  logging.debug(...
  return
self._ParseLogLine(...

https://codereview.appspot.com/319180043/diff/1/tests/formatters/mac_wifi.py
File tests/formatters/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/1/tests/formatters/mac_wifi.py#...
tests/formatters/mac_wifi.py:16: def setUp(self):
You can drop this - you seem to be creating the formatter in several tests, and
a one-line setUp method doesn't add much.

[james.nettesheim, 2017-01-27 02:18:04]

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

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:30: body: The body of the log message.
On 2017/01/26 14:33:07, onager wrote:
> Why are you killing off the extra fields here? If it's because of the "turned
> over" line, just create a new event type with fewer attributes, ala the
"syslog
> comment" event.

I killed them off as for me as an analyst they were causing more confusion than
helping.  I played around with different formats and thought removing them was
cleanest.  

It only parses three known actions and has left both agent and function in a
TODO state that isn't clearly defined.  This causes both the "Action" and "text"
fields to contain duplicate data in many instances, I've experimented with not
outputting one when it is duplicate, but that didn't seem ideal as I tested
further.  This is especially apparent as many of the log lines in newer files
(not just the headers) don't immediately seem to conform to the same structure.

That said I do understand the desire to maintain backward compatibility, so let
me take a look at creating a new event type for what look to be newly formatted
log lines and update once I've experimented more.

[james.nettesheim, 2017-01-29 03:50:00]

Code updated.

[james.nettesheim, 2017-01-29 03:53:48]

https://codereview.appspot.com/319180043/diff/1/plaso/formatters/mac_wifi.py
File plaso/formatters/mac_wifi.py (left):

https://codereview.appspot.com/319180043/diff/1/plaso/formatters/mac_wifi.py#...
plaso/formatters/mac_wifi.py:14: u'Action: {action}',
On 2017/01/26 14:33:07, onager wrote:
> Do we absolutely need to change the content of the event like this? It breaks
> backward compatibility, and while we have a pretty relaxed approach to keeping
> things compatible, I'd like to make sure we don't do so trivially.

I've tried another approach with this, let me know what you think.
Just to note I still need to modify this as parts were never working.  For
example, the line: u'Agent: {user}', never outputs anything as nothing is stored
as 'user' in the structure, so it's always blank.

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

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:28: timestamp: the timestamp, contains the number of
microseconds from
On 2017/01/26 14:33:07, onager wrote:
> Please add type information to args.

Reverted to old args and added type to all args throughout.

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:30: body: The body of the log message.
On 2017/01/27 02:18:04, james.nettesheim wrote:
> On 2017/01/26 14:33:07, onager wrote:
> > Why are you killing off the extra fields here? If it's because of the
"turned
> > over" line, just create a new event type with fewer attributes, ala the
> "syslog
> > comment" event.
> 
> I killed them off as for me as an analyst they were causing more confusion
than
> helping.  I played around with different formats and thought removing them was
> cleanest.  
> 
> It only parses three known actions and has left both agent and function in a
> TODO state that isn't clearly defined.  This causes both the "Action" and
"text"
> fields to contain duplicate data in many instances, I've experimented with not
> outputting one when it is duplicate, but that didn't seem ideal as I tested
> further.  This is especially apparent as many of the log lines in newer files
> (not just the headers) don't immediately seem to conform to the same
structure.
> 
> That said I do understand the desire to maintain backward compatibility, so
let
> me take a look at creating a new event type for what look to be newly
formatted
> log lines and update once I've experimented more.

Please have a look at the new format.  I've still modified the args for the
getAction function, but can revert that to if we'd prefer it the previous way.

https://codereview.appspot.com/319180043/diff/1/plaso/parsers/mac_wifi.py#new...
plaso/parsers/mac_wifi.py:160: if key == u'logline' or key == u'header' or key
== u'turned_over_header':
On 2017/01/26 14:33:07, onager wrote:
> if key in ['u'logline', u'header', u'turned_over_header']
> 
> In fact, swap this around:
> if key not in [....
>   logging.debug(...
>   return
> self._ParseLogLine(...

Done.

https://codereview.appspot.com/319180043/diff/1/tests/formatters/mac_wifi.py
File tests/formatters/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/1/tests/formatters/mac_wifi.py#...
tests/formatters/mac_wifi.py:16: def setUp(self):
On 2017/01/26 14:33:07, onager wrote:
> You can drop this - you seem to be creating the formatter in several tests,
and
> a one-line setUp method doesn't add much.

I deleted the duplicate creation statements, but happy to drop this altogether
and create them individually if you prefer.  Personally, I like it better with a
setUp.

[onager, 2017-01-31 10:49:46]

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py
File plaso/parsers/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:35: an AP, configured, etc. If the action is not
known,
Please fix the wrapping here

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:71: pyparsing.Combine(pyparsing.Literal(u'airportd') +
Please break after Combine(

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:98:
pyparsing.Combine(pyparsing.Word(pyparsing.printables) +
Please break after Combine(

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:172: time (list[int]): a list containing integers with
the number of
This is actually a list containing a list of ints, and an int. For example:
[[20, 36, 37], 222]

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:227: if key != 'known_function_logline':
action = u''
if key == u'known_function_logline':
  action = self._GetAction(...)

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:232: event_object = MacWifiLogEvent(
event =

https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py
File tests/parsers/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py...
tests/parsers/mac_wifi.py:35: self.assertEqual(event_object.text,
Break after Equal(

[james.nettesheim, 2017-02-01 22:20:09]

Code updated.

[james.nettesheim, 2017-02-01 22:24:43]

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py
File plaso/parsers/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:35: an AP, configured, etc. If the action is not
known,
On 2017/01/31 10:49:45, onager wrote:
> Please fix the wrapping here

Done.

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:71: pyparsing.Combine(pyparsing.Literal(u'airportd') +
On 2017/01/31 10:49:45, onager wrote:
> Please break after Combine(

Done.

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:98:
pyparsing.Combine(pyparsing.Word(pyparsing.printables) +
On 2017/01/31 10:49:45, onager wrote:
> Please break after Combine(

Done.

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:172: time (list[int]): a list containing integers with
the number of
On 2017/01/31 10:49:45, onager wrote:
> This is actually a list containing a list of ints, and an int. For example:
> [[20, 36, 37], 222]

I decided to modify this a bit as I as accepting different object types for
structure.time, this way it's a little stronger.  Happy to switch back and put
the two different possibilities in docstring if you prefer.

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:227: if key != 'known_function_logline':
On 2017/01/31 10:49:45, onager wrote:
> action = u''
> if key == u'known_function_logline':
>   action = self._GetAction(...)

Done.

https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
plaso/parsers/mac_wifi.py:232: event_object = MacWifiLogEvent(
On 2017/01/31 10:49:45, onager wrote:
> event = 

Done.

https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py
File tests/parsers/mac_wifi.py (right):

https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py...
tests/parsers/mac_wifi.py:35: self.assertEqual(event_object.text,
On 2017/01/31 10:49:45, onager wrote:
> Break after Equal(

Done.

[onager, 2017-02-03 14:08:58]

On 2017/02/01 22:24:43, james.nettesheim wrote:
> https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py
> File plaso/parsers/mac_wifi.py (right):
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:35: an AP, configured, etc. If the action is not
> known,
> On 2017/01/31 10:49:45, onager wrote:
> > Please fix the wrapping here
> 
> Done.
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:71: pyparsing.Combine(pyparsing.Literal(u'airportd')
+
> On 2017/01/31 10:49:45, onager wrote:
> > Please break after Combine(
> 
> Done.
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:98:
> pyparsing.Combine(pyparsing.Word(pyparsing.printables) +
> On 2017/01/31 10:49:45, onager wrote:
> > Please break after Combine(
> 
> Done.
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:172: time (list[int]): a list containing integers
with
> the number of
> On 2017/01/31 10:49:45, onager wrote:
> > This is actually a list containing a list of ints, and an int. For example:
> > [[20, 36, 37], 222]
> 
> I decided to modify this a bit as I as accepting different object types for
> structure.time, this way it's a little stronger.  Happy to switch back and put
> the two different possibilities in docstring if you prefer.
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:227: if key != 'known_function_logline':
> On 2017/01/31 10:49:45, onager wrote:
> > action = u''
> > if key == u'known_function_logline':
> >   action = self._GetAction(...)
> 
> Done.
> 
>
https://codereview.appspot.com/319180043/diff/20001/plaso/parsers/mac_wifi.py...
> plaso/parsers/mac_wifi.py:232: event_object = MacWifiLogEvent(
> On 2017/01/31 10:49:45, onager wrote:
> > event = 
> 
> Done.
> 
> https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py
> File tests/parsers/mac_wifi.py (right):
> 
>
https://codereview.appspot.com/319180043/diff/20001/tests/parsers/mac_wifi.py...
> tests/parsers/mac_wifi.py:35: self.assertEqual(event_object.text,
> On 2017/01/31 10:49:45, onager wrote:
> > Break after Equal(
> 
> Done.

OK, LGTM, I'll merge this.

[onager, 2017-02-03 14:13:55]

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 feature

[onager, 2017-02-14 14:37:10]

On 2017/02/03 14:13:55, onager wrote:
> 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 feature

Hey James, can you close this review out please?