[eccemotus] Migrating files to public repo. Migrating parsers.

[eccemotus] Migrating files to public repo. Migrating parsers.

[vlejd, 2016-09-20 12:30:30]

Code updated.

[vlejd, 2016-09-20 14:06:20]

Code updated.

[vlejd, 2016-09-21 08:24:55]

Please review.

[onager, 2016-09-21 08:30:53]

On 2016/09/21 08:24:55, vlejd wrote:
> Please review.

Per dicussion yesterday, adamsh is going to look at this one.

[onager, 2016-09-26 05:46:41]

Initial comments, mostly style related.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py
File eccemotus/eccemotus.py (right):

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:9: file, but has no memory requirements.
Well, it still has memory requirements, just low requirements.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:15: Than you have to parse the data with
parsed_data_generator(data_generator).
Then

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:17: Last think you want to do is to call
get_graph_json(parsed_data_generator)
thing

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:26: # How should dhose imports look like to not break
run_tests.py ?
Remove comment

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:31: def file_data_generator(filename, verbose=False):
We have a different style for log2timeline method naming see
https://github.com/log2timeline/plaso/wiki/Style-guide.

This should be FileDataGenerator.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:34: Json_line means, that every event is a json on a
separate line.
a JSON what? And also, JSON, not Json or json

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:38: if not i % 100000 and verbose:
extract 100000 to a constant

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:47: client: Elasticsearch client.
Update docstrings to conform to the style guide:
https://github.com/log2timeline/plaso/wiki/Style-guide

Specifically, you need to add type information.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:55: Uses scan function, so the data is really processed
like a stream.
This sort of comment is better before the args section

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:57: from elasticsearch.helpers import scan  #TODO add try
except
Move to start of file
https://google.github.io/styleguide/pyguide.html#Imports_formatting

Also, don't import functions like this:
https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:77: # this is a bug at elasticsearch.py, because it can
not handle elastic errors
Where is this bug? Which elasticsearch.py?

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:87: except Exception as e: # pylint: disable=broad-except
Please fix

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:93: """ Returns elasticsearch client for given port and
host address."""
+args
+returns

Is port a string or an int? Either?

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:100: """Transform raw event generator to parsed events
generator."""
+args
+yields

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:110: """ Create graph from raw data."""
+args 
+yields

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:116: def get_graph_json(raw_generator, verbose=False):
I'm not sure about the API here - why don't we just call
LoadFromFile()/LoadFromElastic or something on a graph object, and then
GetAsJSON() or similar on the graph?

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:117: """ Returns json representation for graph created
based on raw_generator."""
+args
+returns

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py
File eccemotus/lib/grapher.py (right):

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:3: For Default use-case, call
create_graph(data).minimal_serialize().
default, not Default

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:9: from collections import defaultdict as dfd
-as dfd

This makes the code harder to read/reason about.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:15: class Graph(object):
Why is this file called grapher.py, and not graph.py?

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:16: """Very light-weighed implementation of property
graph.
light-weight

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:65: """Return node's id or create a new one if it does
not exist."""
+args
+returns

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:78: def add_edge(self,
Just break after ( and indent by 4

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:103: EDGE_HAS = "has"
Move these definitions to the top of the class.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:124: """ Return best most specific ssh_source."""
+args
+returns

Please clarify what "best most specific" means

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:135: """ Return best most specific ssh_target."""
+args
+returns

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:147: """Add edge with corresponding nodes to graph."""
+args
+returns

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:156: Encoding is based on simple rules. (source,
target, edge_type).
Fix whitespace

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:197: data can be any iterable (list), but preferably
generator, because of memory
Move to args section

[vlejd, 2016-09-26 12:44:58]

Code updated.

[vlejd, 2016-09-26 12:54:26]

Fixed comments.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py
File eccemotus/eccemotus.py (right):

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:9: file, but has no memory requirements.
On 2016/09/26 05:46:40, onager wrote:
> Well, it still has memory requirements, just low requirements.

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:15: Than you have to parse the data with
parsed_data_generator(data_generator).
On 2016/09/26 05:46:40, onager wrote:
> Then

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:17: Last think you want to do is to call
get_graph_json(parsed_data_generator)
On 2016/09/26 05:46:40, onager wrote:
> thing

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:26: # How should dhose imports look like to not break
run_tests.py ?
On 2016/09/26 05:46:41, onager wrote:
> Remove comment

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:31: def file_data_generator(filename, verbose=False):
On 2016/09/26 05:46:40, onager wrote:
> We have a different style for log2timeline method naming see
> https://github.com/log2timeline/plaso/wiki/Style-guide.
> 
> This should be FileDataGenerator. 

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:34: Json_line means, that every event is a json on a
separate line.
On 2016/09/26 05:46:40, onager wrote:
> a JSON what? And also, JSON, not Json or json

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:38: if not i % 100000 and verbose:
On 2016/09/26 05:46:40, onager wrote:
> extract 100000 to a constant

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:47: client: Elasticsearch client.
On 2016/09/26 05:46:40, onager wrote:
> Update docstrings to conform to the style guide:
> https://github.com/log2timeline/plaso/wiki/Style-guide
> 
> Specifically, you need to add type information.

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:55: Uses scan function, so the data is really processed
like a stream.
On 2016/09/26 05:46:40, onager wrote:
> This sort of comment is better before the args section

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:57: from elasticsearch.helpers import scan  #TODO add try
except
On 2016/09/26 05:46:41, onager wrote:
> Move to start of file
> https://google.github.io/styleguide/pyguide.html#Imports_formatting
> 
> Also, don't import functions like this:
> https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:77: # this is a bug at elasticsearch.py, because it can
not handle elastic errors
On 2016/09/26 05:46:41, onager wrote:
> Where is this bug? Which elasticsearch.py?

Problem is, that python-elasticsearch has an issue and can not interpret errors
from elasticsearch properly. 

But in the end I think it is better to let let the error appear.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:87: except Exception as e: # pylint: disable=broad-except
On 2016/09/26 05:46:40, onager wrote:
> Please fix

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:93: """ Returns elasticsearch client for given port and
host address."""
On 2016/09/26 05:46:40, onager wrote:
> +args
> +returns
> 
> Is port a string or an int? Either?

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:100: """Transform raw event generator to parsed events
generator."""
On 2016/09/26 05:46:40, onager wrote:
> +args
> +yields

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:110: """ Create graph from raw data."""
On 2016/09/26 05:46:40, onager wrote:
> +args 
> +yields

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:116: def get_graph_json(raw_generator, verbose=False):
On 2016/09/26 05:46:40, onager wrote:
> I'm not sure about the API here - why don't we just call
> LoadFromFile()/LoadFromElastic or something on a graph object, and then
> GetAsJSON() or similar on the graph?

Fair point. 
Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:117: """ Returns json representation for graph created
based on raw_generator."""
On 2016/09/26 05:46:40, onager wrote:
> +args
> +returns

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py
File eccemotus/lib/grapher.py (right):

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:3: For Default use-case, call
create_graph(data).minimal_serialize().
On 2016/09/26 05:46:41, onager wrote:
> default, not Default

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:9: from collections import defaultdict as dfd
On 2016/09/26 05:46:41, onager wrote:
> -as dfd
> 
> This makes the code harder to read/reason about.

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:15: class Graph(object):
On 2016/09/26 05:46:41, onager wrote:
> Why is this file called grapher.py, and not graph.py?

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:16: """Very light-weighed implementation of property
graph.
On 2016/09/26 05:46:41, onager wrote:
> light-weight

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:65: """Return node's id or create a new one if it does
not exist."""
On 2016/09/26 05:46:41, onager wrote:
> +args
> +returns

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:78: def add_edge(self,
On 2016/09/26 05:46:41, onager wrote:
> Just break after ( and indent by 4

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:103: EDGE_HAS = "has"
On 2016/09/26 05:46:41, onager wrote:
> Move these definitions to the top of the class.

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:124: """ Return best most specific ssh_source."""
On 2016/09/26 05:46:41, onager wrote:
> +args
> +returns
> 
> Please clarify what "best most specific" means

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:135: """ Return best most specific ssh_target."""
On 2016/09/26 05:46:41, onager wrote:
> +args
> +returns

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:147: """Add edge with corresponding nodes to graph."""
On 2016/09/26 05:46:41, onager wrote:
> +args
> +returns

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:156: Encoding is based on simple rules. (source,
target, edge_type).
On 2016/09/26 05:46:41, onager wrote:
> Fix whitespace

Done.

https://codereview.appspot.com/310260043/diff/40001/eccemotus/lib/grapher.py#...
eccemotus/lib/grapher.py:197: data can be any iterable (list), but preferably
generator, because of memory
On 2016/09/26 05:46:41, onager wrote:
> Move to args section

Done.

[vlejd, 2016-09-26 12:56:18]

Fixed comments.

[onager, 2016-09-26 19:41:42]

On 2016/09/26 12:56:18, vlejd wrote:
> Fixed comments.

A couple of overall comments:
* I recommend getting rid of all the print()s and using logging instead
* Have you considered importing Plaso, and using it to do the
serialization/deserialization of the events? It's going to simplify the API
quite a lot, and may make it more robust to changes in Plaso
* Please check the format and style of all class and method docstrings.

[onager, 2016-09-26 19:41:51]

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py
File eccemotus/eccemotus.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:41: Yields:
+1 blank line here

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:43: 
-1 blank line - also for other other functions in this file.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:45: input_file = open(filename)
Please add error handling and open mode.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:63: json representation of plaso event.
Please add type information

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:138: raw_generator (tierable[dict]): plaso events
iterable

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py
File eccemotus/lib/graph.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:19: This implementation is not meant to be a general
property graph.
Indent this back to level with the """s - -2 spaces

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:88: """Return node's id or create a new one if it does
not exist.
-one +node id

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:99: node = (node_type, node_value)
It looks like you actually want a Node class

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:115: edge_type (str):
Something missing here

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:121: None
No need for a return section here

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:142: """ Return most specific ssh_source.
-space

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:217: target are almost (prefix "source:"/"target:" must
be removed) types of new
I don't understand this comment - can you make it clearer?

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:230: #Simple rules for ownership and identity
+ space and end with a .

This seems to also apply to other comments.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:242: #Rules for access edge
+ space

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:272: print(
printing like this might be problematic for embedding libraries - using the
logging module seems more useful, especially when you consider this might be
used on a web server

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py
File eccemotus/lib/parsers.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:2: """Provides easy way for parsing ssh related
information from plaso logs.
do you mean "remote access" rather than "ssh"?

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:4: Use ParserManager.parse(log) and it will interesting
fields in context of
will what?

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:45: def get_type(information):
Please fix method names per style guide

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:49: information (string): canonical name of interesting
field.
It sounds like this method wants to be called "field name" or something similar.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:76: event (plaso.event (dict)): plaso event.
Is this a plaso event, or a dict? I think it's a dict containing a json
serialized plaso event...

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:81: Example: /tmp/work/dump.plaso --->
dump.plaso/work/tmp/
Move to above args

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:85: if isinstance(spec, basestring):
There must be a nicer way to do this - perhaps import dfvfs and deserialize the
pathspec using the proper library.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:143: Users (names and ids) are extended by first
reasonable machine
Move this to above args

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:156: print(
This looks like it should raise, not write to stderr

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:183: parsed.get(SOURCE_PLASO), "UNKNOWN"
Don't mix " and ' quotes for strings, especially not in the same file

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:235: class LinuxUtmpEventParser(object):
It looks like these should be separated out into their own files, given that
their standalone pieces of code.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:241: """Extracting only important fields."""
Please fix docstring

[vlejd, 2016-09-27 14:43:15]

Code updated.

[vlejd, 2016-09-27 15:01:22]

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py
File eccemotus/eccemotus.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:41: Yields:
On 2016/09/26 19:41:50, onager wrote:
> +1 blank line here

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:43: 
On 2016/09/26 19:41:50, onager wrote:
> -1 blank line - also for other other functions in this file.

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:45: input_file = open(filename)
On 2016/09/26 19:41:50, onager wrote:
> Please add error handling and open mode.

Not sure if only addinw with statenebt is enough.
Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:63: json representation of plaso event.
On 2016/09/26 19:41:50, onager wrote:
> Please add type information

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/eccemotus.py#ne...
eccemotus/eccemotus.py:138: raw_generator (tierable[dict]): plaso events
On 2016/09/26 19:41:50, onager wrote:
> iterable

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py
File eccemotus/lib/graph.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:19: This implementation is not meant to be a general
property graph.
On 2016/09/26 19:41:50, onager wrote:
> Indent this back to level with the """s - -2 spaces

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:88: """Return node's id or create a new one if it does
not exist.
On 2016/09/26 19:41:50, onager wrote:
> -one +node id

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:99: node = (node_type, node_value)
On 2016/09/26 19:41:50, onager wrote:
> It looks like you actually want a Node class

Ok. But I am not sure if it is more readable or nicer.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:115: edge_type (str):
On 2016/09/26 19:41:50, onager wrote:
> Something missing here

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:121: None
On 2016/09/26 19:41:50, onager wrote:
> No need for a return section here

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:142: """ Return most specific ssh_source.
On 2016/09/26 19:41:50, onager wrote:
> -space

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:217: target are almost (prefix "source:"/"target:" must
be removed) types of new
On 2016/09/26 19:41:51, onager wrote:
> I don't understand this comment - can you make it clearer?

I changed the RULES to be a namedtuple and changed the comment. Hope it is
clearer now.
Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:230: #Simple rules for ownership and identity
On 2016/09/26 19:41:50, onager wrote:
> + space and end with a .
> 
> This seems to also apply to other comments.

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:242: #Rules for access edge
On 2016/09/26 19:41:51, onager wrote:
> + space

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/graph.py#ne...
eccemotus/lib/graph.py:272: print(
On 2016/09/26 19:41:50, onager wrote:
> printing like this might be problematic for embedding libraries - using the
> logging module seems more useful, especially when you consider this might be
> used on a web server

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py
File eccemotus/lib/parsers.py (right):

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:2: """Provides easy way for parsing ssh related
information from plaso logs.
On 2016/09/26 19:41:51, onager wrote:
> do you mean "remote access" rather than "ssh"?

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:4: Use ParserManager.parse(log) and it will interesting
fields in context of
On 2016/09/26 19:41:51, onager wrote:
> will what?

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:45: def get_type(information):
On 2016/09/26 19:41:51, onager wrote:
> Please fix method names per style guide

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:49: information (string): canonical name of interesting
field.
On 2016/09/26 19:41:51, onager wrote:
> It sounds like this method wants to be called "field name" or something
similar.

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:76: event (plaso.event (dict)): plaso event.
On 2016/09/26 19:41:51, onager wrote:
> Is this a plaso event, or a dict? I think it's a dict containing a json
> serialized plaso event...

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:81: Example: /tmp/work/dump.plaso --->
dump.plaso/work/tmp/
On 2016/09/26 19:41:51, onager wrote:
> Move to above args

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:85: if isinstance(spec, basestring):
On 2016/09/26 19:41:51, onager wrote:
> There must be a nicer way to do this - perhaps import dfvfs and deserialize
the
> pathspec using the proper library.

PathSpec class does not have something as deserialize (I do not think here was a
need for that) and I am not sure where else should I search for it.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:143: Users (names and ids) are extended by first
reasonable machine
On 2016/09/26 19:41:51, onager wrote:
> Move this to above args

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:156: print(
On 2016/09/26 19:41:51, onager wrote:
> This looks like it should raise, not write to stderr

Actually I do not think the program should depend on data format. More precisely
it should not die in case of unexpected event. But I can log it as a warning.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:183: parsed.get(SOURCE_PLASO), "UNKNOWN"
On 2016/09/26 19:41:51, onager wrote:
> Don't mix " and ' quotes for strings, especially not in the same file

Done.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:235: class LinuxUtmpEventParser(object):
On 2016/09/26 19:41:51, onager wrote:
> It looks like these should be separated out into their own files, given that
> their standalone pieces of code.

This will take a while. I will do separate update for that.

https://codereview.appspot.com/310260043/diff/60001/eccemotus/lib/parsers.py#...
eccemotus/lib/parsers.py:241: """Extracting only important fields."""
On 2016/09/26 19:41:51, onager wrote:
> Please fix docstring

Done.

[vlejd, 2016-09-28 15:01:08]

Code updated.

[vlejd, 2016-09-28 15:29:22]

Code updated.

[onager, 2016-09-30 11:51:18]

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib.py
File eccemotus/eccemotus_lib.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:31: import lib.graph as G # pylint:
disable=relative-import
Please import with more specific name, or just with its regular name. You're
only call this once.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:64: dict: json representation of plaso event.
JSON

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:101: """creates elasticsearch client.
Captail C for "Creates"

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:111: raise ImportError((u'Please install
elasticsearch to use this '
Doesn't this fit on one line?

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_test.py
File eccemotus/eccemotus_test.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_test...
eccemotus/eccemotus_test.py:5: import eccemotus.eccemotus_lib as E
Don't import with this name

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py
File eccemotus/lib/graph.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:59: EDGE_ACCEESS = u'access'
EDGE_ACCESS

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:137: def GetSshSource(cls, event):
This is called "GetSshSource", but seems to work on all remote access events,
unless I'm missing something. Please rename it, and other similar methods.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:224: event (dict[str, str]): parsed event. Result of
utils.parse()
Please don't call this an event, as eccemotus is also dealing with Plaso events,
which this isn't. See comment in the parser interface about creating a new class
for this.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:239: ssh_source_type, ssh_source_value =
self.__class__.GetSshSource(event)
As above, these are remote access sources and values, not ssh ones.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/li...
File eccemotus/lib/parsers/linux_utmp.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/li...
eccemotus/lib/parsers/linux_utmp.py:10: """Parser for linux:utm:event
data_type."""
utmp

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
File eccemotus/lib/parsers/parser_interface.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
eccemotus/lib/parsers/parser_interface.py:2: """Parser for linux:utm:event
data_type."""
Please fix docstring

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
eccemotus/lib/parsers/parser_interface.py:39: dict[str, str]: information parsed
from event.
Please return an object here, rather than just a plain dict. It's harder to
reason about what's being returned, and what can be expected in the result.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/sy...
File eccemotus/lib/parsers/syslog_line.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/sy...
eccemotus/lib/parsers/syslog_line.py:18: # NOTE: I do not care for fail attempts
tight now.
Remove comment or replace with a todo

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
File eccemotus/lib/parsers/utils.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
eccemotus/lib/parsers/utils.py:16: TARGET_PLASO = u'target:plaso'
What's a target plaso? Is this the storage file name, or something similar?

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
eccemotus/lib/parsers/utils.py:78: spec = eval(spec)  # pylint:
disable=eval-used
Please explain why this is needed - it seems very strange

[vlejd, 2016-10-05 13:23:50]

Code updated.

[vlejd, 2016-10-05 13:45:26]

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib.py
File eccemotus/eccemotus_lib.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:31: import lib.graph as G # pylint:
disable=relative-import
On 2016/09/30 11:51:18, onager wrote:
> Please import with more specific name, or just with its regular name. You're
> only call this once.

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:64: dict: json representation of plaso event.
On 2016/09/30 11:51:18, onager wrote:
> JSON

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:101: """creates elasticsearch client.
On 2016/09/30 11:51:18, onager wrote:
> Captail C for "Creates"

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:111: raise ImportError((u'Please install
elasticsearch to use this '
On 2016/09/30 11:51:18, onager wrote:
> Doesn't this fit on one line?

No. :( . one off.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_test.py
File eccemotus/eccemotus_test.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/eccemotus_test...
eccemotus/eccemotus_test.py:5: import eccemotus.eccemotus_lib as E
On 2016/09/30 11:51:18, onager wrote:
> Don't import with this name

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py
File eccemotus/lib/graph.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:59: EDGE_ACCEESS = u'access'
On 2016/09/30 11:51:18, onager wrote:
> EDGE_ACCESS

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:137: def GetSshSource(cls, event):
On 2016/09/30 11:51:18, onager wrote:
> This is called "GetSshSource", but seems to work on all remote access events,
> unless I'm missing something. Please rename it, and other similar methods.

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:224: event (dict[str, str]): parsed event. Result of
utils.parse()
On 2016/09/30 11:51:18, onager wrote:
> Please don't call this an event, as eccemotus is also dealing with Plaso
events,
> which this isn't. See comment in the parser interface about creating a new
class
> for this.

Acknowledged.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/graph.py#n...
eccemotus/lib/graph.py:239: ssh_source_type, ssh_source_value =
self.__class__.GetSshSource(event)
On 2016/09/30 11:51:18, onager wrote:
> As above, these are remote access sources and values, not ssh ones.

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/li...
File eccemotus/lib/parsers/linux_utmp.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/li...
eccemotus/lib/parsers/linux_utmp.py:10: """Parser for linux:utm:event
data_type."""
On 2016/09/30 11:51:18, onager wrote:
> utmp

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
File eccemotus/lib/parsers/parser_interface.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
eccemotus/lib/parsers/parser_interface.py:2: """Parser for linux:utm:event
data_type."""
On 2016/09/30 11:51:18, onager wrote:
> Please fix docstring

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/pa...
eccemotus/lib/parsers/parser_interface.py:39: dict[str, str]: information parsed
from event.
On 2016/09/30 11:51:18, onager wrote:
> Please return an object here, rather than just a plain dict. It's harder to
> reason about what's being returned, and what can be expected in the result.

Acknowledged.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/sy...
File eccemotus/lib/parsers/syslog_line.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/sy...
eccemotus/lib/parsers/syslog_line.py:18: # NOTE: I do not care for fail attempts
tight now.
On 2016/09/30 11:51:18, onager wrote:
> Remove comment or replace with a todo

Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
File eccemotus/lib/parsers/utils.py (right):

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
eccemotus/lib/parsers/utils.py:16: TARGET_PLASO = u'target:plaso'
On 2016/09/30 11:51:18, onager wrote:
> What's a target plaso? Is this the storage file name, or something similar?

I clarified it in a comment.
Done.

https://codereview.appspot.com/310260043/diff/120001/eccemotus/lib/parsers/ut...
eccemotus/lib/parsers/utils.py:78: spec = eval(spec)  # pylint:
disable=eval-used
On 2016/09/30 11:51:18, onager wrote:
> Please explain why this is needed - it seems very strange

because event['patspec'] can be a nested dictionary. For some elastic returns it
as a string and I need to retrieve the dictionary structure.
I added it to the comment.

[onager, 2016-10-11 09:19:41]

Almost there!

One overall comment - please check that your method docstrings are all active,
so things like:

"Parses a message from a string" and not "Parsing messages from strings"
and
"Adds a thing to a list" and not "Add a thing to a list"

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data.py
File eccemotus/lib/event_data.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:27: can not be both True at the same time.
source and target should have default values of False then, not None

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:44: Return: containing:
+ white line

Also mention that this is a tuple

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:52: """Readable printable string representation of
datum.
I think you want this to be __str__, not __repr__, as feeding the result of this
function to eval() will return a a Tuple, not an eventdatum.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:62: def GetName(cls):
Does this need to be a method? It seems like a a class constant would serve the
same purpose, and be clearer

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:92: class Plaso(EventDatum):
This should be StorageFileName or something similar

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:141: """Initialize empty EventData.
Initializes

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:194: def Empty(self):
rename to IsEmpty()

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/bs...
File eccemotus/lib/parsers/bsm.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/bs...
eccemotus/lib/parsers/bsm.py:32: data.Add(event_data.Plaso(target=True,
value=utils.GetImageName(event)))
Please simplify this a bit, to aid comprehension and debugging. Something like:

storage_file_name = utils.GetImageName(event)
storage_datum = event_data.Plaso(value=storage_file_name, target=true)
data.Add(storage_datum)

This applies across most places where you're doing parsing like this.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/ma...
File eccemotus/lib/parsers/manager.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/ma...
eccemotus/lib/parsers/manager.py:63: event (dict): dict serialize plaso event.
serialized plaso event

[vlejd, 2016-10-11 13:22:27]

Code updated.

[vlejd, 2016-10-11 13:44:47]

Fixed comments and couple of other nits.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data.py
File eccemotus/lib/event_data.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:27: can not be both True at the same time.
On 2016/10/11 09:19:41, onager wrote:
> source and target should have default values of False then, not None

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:44: Return: containing:
On 2016/10/11 09:19:41, onager wrote:
> + white line
> 
> Also mention that this is a tuple

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:52: """Readable printable string representation of
datum.
On 2016/10/11 09:19:41, onager wrote:
> I think you want this to be __str__, not __repr__, as feeding the result of
this
> function to eval() will return a a Tuple, not an eventdatum.

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:62: def GetName(cls):
On 2016/10/11 09:19:41, onager wrote:
> Does this need to be a method? It seems like a a class constant would serve
the
> same purpose, and be clearer

I wanted to make it required to implement/set in subclasses.
Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:92: class Plaso(EventDatum):
On 2016/10/11 09:19:41, onager wrote:
> This should be StorageFileName or something similar

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:141: """Initialize empty EventData.
On 2016/10/11 09:19:41, onager wrote:
> Initializes

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/event_data...
eccemotus/lib/event_data.py:194: def Empty(self):
On 2016/10/11 09:19:41, onager wrote:
> rename to IsEmpty()

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/bs...
File eccemotus/lib/parsers/bsm.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/bs...
eccemotus/lib/parsers/bsm.py:32: data.Add(event_data.Plaso(target=True,
value=utils.GetImageName(event)))
On 2016/10/11 09:19:41, onager wrote:
> Please simplify this a bit, to aid comprehension and debugging. Something
like:
> 
> storage_file_name = utils.GetImageName(event)
> storage_datum = event_data.Plaso(value=storage_file_name, target=true)
> data.Add(storage_datum)
> 
> This applies across most places where you're doing parsing like this.

Done.

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/ma...
File eccemotus/lib/parsers/manager.py (right):

https://codereview.appspot.com/310260043/diff/140001/eccemotus/lib/parsers/ma...
eccemotus/lib/parsers/manager.py:63: event (dict): dict serialize plaso event.
On 2016/10/11 09:19:41, onager wrote:
> serialized plaso event

Done.

[onager, 2016-10-16 20:44:49]

OK, this LGTM to me, there's a little docstring cleanup I'd like to do, but I'll
handle that in a follow up CL.

https://codereview.appspot.com/310260043/diff/160001/eccemotus/eccemotus_lib.py
File eccemotus/eccemotus_lib.py (right):

https://codereview.appspot.com/310260043/diff/160001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:6: ElasticDataGenerator), depending of where you want
to get plaso events from.
depending on

[onager, 2016-10-16 20:51:46]

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 lib

[vlejd, 2016-10-17 17:07:24]

https://codereview.appspot.com/310260043/diff/160001/eccemotus/eccemotus_lib.py
File eccemotus/eccemotus_lib.py (right):

https://codereview.appspot.com/310260043/diff/160001/eccemotus/eccemotus_lib....
eccemotus/eccemotus_lib.py:6: ElasticDataGenerator), depending of where you want
to get plaso events from.
On 2016/10/16 20:44:49, onager wrote:
> depending on

Done.