Add build mailer capability to support gatekeeper_ng.

Add build mailer capability to support gatekeeper_ng.

BUG=227883
R=iannucci@chromium.org

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

[Mike Stip (use stip instead), 2013-07-23 12:26:21]

ptal

Note that this is still under development (auth and tests) but any early
comments would be helpful.

[agable, 2013-08-01 17:52:44]

I'd suggest a general refactoring of gatekeeper_mailer, just because its
organization is a little bit all over the place. The html-generation logic flow
could be made clearer with some reorganization, renaming, and thought about what
does and doesn't belong inside the class.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py
File gatekeeper_mailer.py (right):

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode27
gatekeeper_mailer.py:27: url = '%sbuilders/%s/builds/%d' % (
Make sure that waterfall_url ends with '/'

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode37
gatekeeper_mailer.py:37: # escape taken from twisted/web/microdom.py
nit: throughout, make #-style comments full sentences.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode39
gatekeeper_mailer.py:39: HTML_ESCAPE_CHARS = (('&', '&'), # don't add any
entities before this one
Weird to have this between _GenBuildBox and _GenStepBox

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode46
gatekeeper_mailer.py:46: "Escape a few XML special chars with XML entities."
Make docstring.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode55
gatekeeper_mailer.py:55: # class == running or results
What does this mean? class_ will be one of "success", "warnings", etc.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode57
gatekeeper_mailer.py:57: if class_ and class_ in styles:
class_ is guaranteed to have a value, unless the index into Results fails, in
which case we have worse problems.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode96
gatekeeper_mailer.py:96: html_template = (
Might be a good idea to document each of these templates here with what's
intended to fill each %s, or make them functions that take their substitutions
as parameters and just return the completed string.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode165
gatekeeper_mailer.py:165: def UL(lst):
docstrings even for ported methods.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode165
gatekeeper_mailer.py:165: def UL(lst):
It's a little weird to have UL as a @staticmethod on this class, while the
_GenFooBox methods aren't. Either put everything in this class to have it be
self-contained, or take UL out as well.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode166
gatekeeper_mailer.py:166: # from third_party/twisted_10_2/twisted/web/html.py
add tools/build/ to beginning of port paths, since this app doesn't live in
build/.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode167
gatekeeper_mailer.py:167: io = cStringIO.StringIO()
Is this necessary? What advantage does this bring over manual string
construction?

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode174
gatekeeper_mailer.py:174: def changeToHTML(self, change):
genChangeHTML?

https://codereview.chromium.org/19878007/diff/1/mailer.py
File mailer.py (right):

https://codereview.chromium.org/19878007/diff/1/mailer.py#newcode111
mailer.py:111: if not self._validate_message(message, 'pajamas'):
Nice secret :)

https://codereview.chromium.org/19878007/diff/1/mailer.py#newcode121
mailer.py:121: self.response.out.write('json build format is incorrect')
Could be a more informative error message -- what about it is incorrect? Not
necessary though.

https://codereview.chromium.org/19878007/diff/1/mailer.py#newcode125
mailer.py:125: from_addr = build_data.get('from_addr', 'buildbot@chromium.org')
Does sending as this address work, or does appengine get unhappy because you're
sending from a different domain than the app itself?

[Vadim Sh., 2013-08-26 21:30:41]

https://codereview.chromium.org/19878007/diff/7001/app.yaml
File app.yaml (right):

https://codereview.chromium.org/19878007/diff/7001/app.yaml#newcode33
app.yaml:33: script: mailer.app
secure: always

and use https:// on sender side

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py
File gatekeeper_mailer.py (right):

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py#newcode1
gatekeeper_mailer.py:1: #!/usr/bin/env python
I'm assuming this is being converted to jinja2 now?

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py#newco...
gatekeeper_mailer.py:17: Results = ["success", "warnings", "failure", "skipped",
"exception", "retry"]
Why camelcase here instead all CAPS?

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py#newco...
gatekeeper_mailer.py:20: def _GenBuildBox(buildstatus, waterfall_url, styles):
Also why camel case in functions instead of PEP8?..

https://codereview.chromium.org/19878007/diff/7001/mailer.py
File mailer.py (right):

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode2
mailer.py:2: from google.appengine.ext import db
ndb? 

It will speed up secret fetch by automagically caching it in memcache.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode5
mailer.py:5: import hmac
nit: reorganize imports order:
1. stdlib.
2. other third party libs (google.* and webapp2 in that case).
3. modules belonging to this app.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode32
mailer.py:32: class MailerSecret(db.Model):
nit: Move this class to the top of the page before BaseHandler. I think it sort
of a convention to put models on top...

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode40
mailer.py:40: # exists only in python 2.7.
Is this comment still relevant?

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode43
mailer.py:43: utc_now = utc_now_td.days * 86400 + utc_now_td.seconds
And I think all this can be reduced to time.time(). (I hate datetime module
too).

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode45
mailer.py:45: if (utc_now < message['time']) or (utc_now - message['time'] >
60):
abs(utc_now - message['time']) > 60

clock skew can happen in any direction

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode56
mailer.py:56: return client_hash == message['sha256']
Usually this comparison is done using constant time algorithm that always runs
same number of steps regardless of actual inputs to prevent timing attacks:

if len(a) != len(b):
  return False
result = 0
for x, y in zip(a, b):
  result |= ord(x) ^ ord(y)
return result == 0


(Though I myself don't believe it's of any difference for python apps on
appengine...)

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode80
mailer.py:80: stepfields = ['started',
nit: step_fields

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode99
mailer.py:99: blob = self.request.get('json')
I'm not sure what part of the system is sending this.. would it be simpler just
to put json directly into POST body with content-type application/json instead
of application/x-www-form-urlencoded?

Alternatively, send 4 separate POST parameters: 'message', 'time', 'salt',
'sha256'.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode115
mailer.py:115: secret = str(MailerSecret.get_or_insert('mailer_secret').secret)
Why 'str' here?
I'm thinking about a possibility of 'secret' to be None. In that case str will
convert it to 'None' which is undesirable.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode128
mailer.py:128: build_data = json.loads(message['message'])
Catch ValueError here as well?

[iannucci, 2013-08-27 21:45:20]

https://codereview.chromium.org/19878007/diff/7001/mailer.py
File mailer.py (right):

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode56
mailer.py:56: return client_hash == message['sha256']
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> Usually this comparison is done using constant time algorithm that always runs
> same number of steps regardless of actual inputs to prevent timing attacks:
> 
> if len(a) != len(b):
>   return False
> result = 0
> for x, y in zip(a, b):
>   result |= ord(x) ^ ord(y)
> return result == 0
> 
> 
> (Though I myself don't believe it's of any difference for python apps on
> appengine...)

((Actually it can make a difference. I've used a padding oracle attack to get
into a python appengine app before [part of a crypto class]. Just takes a few
more runs to eliminate timing noise)).

[Mike Stip (use stip instead), 2013-08-29 19:56:46]

still WIP

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py
File gatekeeper_mailer.py (right):

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode27
gatekeeper_mailer.py:27: url = '%sbuilders/%s/builds/%d' % (
On 2013/08/01 17:52:44, Aaron Gable wrote:
> Make sure that waterfall_url ends with '/'

Done.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode37
gatekeeper_mailer.py:37: # escape taken from twisted/web/microdom.py
On 2013/08/01 17:52:44, Aaron Gable wrote:
> nit: throughout, make #-style comments full sentences.

Done.

https://codereview.chromium.org/19878007/diff/1/gatekeeper_mailer.py#newcode57
gatekeeper_mailer.py:57: if class_ and class_ in styles:
On 2013/08/01 17:52:44, Aaron Gable wrote:
> class_ is guaranteed to have a value, unless the index into Results fails, in
> which case we have worse problems.

converted to jinja, but will clean up

https://codereview.chromium.org/19878007/diff/1/mailer.py
File mailer.py (right):

https://codereview.chromium.org/19878007/diff/1/mailer.py#newcode125
mailer.py:125: from_addr = build_data.get('from_addr', 'buildbot@chromium.org')
On 2013/08/01 17:52:44, Aaron Gable wrote:
> Does sending as this address work, or does appengine get unhappy because
you're
> sending from a different domain than the app itself?

it doesn't, fixed

https://codereview.chromium.org/19878007/diff/7001/app.yaml
File app.yaml (right):

https://codereview.chromium.org/19878007/diff/7001/app.yaml#newcode33
app.yaml:33: script: mailer.app
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> secure: always
> 
> and use https:// on sender side
> 

Done.

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py
File gatekeeper_mailer.py (right):

https://codereview.chromium.org/19878007/diff/7001/gatekeeper_mailer.py#newcode1
gatekeeper_mailer.py:1: #!/usr/bin/env python
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> I'm assuming this is being converted to jinja2 now?

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py
File mailer.py (right):

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode2
mailer.py:2: from google.appengine.ext import db
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> ndb? 
> 
> It will speed up secret fetch by automagically caching it in memcache.

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode5
mailer.py:5: import hmac
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> nit: reorganize imports order:
> 1. stdlib.
> 2. other third party libs (google.* and webapp2 in that case).
> 3. modules belonging to this app.

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode32
mailer.py:32: class MailerSecret(db.Model):
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> nit: Move this class to the top of the page before BaseHandler. I think it
sort
> of a convention to put models on top...

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode40
mailer.py:40: # exists only in python 2.7.
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> Is this comment still relevant?

nope

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode43
mailer.py:43: utc_now = utc_now_td.days * 86400 + utc_now_td.seconds
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> And I think all this can be reduced to time.time(). (I hate datetime module
> too).

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode45
mailer.py:45: if (utc_now < message['time']) or (utc_now - message['time'] >
60):
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> abs(utc_now - message['time']) > 60
> 
> clock skew can happen in any direction

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode56
mailer.py:56: return client_hash == message['sha256']
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> Usually this comparison is done using constant time algorithm that always runs
> same number of steps regardless of actual inputs to prevent timing attacks:
> 
> if len(a) != len(b):
>   return False
> result = 0
> for x, y in zip(a, b):
>   result |= ord(x) ^ ord(y)
> return result == 0
> 
> 
> (Though I myself don't believe it's of any difference for python apps on
> appengine...)

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode80
mailer.py:80: stepfields = ['started',
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> nit: step_fields

Done.

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode99
mailer.py:99: blob = self.request.get('json')
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> I'm not sure what part of the system is sending this.. would it be simpler
just
> to put json directly into POST body with content-type application/json instead
> of application/x-www-form-urlencoded?
> 
> Alternatively, send 4 separate POST parameters: 'message', 'time', 'salt',
> 'sha256'.

that's scripts/slave/gatekeeper_ng.py. I'll make a separate CL there, then
update this

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode115
mailer.py:115: secret = str(MailerSecret.get_or_insert('mailer_secret').secret)
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> Why 'str' here?
> I'm thinking about a possibility of 'secret' to be None. In that case str will
> convert it to 'None' which is undesirable.

stuff from db came in as unicode, while stuff from the web wasn't. I'll look
into another way of converting it

https://codereview.chromium.org/19878007/diff/7001/mailer.py#newcode128
mailer.py:128: build_data = json.loads(message['message'])
On 2013/08/26 21:30:41, Vadim Sh. wrote:
> Catch ValueError here as well?

Done.

[Mike Stip (use stip instead), 2013-08-30 04:01:28]

ptal

[iannucci, 2013-08-30 04:24:05]

this is too big and lgtm. Probably just commit+deploy and then issue some more
CLs on top of it :)

https://chromiumcodereview.appspot.com/19878007/diff/46001/mailer.py
File mailer.py (right):

https://chromiumcodereview.appspot.com/19878007/diff/46001/mailer.py#newcode75
mailer.py:75: hashlib.sha256)
just use hmac.update() 3 times, lose the colons

https://chromiumcodereview.appspot.com/19878007/diff/46001/mailer.py#newcode79
mailer.py:79: return Email.linear_compare(client_hash, message['sha256'])
key should be hmac-sha256

[Mike Stip (use stip instead), 2013-09-01 21:31:07]

Committed patchset #11 manually as r220739 (presubmit successful).