Add chromium-committers appengine app.

chromium-committers/auth_util.py

+# Copyright (c) 2013 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Utilities for talking to another appengine app with authentication."""
+
+__author__ = 'agable@google.com (Aaron Gable)'
+
+
+import datetime
+import hashlib
+import functools
+import hmac
+import json
+import logging
+import time
+
+from google.appengine.ext import ndb
+
+
+class AuthError(Exception):
+  pass
+
+
+class AuthToken(ndb.Model):
+  """Represents an id/key pair for authentication.
+
+  This app also uses the id field as the id/name (unique datastore key) of the
+  object for easy lookup. The app's own AuthToken is stored with name 'self'.
+
+  Attributes:
+    id: The unique identity for this token.
+    token: The corresponding authentication key.
+  """
+  id = ndb.StringProperty()
+  token = ndb.StringProperty()
+
+
+def CheckHmacAuth(should_403=True):

[Vadim Sh., 2013/10/04 04:38:19]

single default arg in such pseudo decorators (decorator factories actually) is
confusing:

@CheckHmacAuth()
def stuff():
  ...

works

@CheckHmacAuth
def stuff():
  ...

doesn't :)

I've seen people do stuff like:

def CheckHmacAuth(should_403=True):
  if inspec.isfunction(should_403):
     # Ohh.. we are called as decorator
     ...
     return wrapper
  else:
     # We are decorator factory
     ...
     return decorator

I have mixed feelings about this, but I definitely don't like @CheckHmacAuth()
with empty arg list.

(you can choose to ignore this)

[agable, 2013/10/04 21:10:15]

I absolutely agree. I thought about this for a while when I first wrote it
because I didn't want to have decorators with empty args either. I considered
not giving should_403 a default value just to force the hand of the client code,
but decided that was even worse in a different way. I also really don't like the
switch/case on whether should_403 is callable -- that's hack, ugly, horrific,
and unreadable :P

I'm going to leave this as-is for now, but see my response to iannucci@'s
comment about making user auth a decorator too. If I do that, then I have to
make 403'ing on auth failure its own decorator too, and then none of them will
have arguments.

+  """Decorator for webapp2 request handler methods.
+
+  Only use on webapp2.RequestHandler methods (e.g. get, post, put).
+
+  Expects the request to contain:
+    id: Unique ID of requester, used to get an AuthToken from ndb
+    time: Unix epoch time the request was made (to prevent replay attacks)
+    auth: The hmac(key, id+time+params, sha256).hexdigest, to authenticate
+      the request, where the key is the matching token in the ID's AuthToken
+    **params: All of the request GET/POST parameters
+
+  If should_403 is True, simply 403s if things don't check out right.
+  Otherwise, sets request.hmac_authenticated to True or False, as appropriate.
+  """
+  def decorator(handler):
+    """The real decorator, conditioned on should_throw."""

[Vadim Sh., 2013/10/04 04:38:19]

should_403?

[agable, 2013/10/04 21:10:15]

Done.

+    @functools.wrap(handler)

[Vadim Sh., 2013/10/04 04:38:19]

wraps

[agable, 2013/10/04 21:10:15]

Already done (I may have uploaded this patchset before beginning testing...)

+    def wrapper(self, *args, **kwargs):
+      """Does the real legwork and calls the wrapped handler or 403s."""
+      def abort_auth(log_msg):
+        """Helper method to be an exit hatch when authentication fails.
+
+        If should_403 is True, writes a 403 to the response.
+        If should_403 is False, sets hmac_authenticated to False and falls
+        through to the wrapped handler.
+        """
+        logging.info(log_msg)
+        if should_403:
+          self.response.headers['Content-Type'] = 'text/plain'
+          self.reponse.status = 403
+          self.response.write('403: Forbidden')

[Vadim Sh., 2013/10/04 04:38:19]

Same code shorter and ensuring request processing aborts:

self.abort(403, detail='403: Forbidden')

(afaik just self.abort(403) would produce appropriate text body as well).

[agable, 2013/10/04 21:10:15]

Thanks, didn't know about that! Done.

+        else:
+          setattr(self.request, 'hmac_authenticated', False)

[iannucci, 2013/10/03 22:20:51]

self.request.hmac_authenticated = False

doesn't work?

[agable, 2013/10/04 21:10:15]

Done.

+          handler(self, *args, **kwargs)
+
+      # Get the id, time, and auth fields from the request.
+      id = self.request.get('id')
+      if not id:
+        abort_auth('No id in request.')

[Vadim Sh., 2013/10/04 04:38:19]

I'd rename it into 'finish_auth' or something without 'abort'. Because with
should_403=False it's not an abort but exact opposite thing: carry one with the
request right now.

[agable, 2013/10/04 21:10:15]

I'd say that it still clearly is an abort -- of the authentication, not of the
request -- as the function name says. And 'finish_auth' sounds like it's
finishing authenticating, not giving up. Point taken though, abort does sound a
bit extreme. I'll try to think of something else.

+        return
+      authtoken = ndb.Key(AuthToken, id).get()
+      if not authtoken:
+        abort_auth('No auth token found for id %s.' % id)
+        return
+      key = authtoken.token
+
+      time = self.request.get('time')
+      if not time:
+        abort_auth('No time in request.')
+        return
+      then = datetime.datetime.fromtimestamp(time)
+      now = datetime.datetime.now()
+      if abs(now - then) > datetime.timedelta(minutes=10):

[Vadim Sh., 2013/10/04 04:38:19]

for inter app engine communication 10 min is forever, I believe app engine
clocks are quite synchronized. 

[agable, 2013/10/04 21:10:15]

Agreed. This isn't solely for inter-appengine communication, though. For
example. the commit queue might pull the committer list directly from this app
eventually, and would have to authenticate with it using this method. I changed
it to 1 minute, though, because seriously this is the internet age and our
clocks shouldn't be more than 10 seconds off. If they are, we *deserve* to fail.

+        abort_auth('Time more than 10 minutes out of sync.')
+        return
+
+      auth = self.request.get('auth')
+      if not auth:
+        abort_auth('No auth in request.')
+        return
+
+      # Don't include the auth hmac itself in the check.
+      params = self.request.params.copy()
+      params.pop('auth')
+      blob = json.dumps(.params, ensure_ascii=True, sort_keys=True,
+                        separators=('\0', '='))

[iannucci, 2013/10/03 22:20:51]

no! normal separators! blargh!

[agable, 2013/10/04 21:10:15]

Done.

+      check = hmac.new(key, blob, hashlib.sha256).hexdigest()

[iannucci, 2013/10/03 22:20:51]

hmac should really return a file-like object which has a write method... then
you could json.dump into it without allocating the intermediate string. meh.

+
+      if len(check) != len(auth):

[iannucci, 2013/10/03 22:20:51]

you can do this check before the dumps since you know that you're using sha256.

[agable, 2013/10/04 21:10:15]

Done.

+        abort_auth('Incorrect authentication (length mismatch).')
+        return
+
+      # Constant time comparison.
+      if reduce(lambda x,y: x or y,

[iannucci, 2013/10/03 22:20:51]

I think you want | here not or, since you don't want to short-circuit. Otherwise
they can tell which Nth byte is wrong.

Also use operator.or_
(http://docs.python.org/2/library/operator.html#operator.or_) instead of lambda.

[Vadim Sh., 2013/10/04 04:38:19]

I see you like functional languages a lot, Aaron :D

Imho plain old grandfather's 'for' loop would be more readable.

[agable, 2013/10/04 21:10:15]

Done.

[agable, 2013/10/04 21:10:15]

I do! And I disagree -- this is exactly explicitly the case reduce is written
for. If I don't get to use it now, I am a sad panda. I removed the lambda,
though.

+                (ord(a) ^ ord(b) for a, b in zip(check, auth)), 0):
+        abort_auth('Incorrect authentication.')
+        return
+
+      # Hooray, they made it!
+      setattr(self.request, 'hmac_authenticated', True)
+      handler(self, *args, **kwargs)
+
+    return wrapper
+  return decorator
+
+
+def CreateRequest(**params):
+  """Given a payload to send, constructs an authenticated request.
+
+  Returns a dictionary containing:
+    id: Unique  ID of this app, from the datastore AuthToken 'self'
+    time: Current Unix epoch time
+    auth: The hmac(key, id+time+parms, sha256), to authenticate the request,
+      where the key is the corresponding token in the app's AuthToken
+    **params: All of the GET/POST parameters
+
+  It is up to the calling code to convert this dictionary into valid GET/POST
+  parameters.
+  """
+  authtoken = ndb.Key(AuthToken, 'self').get()
+  if not authtoken:
+    raise AuthError('No AuthToken found for this app.')
+  id = authtoken.id
+  key = authtoken.token
+
+  time = time.mktime(datetime.datetime.now().timetuple())

[Vadim Sh., 2013/10/04 04:38:19]

I think code that generates HMAC for a request, and re-generates it for a check
in CheckHmacAuth should be exactly the same, i.e. same function.

[agable, 2013/10/04 21:10:15]

Done.

+
+  ret = params.copy()
+  ret.update({'id': id, 'time': time})
+
+  blob = json.dumps(ret, ensure_ascii=True, sort_keys=True,
+                    separators=('\0'.  '='))

[iannucci, 2013/10/03 22:20:51]

I think this should be a comma, not a dot

also no

[agable, 2013/10/04 21:10:15]

Already Done.

+  auth = hmac.new(key, blob, hashlib.sha256).hexdigest()
+
+  ret['auth'] = auth
+  return ret