[dftimewolf] First draft of timewolf code.

[dftimewolf] First draft of timewolf code.

[jberggren, 2016-10-05 14:24:20]

First round of comments, but it looks good!

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py
File dftimewolf/lib/collectors.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py#...
dftimewolf/lib/collectors.py:94: verbose=False):
4 space hanging indent?

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/processors.py
File dftimewolf/lib/processors.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/processors.py#...
dftimewolf/lib/processors.py:31: """Initialize the Plaso artifact processor
object."""
add class attributes to docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
File dftimewolf/lib/timesketch_utils.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:2: from bs4 import BeautifulSoup
Just out of curiosity, is bs4 better than bs-classic?

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:7: """API Client for Timesketch."""
Add class attributes to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:11: self.username = username
This is only used in the _CreateSession method. Skip adding it to the object
attribute space and just pass it as argument to the method?

E.g.:
self.session = self._CreateSession(username, password)

   def _CreateSession(self, username, password):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:33: """Create a new sketch with the specified
name and description."""
Please add args and returns to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:34: resource_url =
u'{0:s}/api/v1/sketches/'.format(self.host_url)
This is a bit repetitive, maybe we can add:

self.api_base_url = u'{0:s}/api/v1/'.format(host_url)

to the costructor and then use that?

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:42: """Create a timeline with the specified
name from the given plaso file."""
Please add args and returns to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:52: """Associate the specified timeline and
sketch."""
Please add args and returns to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:53: resource_url =
u'{0:s}/api/v1/sketches/{1:d}/'.format(self.host_url,
I think the style guide for Plaso says 4 spaces hanging indent, i.e just break
at the parentheses

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:59: """Get information on the specified
sketch."""
Please add args and returns to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:71: """Get the full URL of the specified
sketch."""
Please add args and returns to the docstring

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:77: if __name__ == '__main__':
No need for this. Remove as this is only intended to be imported by others.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py
File dftimewolf/lib/utils.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py#newco...
dftimewolf/lib/utils.py:20: """Format message with script name and sender
name."""
Add args and returns to docstring

also for the rest of the methods.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py#newco...
dftimewolf/lib/utils.py:23: self._sender, message)
4 space hanging indent instead?

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py#ne...
dftimewolf/timewolf_cli.py:96: FLAGS.timezone,
wrong indent

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.py
File dftimewolf/timewolf_collect.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.p...
dftimewolf/timewolf_collect.py:36: gflags.DEFINE_string(u'username', None, u'GRR
username')
Do we also want .netrc support? no rush though, we can do that later :)

[someguyiknow, 2016-10-05 19:27:37]

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py
File dftimewolf/lib/collectors.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py#...
dftimewolf/lib/collectors.py:94: verbose=False):
On 2016/10/05 14:24:19, jberggren wrote:
> 4 space hanging indent?

The formatter bumps it back this way, unless there's a different formatter I
should be using?

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/processors.py
File dftimewolf/lib/processors.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/processors.py#...
dftimewolf/lib/processors.py:31: """Initialize the Plaso artifact processor
object."""
On 2016/10/05 14:24:19, jberggren wrote:
> add class attributes to docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
File dftimewolf/lib/timesketch_utils.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:2: from bs4 import BeautifulSoup
On 2016/10/05 14:24:20, jberggren wrote:
> Just out of curiosity, is bs4 better than bs-classic?

Shrug. Isn't new always better? ;)

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:7: """API Client for Timesketch."""
On 2016/10/05 14:24:19, jberggren wrote:
> Add class attributes to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:11: self.username = username
On 2016/10/05 14:24:19, jberggren wrote:
> This is only used in the _CreateSession method. Skip adding it to the object
> attribute space and just pass it as argument to the method?
> 
> E.g.:
> self.session = self._CreateSession(username, password)
> 
>    def _CreateSession(self, username, password):

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:33: """Create a new sketch with the specified
name and description."""
On 2016/10/05 14:24:20, jberggren wrote:
> Please add args and returns to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:34: resource_url =
u'{0:s}/api/v1/sketches/'.format(self.host_url)
On 2016/10/05 14:24:19, jberggren wrote:
> This is a bit repetitive, maybe we can add:
> 
> self.api_base_url = u'{0:s}/api/v1/'.format(host_url)
> 
> to the costructor and then use that?

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:42: """Create a timeline with the specified
name from the given plaso file."""
On 2016/10/05 14:24:20, jberggren wrote:
> Please add args and returns to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:52: """Associate the specified timeline and
sketch."""
On 2016/10/05 14:24:20, jberggren wrote:
> Please add args and returns to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:53: resource_url =
u'{0:s}/api/v1/sketches/{1:d}/'.format(self.host_url,
On 2016/10/05 14:24:20, jberggren wrote:
> I think the style guide for Plaso says 4 spaces hanging indent, i.e just break
> at the parentheses 

I think the api_base_url resolved this.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:59: """Get information on the specified
sketch."""
On 2016/10/05 14:24:19, jberggren wrote:
> Please add args and returns to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:71: """Get the full URL of the specified
sketch."""
On 2016/10/05 14:24:19, jberggren wrote:
> Please add args and returns to the docstring

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/timesketch_uti...
dftimewolf/lib/timesketch_utils.py:77: if __name__ == '__main__':
On 2016/10/05 14:24:20, jberggren wrote:
> No need for this. Remove as this is only intended to be imported by others.

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py
File dftimewolf/lib/utils.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py#newco...
dftimewolf/lib/utils.py:20: """Format message with script name and sender
name."""
On 2016/10/05 14:24:20, jberggren wrote:
> Add args and returns to docstring
> 
> also for the rest of the methods.

Done.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/utils.py#newco...
dftimewolf/lib/utils.py:23: self._sender, message)
On 2016/10/05 14:24:20, jberggren wrote:
> 4 space hanging indent instead?

Same question about formatter.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py#ne...
dftimewolf/timewolf_cli.py:96: FLAGS.timezone,
On 2016/10/05 14:24:20, jberggren wrote:
> wrong indent

Same question about formatter.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.py
File dftimewolf/timewolf_collect.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.p...
dftimewolf/timewolf_collect.py:36: gflags.DEFINE_string(u'username', None, u'GRR
username')
On 2016/10/05 14:24:20, jberggren wrote:
> Do we also want .netrc support? no rush though, we can do that later :)

That would probably be super useful, but I'll add that after this gets checked
in.

[someguyiknow, 2016-10-05 19:40:53]

Code updated.

[someguyiknow, 2016-10-05 19:55:12]

Code updated.

[jberggren, 2016-10-06 13:01:56]

Just some minor things, otherwise LGTM

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py
File dftimewolf/lib/collectors.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/lib/collectors.py#...
dftimewolf/lib/collectors.py:94: verbose=False):
On 2016/10/05 19:27:36, someguyiknow wrote:
> On 2016/10/05 14:24:19, jberggren wrote:
> > 4 space hanging indent?
> 
> The formatter bumps it back this way, unless there's a different formatter I
> should be using?

Huh, ok cool. I have not used the formatter myself, but I guess if that is the
preferred style and Plaso use it I guess all is fine. Then you can ignore my
comments :)

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_cli.py#ne...
dftimewolf/timewolf_cli.py:96: FLAGS.timezone,
On 2016/10/05 19:27:37, someguyiknow wrote:
> On 2016/10/05 14:24:20, jberggren wrote:
> > wrong indent
> 
> Same question about formatter.

Acknowledged.

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.py
File dftimewolf/timewolf_collect.py (right):

https://codereview.appspot.com/308600043/diff/1/dftimewolf/timewolf_collect.p...
dftimewolf/timewolf_collect.py:36: gflags.DEFINE_string(u'username', None, u'GRR
username')
On 2016/10/05 19:27:37, someguyiknow wrote:
> On 2016/10/05 14:24:20, jberggren wrote:
> > Do we also want .netrc support? no rush though, we can do that later :)
> 
> That would probably be super useful, but I'll add that after this gets checked
> in.

Acknowledged. SGTM

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_cli.p...
dftimewolf/timewolf_cli.py:61: print e
You can use the console_out.StdErr() functions from utils. E.g:

console_out.StdErr(e, die=True)

This will send error to StdErr and exit for you.

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_colle...
File dftimewolf/timewolf_collect.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_colle...
dftimewolf/timewolf_collect.py:48: print e
Use: console_out.StdErr(e, die=True)

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_expor...
File dftimewolf/timewolf_export.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_expor...
dftimewolf/timewolf_export.py:50: print e
Use: console_out.StdErr(e, die=True)

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_proce...
File dftimewolf/timewolf_process.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_proce...
dftimewolf/timewolf_process.py:40: print e
Use: console_out.StdErr(self, e, die=True)

[someguyiknow, 2016-10-07 03:59:11]

Code updated.

[someguyiknow, 2016-10-07 03:59:21]

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_cli.p...
dftimewolf/timewolf_cli.py:61: print e
On 2016/10/06 13:01:56, jberggren wrote:
> You can use the console_out.StdErr() functions from utils. E.g:
> 
> console_out.StdErr(e, die=True)
> 
> This will send error to StdErr and exit for you.

Done.

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_colle...
File dftimewolf/timewolf_collect.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_colle...
dftimewolf/timewolf_collect.py:48: print e
On 2016/10/06 13:01:56, jberggren wrote:
> Use: console_out.StdErr(e, die=True)

Done.

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_expor...
File dftimewolf/timewolf_export.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_expor...
dftimewolf/timewolf_export.py:50: print e
On 2016/10/06 13:01:56, jberggren wrote:
> Use: console_out.StdErr(e, die=True)

Done.

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_proce...
File dftimewolf/timewolf_process.py (right):

https://codereview.appspot.com/308600043/diff/40001/dftimewolf/timewolf_proce...
dftimewolf/timewolf_process.py:40: print e
On 2016/10/06 13:01:56, jberggren wrote:
> Use: console_out.StdErr(self, e, die=True)

Done.

[someguyiknow, 2016-10-11 23:33:38]

Code updated.

[someguyiknow, 2016-10-11 23:38:23]

Code updated.

[someguyiknow, 2016-10-12 01:47:11]

Code updated.

[someguyiknow, 2016-10-12 02:33:20]

Code updated.

[someguyiknow, 2016-10-12 03:04:25]

Code updated.

[someguyiknow, 2016-10-12 13:23:37]

Code updated.

[someguyiknow, 2016-10-13 21:39:11]

Code updated.

[jberggren, 2016-10-18 05:34:13]

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:16: In the case of collecting artifacts from a host
via GRR you will need approval
Are approvals activated by default in a vanilla deployment of GRR?

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:38: gflags.DEFINE_string(u'grr_server_url', None,
u'GRR server to use')
We should maybe default to http://localhost:8000 here?

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:39: gflags.DEFINE_string(u'timesketch_server_url',
None,
Maybe default to We should maybe default to http://localhost:5000 here?

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:56: gflags.MarkFlagAsRequired('reason')
Should this be required?

[jberggren, 2016-10-18 05:36:55]

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:68: 
We should check for FLAGS.hosts and FLAGS.paths here, and if both are empty we
should exit. Otherwise we get an empty sketch created.

[someguyiknow, 2016-10-18 19:30:18]

Code updated.

[someguyiknow, 2016-10-18 19:32:33]

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:16: In the case of collecting artifacts from a host
via GRR you will need approval
On 2016/10/18 05:34:13, jberggren wrote:
> Are approvals activated by default in a vanilla deployment of GRR?

Done.

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:38: gflags.DEFINE_string(u'grr_server_url', None,
u'GRR server to use')
On 2016/10/18 05:34:13, jberggren wrote:
> We should maybe default to http://localhost:8000 here?

Done.

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:39: gflags.DEFINE_string(u'timesketch_server_url',
None,
On 2016/10/18 05:34:13, jberggren wrote:
> Maybe default to We should maybe default to http://localhost:5000 here?

Done.

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:56: gflags.MarkFlagAsRequired('reason')
On 2016/10/18 05:34:13, jberggren wrote:
> Should this be required?

Done.

https://codereview.appspot.com/308600043/diff/200001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:68: 
On 2016/10/18 05:36:55, jberggren wrote:
> We should check for FLAGS.hosts and FLAGS.paths here, and if both are empty we
> should exit. Otherwise we get an empty sketch created.

Added the checks to cli and collect, but also changed the logic so that if stuff
returns no artifacts, no creation happens.

[jberggren, 2016-10-18 19:47:14]

https://codereview.appspot.com/308600043/diff/220001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/220001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:71: netrc_entry = netrc_file.authenticators(ts_host)
the netrc stuff is repeated below. Maybe turn this into a function with the host
as parameter:

def GetUsernamePassword(host):
 ...
 return username, password

Feel free to do this in a followup CL though!

[someguyiknow, 2016-10-18 21:32:22]

Code updated.

[someguyiknow, 2016-10-18 21:33:19]

https://codereview.appspot.com/308600043/diff/220001/dftimewolf/timewolf_cli.py
File dftimewolf/timewolf_cli.py (right):

https://codereview.appspot.com/308600043/diff/220001/dftimewolf/timewolf_cli....
dftimewolf/timewolf_cli.py:71: netrc_entry = netrc_file.authenticators(ts_host)
On 2016/10/18 19:47:14, jberggren wrote:
> the netrc stuff is repeated below. Maybe turn this into a function with the
host
> as parameter:
> 
> def GetUsernamePassword(host):
>  ...
>  return username, password
> 
> Feel free to do this in a followup CL though!

Done.