[Findit] Initiate Findit for crash.

[Findit] Initiate Findit for crash.

This is to initiate Findit for crash with two features:

1. Added a http client to support HTTPS request.
We don't want to add dependency to other library for handling https certificate verification: setup on GCE in clusterfuzz will be difficult.

2. Add a parser for DEPS file (https://chromium.googlesource.com/chromium/src/+/master/DEPS) in chromium to make Findit to support all components/dependencies (Blink, V8, WebRTC, etc).

R=mbarbella@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284775

[stgao, 2014-07-16 01:06:45]

Hi Abhishek and Martin,

This CL is to initiate the directory for Findit with a unittest skeleton.

Would you mind a review?

Thanks,
Shuotao Gao

[Martin Barbella, 2014-07-16 20:50:49]

not lgtm

I still haven't done a full pass over everything, but it might be a good idea to
see if there is other code that has already been written where this is handled
in a better way.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:11: DEPS_FILE_URL =
'http://src.chromium.org/chrome/trunk/src/DEPS?p=%s'
Definitely don't download this over HTTP.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:36: exec(content, global_scope, local_scope)
This really shouldn't be done with exec if possible. As it stands, this is
currently pulling something from HTTP and executing it.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:75: return urllib2.urlopen(DEPS_FILE_URL %
chromium_revision).read()
What other sources would this come from? We need to be really careful about this
if the deps file will be parsed with exec.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/utils.py
File tools/findit/utils.py (right):

https://codereview.chromium.org/391173002/diff/20001/tools/findit/utils.py#ne...
tools/findit/utils.py:21: return len(revision) < 8 and revision.isdigit()
This seems hacky.

[stgao, 2014-07-22 00:35:46]

Hi Martin,

I've added the support of server certificate verification with a list of trusted
root certificates.
Would you mind another review?

Thanks,
Shuotao Gao

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:11: DEPS_FILE_URL =
'http://src.chromium.org/chrome/trunk/src/DEPS?p=%s'
On 2014/07/16 20:50:49, mbarbella wrote:
> Definitely don't download this over HTTP.
Done.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:36: exec(content, global_scope, local_scope)
On 2014/07/16 20:50:49, mbarbella wrote:
> This really shouldn't be done with exec if possible. As it stands, this is
> currently pulling something from HTTP and executing it.
This is how chromium parses the DEPS file. As discussed offline, still keep the
current implementation, request DEPS through https connection, but verify the
certificate from the server.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:75: return urllib2.urlopen(DEPS_FILE_URL %
chromium_revision).read()
On 2014/07/16 20:50:49, mbarbella wrote:
> What other sources would this come from? We need to be really careful about
this
> if the deps file will be parsed with exec.
Good catch.

https://codereview.chromium.org/391173002/diff/20001/tools/findit/utils.py
File tools/findit/utils.py (right):

https://codereview.chromium.org/391173002/diff/20001/tools/findit/utils.py#ne...
tools/findit/utils.py:21: return len(revision) < 8 and revision.isdigit()
On 2014/07/16 20:50:49, mbarbella wrote:
> This seems hacky.
Yes, it is a little hacky. But I can't think out a better solution.
Do you have any suggestion?

https://codereview.chromium.org/391173002/diff/60001/tools/findit/cacert.pem
File tools/findit/cacert.pem (right):

https://codereview.chromium.org/391173002/diff/60001/tools/findit/cacert.pem#...
tools/findit/cacert.pem:2: ## ca-bundle.crt -- Bundle of CA Root Certificates
This is a copy from another cacert.pem in the chromium code base.
If there is a better location we could get a copy from, I'm happy to make a new
copy.

[Martin Barbella, 2014-07-22 06:35:43]

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:80: deps_file_downloader=_GetContentOfDEPS):
Please leave clear comments wherever deps_file_downloader can be specified that
it is assumed to be trusted input and will be evaluated as python code.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:91: deps, deps_os, _, _, _ =
_ParseDEPS(deps_content)
Since this is already a slightly modified copy, might as well change the return
values to only have what you need. It doesn't look like there are any other
calls to this.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:104: repository =
'http://src.chromium.org/chrome%s' % repository
Please use https where possible. Regardless of what it's being used for here,
there shouldn't be a reason to use http.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:124: 'repository':
'http://src.chromium.org/chrome/trunk',
Same as above.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py
File tools/findit/utils.py (right):

https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py#ne...
tools/findit/utils.py:21: return len(revision) < 8 and revision.isdigit()
This seems like a hack. It would probably be cleaner to determine that something
isn't a git hash based on the length.

[stgao, 2014-07-22 20:30:35]

Hi Martin,

I've addressed your comments.
Would you mind another review?

Thanks,
Shuotao Gao

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:80: deps_file_downloader=_GetContentOfDEPS):
On 2014/07/22 06:35:43, mbarbella wrote:
> Please leave clear comments wherever deps_file_downloader can be specified
that
> it is assumed to be trusted input and will be evaluated as python code.

Good suggestion.
Done.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:91: deps, deps_os, _, _, _ =
_ParseDEPS(deps_content)
On 2014/07/22 06:35:43, mbarbella wrote:
> Since this is already a slightly modified copy, might as well change the
return
> values to only have what you need. It doesn't look like there are any other
> calls to this.

SG. Done.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:104: repository =
'http://src.chromium.org/chrome%s' % repository
On 2014/07/22 06:35:43, mbarbella wrote:
> Please use https where possible. Regardless of what it's being used for here,
> there shouldn't be a reason to use http.

Done.
However, later we might use whatever we have in the DEPS file.
There are repo with http url instead of urls, like googlecode in
https://src.chromium.org/chrome/trunk/src/DEPS?p=284750

The links to the repos are used only to pull change logs, diffs, and the like
for analysis. So I don't think there are security issues there.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:124: 'repository':
'http://src.chromium.org/chrome/trunk',
On 2014/07/22 06:35:43, mbarbella wrote:
> Same as above.

Done.

https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py
File tools/findit/utils.py (right):

https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py#ne...
tools/findit/utils.py:21: return len(revision) < 8 and revision.isdigit()
On 2014/07/22 06:35:43, mbarbella wrote:
> This seems like a hack. It would probably be cleaner to determine that
something
> isn't a git hash based on the length.

In fact, depot_tools/roll_dep.py also does a check on length, but it could
verify the git revision against a local repos checkout.

Here, we could do a pattern matching which is more strict then checking the
length.

[Martin Barbella, 2014-07-22 20:36:42]

On 2014/07/22 20:30:35, Shuotao wrote:
> Hi Martin,
> 
> I've addressed your comments.
> Would you mind another review?
> 
> Thanks,
> Shuotao Gao
> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
> File tools/findit/chromium_deps.py (right):
> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
> tools/findit/chromium_deps.py:80: deps_file_downloader=_GetContentOfDEPS):
> On 2014/07/22 06:35:43, mbarbella wrote:
> > Please leave clear comments wherever deps_file_downloader can be specified
> that
> > it is assumed to be trusted input and will be evaluated as python code.
> 
> Good suggestion.
> Done.
> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
> tools/findit/chromium_deps.py:91: deps, deps_os, _, _, _ =
> _ParseDEPS(deps_content)
> On 2014/07/22 06:35:43, mbarbella wrote:
> > Since this is already a slightly modified copy, might as well change the
> return
> > values to only have what you need. It doesn't look like there are any other
> > calls to this.
> 
> SG. Done.
> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
> tools/findit/chromium_deps.py:104: repository =
> 'http://src.chromium.org/chrome%s' % repository
> On 2014/07/22 06:35:43, mbarbella wrote:
> > Please use https where possible. Regardless of what it's being used for
here,
> > there shouldn't be a reason to use http.
> 
> Done.
> However, later we might use whatever we have in the DEPS file.
> There are repo with http url instead of urls, like googlecode in
> https://src.chromium.org/chrome/trunk/src/DEPS?p=284750
> 
> The links to the repos are used only to pull change logs, diffs, and the like
> for analysis. So I don't think there are security issues there.

This isn't a major issue, but there shouldn't be a reason not to use https where
possible in new tools.

> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/chromium_de...
> tools/findit/chromium_deps.py:124: 'repository':
> 'http://src.chromium.org/chrome/trunk',
> On 2014/07/22 06:35:43, mbarbella wrote:
> > Same as above.
> 
> Done.
> 
> https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py
> File tools/findit/utils.py (right):
> 
>
https://codereview.chromium.org/391173002/diff/60001/tools/findit/utils.py#ne...
> tools/findit/utils.py:21: return len(revision) < 8 and revision.isdigit()
> On 2014/07/22 06:35:43, mbarbella wrote:
> > This seems like a hack. It would probably be cleaner to determine that
> something
> > isn't a git hash based on the length.
> 
> In fact, depot_tools/roll_dep.py also does a check on length, but it could
> verify the git revision against a local repos checkout.
> 
> Here, we could do a pattern matching which is more strict then checking the
> length.

Don't worry about this one then, it's not a big deal.

I'll take a quick look over this to see if I have any other minor nits, but it
seems fine in general.

[Martin Barbella, 2014-07-22 20:38:38]

lgtm

[stgao, 2014-07-22 21:19:35]

Committed patchset #6 manually as r284775 (presubmit successful).