Code review - Issue 335560043: [plaso] Add parser and formatter for Trend Micro antivirus.https://codereview.appspot.com/2018-03-23T07:05:00+00:00rietveld
Message from unknown
2018-02-09T14:04:39+00:00epurn:md5:63a7d301b04c91c8554f014d14ce45d6
Message from puccia@gmail.com
2018-02-09T14:04:44+00:00epurn:md5:c6b95391c69e3ef9261ea2c84f834b51
Message from puccia@gmail.com
2018-02-09T16:54:35+00:00epurn:md5:53991f9b2c93c48613f3c587102e1773
There are several mistakes that I would like to correct here. Please disregard until the next patch set is pushed. Thanks, and sorry for the noise!
Message from unknown
2018-02-12T09:25:10+00:00epurn:md5:76ec3e53e6c4ae312264bf55ebec17c9
Message from puccia@gmail.com
2018-02-12T09:25:14+00:00epurn:md5:85fbc722aaed1c7fe34d08bda1a36166
Code updated.
Message from onager@deerpie.com
2018-02-14T16:10:34+00:00onagerurn:md5:f31d1487d212cede067ca5b90960d4f4
https://codereview.appspot.com/335560043/diff/20001/plaso/formatters/trendmicroav.py
File plaso/formatters/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/formatters/trendmicroav.py#newcode20
plaso/formatters/trendmicroav.py:20: '-> {action}',
Using "->" is a little weird, please use a : instead, for better consistency.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/dsv_parser.py
File plaso/parsers/dsv_parser.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/dsv_parser.py#newcode86
plaso/parsers/dsv_parser.py:86: def _CreateDictReader(self, parser_mediator, line_reader):
Please rebase this on the current master
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py
File plaso/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode20
plaso/parsers/trendmicroav.py:20: _SCAN_RESULTS = {
Please move these to the formatter.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode27
plaso/parsers/trendmicroav.py:27: 6: "Falure (move)",
Failure
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode40
plaso/parsers/trendmicroav.py:40: _SCAN_TYPES = {
Also in the formatter.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode57
plaso/parsers/trendmicroav.py:57: trigger_location (str): trigger location.
Trigger location and username aren't set in init - please remove them from the attributes, or set them in init.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode76
plaso/parsers/trendmicroav.py:76: DELIMITER = '<;>'
+blank line after docstring
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode79
plaso/parsers/trendmicroav.py:79: MIN_COLUMNS = None
This doesn't work, the DSVParser checks the numbers of columns match https://github.com/log2timeline/plaso/blob/d64d622e91a892296989645de871de16798837e1/plaso/parsers/dsv_parser.py#L148
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode82
plaso/parsers/trendmicroav.py:82: """Iterate over the log lines and provide a reader for the values.
Please re-write this docstring:
1) Add a Yields: section
2) Move the format description to the class docstring
3) The single line docstring should be active "Iterates..."
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode95
plaso/parsers/trendmicroav.py:95: values = line.decode(self._encoding).strip().split(self.DELIMITER)
Break this down into multiple lines to make debugging easier:
eg.
try:
line = line.decode(...)
except Unicode...
lines = line.strip(...)
values = lines.split(..)
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode96
plaso/parsers/trendmicroav.py:96: except UnicodeDecodeError, exception:
UnicodeDecodeError as exception
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode132
plaso/parsers/trendmicroav.py:132: else:
Remove the else, and dedent this block.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode151
plaso/parsers/trendmicroav.py:151: The date value is an 8-character string in the YYYYMMDD format.
Move these descriptions to the argument docstrings.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode157
plaso/parsers/trendmicroav.py:157: timestamp (str): Unix time in seconds since the epoch.
timestamps isn't an argument to this function
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode178
plaso/parsers/trendmicroav.py:178: return timelib.Timestamp.FromTimeParts(
Use dfdatetime instead, this function is on the way out: https://codereview.appspot.com/332630043/
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode193
plaso/parsers/trendmicroav.py:193: kwargs['encoding'] = 'cp1252'
+docstring
https://codereview.appspot.com/335560043/diff/20001/tests/parsers/trendmicroav.py
File tests/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/tests/parsers/trendmicroav.py#newcode34
tests/parsers/trendmicroav.py:34: expected_timestamp = timelib.Timestamp.CopyFromString(
Please use the newer checktimestamp method.
Message from unknown
2018-03-05T16:13:09+00:00epurn:md5:9c8752be76ba540d6b2c85f98a40f994
Message from puccia@gmail.com
2018-03-05T16:13:11+00:00epurn:md5:6df14adae69899c0716f2cd96d309a8b
Code updated.
Message from puccia@gmail.com
2018-03-05T16:14:01+00:00epurn:md5:7ce2e78a0936a49351d727194fdd781d
https://codereview.appspot.com/335560043/diff/20001/plaso/formatters/trendmicroav.py
File plaso/formatters/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/formatters/trendmicroav.py#newcode20
plaso/formatters/trendmicroav.py:20: '-> {action}',
On 2018/02/14 16:10:33, onager wrote:
> Using "->" is a little weird, please use a : instead, for better consistency.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/dsv_parser.py
File plaso/parsers/dsv_parser.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/dsv_parser.py#newcode86
plaso/parsers/dsv_parser.py:86: def _CreateDictReader(self, parser_mediator, line_reader):
On 2018/02/14 16:10:33, onager wrote:
> Please rebase this on the current master
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py
File plaso/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode20
plaso/parsers/trendmicroav.py:20: _SCAN_RESULTS = {
On 2018/02/14 16:10:33, onager wrote:
> Please move these to the formatter.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode27
plaso/parsers/trendmicroav.py:27: 6: "Falure (move)",
On 2018/02/14 16:10:33, onager wrote:
> Failure
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode40
plaso/parsers/trendmicroav.py:40: _SCAN_TYPES = {
On 2018/02/14 16:10:33, onager wrote:
> Also in the formatter.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode57
plaso/parsers/trendmicroav.py:57: trigger_location (str): trigger location.
On 2018/02/14 16:10:34, onager wrote:
> Trigger location and username aren't set in init - please remove them from the
> attributes, or set them in init.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode76
plaso/parsers/trendmicroav.py:76: DELIMITER = '<;>'
On 2018/02/14 16:10:34, onager wrote:
> +blank line after docstring
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode79
plaso/parsers/trendmicroav.py:79: MIN_COLUMNS = None
On 2018/02/14 16:10:33, onager wrote:
> This doesn't work, the DSVParser checks the numbers of columns match
> https://github.com/log2timeline/plaso/blob/d64d622e91a892296989645de871de16798837e1/plaso/parsers/dsv_parser.py#L148
>
I believe this is OK; if there are less columns than those in COLUMNS, the zip() in _CreateDictReader() will fill them with None.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode82
plaso/parsers/trendmicroav.py:82: """Iterate over the log lines and provide a reader for the values.
On 2018/02/14 16:10:34, onager wrote:
> Please re-write this docstring:
>
> 1) Add a Yields: section
> 2) Move the format description to the class docstring
> 3) The single line docstring should be active "Iterates..."
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode95
plaso/parsers/trendmicroav.py:95: values = line.decode(self._encoding).strip().split(self.DELIMITER)
On 2018/02/14 16:10:33, onager wrote:
> Break this down into multiple lines to make debugging easier:
>
> eg.
>
> try:
> line = line.decode(...)
> except Unicode...
>
> lines = line.strip(...)
> values = lines.split(..)
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode96
plaso/parsers/trendmicroav.py:96: except UnicodeDecodeError, exception:
On 2018/02/14 16:10:33, onager wrote:
> UnicodeDecodeError as exception
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode132
plaso/parsers/trendmicroav.py:132: else:
On 2018/02/14 16:10:33, onager wrote:
> Remove the else, and dedent this block.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode151
plaso/parsers/trendmicroav.py:151: The date value is an 8-character string in the YYYYMMDD format.
On 2018/02/14 16:10:33, onager wrote:
> Move these descriptions to the argument docstrings.
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode157
plaso/parsers/trendmicroav.py:157: timestamp (str): Unix time in seconds since the epoch.
On 2018/02/14 16:10:33, onager wrote:
> timestamps isn't an argument to this function
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode178
plaso/parsers/trendmicroav.py:178: return timelib.Timestamp.FromTimeParts(
On 2018/02/14 16:10:33, onager wrote:
> Use dfdatetime instead, this function is on the way out:
> https://codereview.appspot.com/332630043/
Done.
https://codereview.appspot.com/335560043/diff/20001/plaso/parsers/trendmicroav.py#newcode193
plaso/parsers/trendmicroav.py:193: kwargs['encoding'] = 'cp1252'
On 2018/02/14 16:10:33, onager wrote:
> +docstring
Done.
https://codereview.appspot.com/335560043/diff/20001/tests/parsers/trendmicroav.py
File tests/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/20001/tests/parsers/trendmicroav.py#newcode34
tests/parsers/trendmicroav.py:34: expected_timestamp = timelib.Timestamp.CopyFromString(
On 2018/02/14 16:10:34, onager wrote:
> Please use the newer checktimestamp method.
Done.
Message from onager@deerpie.com
2018-03-08T10:59:12+00:00onagerurn:md5:e90924145ade350178a8a3218588a771
Just a few nits left.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py
File plaso/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode20
plaso/parsers/trendmicroav.py:20: from plaso.formatters import trendmicroav as formatter
Don't import the formatter here, let it expand the values read from the file.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode132
plaso/parsers/trendmicroav.py:132: timestamp (str): Unix time in seconds since the epoch.
Remove the timestamp and timezone docstrings - these args no longer exist
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode139
plaso/parsers/trendmicroav.py:139: int: a timestamp integer containing the number of micro seconds since
This now returns a TimeElements, not an int.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode143
plaso/parsers/trendmicroav.py:143: TimestampError: if the timestamp is badly formed or unable to transfer
There's no timestamp to be badly formed any more.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode148
plaso/parsers/trendmicroav.py:148: return None
The docstring seems to suggest this should raise - please harmonize the docstring and the behavior here.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode207
plaso/parsers/trendmicroav.py:207: event_data.action = formatter.SCAN_RESULTS[int(row['action'])]
Don't do this - store the action as an int in the event, then expand it in the formatter, when the event is being exported.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode242
plaso/parsers/trendmicroav.py:242: if action not in formatter.SCAN_RESULTS:
OK, I can see why you might want to import the formatter here. This is better than duplicated the valid keys I think.
Message from unknown
2018-03-08T13:39:27+00:00epurn:md5:f1bc5244de35783b9e77d74f08b189f2
Message from puccia@gmail.com
2018-03-08T13:39:28+00:00epurn:md5:a03e0a2bd8350a92257c063b91977ae0
Code updated.
Message from puccia@gmail.com
2018-03-08T13:40:13+00:00epurn:md5:958a3dfeaabb219070845a6b6990e836
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py
File plaso/parsers/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode20
plaso/parsers/trendmicroav.py:20: from plaso.formatters import trendmicroav as formatter
On 2018/03/08 10:59:12, onager wrote:
> Don't import the formatter here, let it expand the values read from the file.
(keeping it as per your note later on in the review.)
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode132
plaso/parsers/trendmicroav.py:132: timestamp (str): Unix time in seconds since the epoch.
On 2018/03/08 10:59:11, onager wrote:
> Remove the timestamp and timezone docstrings - these args no longer exist
Done.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode139
plaso/parsers/trendmicroav.py:139: int: a timestamp integer containing the number of micro seconds since
On 2018/03/08 10:59:12, onager wrote:
> This now returns a TimeElements, not an int.
Done.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode143
plaso/parsers/trendmicroav.py:143: TimestampError: if the timestamp is badly formed or unable to transfer
On 2018/03/08 10:59:11, onager wrote:
> There's no timestamp to be badly formed any more.
Done (and replaced with ValueError, which can crop up during the str->int conversions or in dfdatetime. If it is raised, it's caught by the calling function.)
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode148
plaso/parsers/trendmicroav.py:148: return None
On 2018/03/08 10:59:12, onager wrote:
> The docstring seems to suggest this should raise - please harmonize the
> docstring and the behavior here.
Done.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode207
plaso/parsers/trendmicroav.py:207: event_data.action = formatter.SCAN_RESULTS[int(row['action'])]
On 2018/03/08 10:59:11, onager wrote:
> Don't do this - store the action as an int in the event, then expand it in the
> formatter, when the event is being exported.
Done.
https://codereview.appspot.com/335560043/diff/40001/plaso/parsers/trendmicroav.py#newcode242
plaso/parsers/trendmicroav.py:242: if action not in formatter.SCAN_RESULTS:
On 2018/03/08 10:59:11, onager wrote:
> OK, I can see why you might want to import the formatter here. This is better
> than duplicated the valid keys I think.
Acknowledged.
Message from onager@deerpie.com
2018-03-08T14:46:21+00:00onagerurn:md5:244936feb854ce325741f36e6fdfc26e
https://codereview.appspot.com/335560043/diff/60001/plaso/formatters/trendmicroav.py
File plaso/formatters/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/60001/plaso/formatters/trendmicroav.py#newcode69
plaso/formatters/trendmicroav.py:69: def _ConditionalFormatMessages(self, event_values):
Rather than overriding _ConditionalFormatMessages, move this to GetMessages instead, like the other formatters do, just so this is a little more consistent.
Can you also open an issue to create a formatter superclass for expanding values, so we can generalize this as discussed.
Message from unknown
2018-03-08T15:01:18+00:00epurn:md5:7abcbaf61171ba30a549812d69fcb164
Message from puccia@gmail.com
2018-03-08T15:01:20+00:00epurn:md5:f7a77c49451a1d37f1a6bf2b3c6d2cf9
Code updated.
Message from puccia@gmail.com
2018-03-08T15:01:53+00:00epurn:md5:041d837e60c465d501248f37bbe982c5
https://codereview.appspot.com/335560043/diff/60001/plaso/formatters/trendmicroav.py
File plaso/formatters/trendmicroav.py (right):
https://codereview.appspot.com/335560043/diff/60001/plaso/formatters/trendmicroav.py#newcode69
plaso/formatters/trendmicroav.py:69: def _ConditionalFormatMessages(self, event_values):
On 2018/03/08 14:46:21, onager wrote:
> Rather than overriding _ConditionalFormatMessages, move this to GetMessages
> instead, like the other formatters do, just so this is a little more consistent.
>
>
> Can you also open an issue to create a formatter superclass for expanding
> values, so we can generalize this as discussed.
Done.
Message from puccia@gmail.com
2018-03-08T15:02:17+00:00epurn:md5:34d863f2647737c4450ddc4dbcddc236
On 2018/03/08 15:01:53, ep wrote:
> > Can you also open an issue to create a formatter superclass for expanding
> > values, so we can generalize this as discussed.
>
> Done.
(Issue #1744, for the record.)
Message from onager@deerpie.com
2018-03-08T15:05:35+00:00onagerurn:md5:342be9fa9d1fd013df55c8307e17692c
LGTM, I'll merge this
Message from onager@deerpie.com
2018-03-08T15:12:02+00:00onagerurn:md5:820b851f412ea325ba59b7e3be7116b7
Changes have been merged with master branch. To close the review and clean up the feature branch you can run: review.py close trendmicro
Message from joachim.metz@gmail.com
2018-03-23T07:05:00+00:00Joachim Metzurn:md5:59098e6c2f43d22c9324da5db88558b2
this was merged please close