Add chromium-committers appengine app.

chromium-committers/hmac_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 generating and verifying hmac authentication."""
+
+__author__ = 'agable@google.com (Aaron Gable)'
+
+
+import datetime
+import hashlib
+import functools
+import hmac
+import json
+import logging
+import operator
+import time
+import urllib
+
+from google.appengine.ext import ndb
+
+
+class AuthToken(ndb.Model):

[Vadim Sh., 2013/10/09 19:13:02]

Can |client| or |client_id| be reused as a key of this model?

[agable, 2013/10/09 19:23:51]

I'd like to reuse client_id as the key of the model. Unfortunately, if you're
using appengine.google.com to directly modify the datastore, you can't manually
set the key. So doing so would also require writing an admin page to add / edit
/ remove the stored keys. While I'd like to do that eventually, it's a lower
priority and won't make it into version 1.0.

+  """Represents an id/key pair for authentication.
+
+  Attributes:
+    client: The human-readable name of the client.
+    client_id: The unique identity for the client that uses this token.
+    secret: The corresponding authentication key.
+  """
+  client = ndb.StringProperty()

[iannucci, 2013/10/09 20:34:55]

'name' ?  'client_name' ?

[agable, 2013/10/09 22:36:04]

'name' is a reserved keyword. will do client_name.

+  client_id = ndb.StringProperty()
+  secret = ndb.StringProperty()
+
+
+def GenerateHmac(authtoken, t=None, **params):
+  """Generates an HMAC cryptographic hash of the given parameters.
+
+  Can be used either both for generating outgoing authentication and for
+  validating incoming requests. Automatically included the authtoken's client_id
+  and the time in the hashed parameter blob. If t (timestamp) is None, uses now.
+  """
+  if t is None:
+    t = str(int(time.time()))
+  hmac_params = params.copy()
+  hmac_params.update({'id': authtoken.client_id, 't': t})
+  blob = urllib.urlencode(sorted(hmac_params.items()))

[iannucci, 2013/10/09 20:34:55]

I'm assuming this will throw if some of the parameters are non-ordered things?
Like a set() or a dict()?

[agable, 2013/10/09 22:36:04]

I assumed so, but apparently not D:
[set(['zxcv', 'qwer', 'asdf']), set(['vbnm', 'uiop', 'hjkl'])]
[set(['vbnm', 'uiop', 'hjkl']), set(['zxcv', 'qwer', 'asdf'])]
Apparently sets are sortable... though I have no clue what the comparator is.
Probably just "True". I now assert that everything is hashable, which is what we
really want.

+  logging.debug('Generating HMAC from blob: %s' % blob)
+  return hmac.new(authtoken.secret, blob, hashlib.sha256).hexdigest()
+
+
+def CheckHmacAuth(handler):
+  """Decorator for webapp2 request handler methods.
+
+  Only use on webapp2.RequestHandler methods (e.g. get, post, put).
+
+  Expects the handler's self.request to contain:
+    id: Unique ID of requester, used to get an AuthToken from ndb
+    t: 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
+
+  Sets request.authenticated to 'hmac' if successful. Otherwise, None.
+  """
+  @functools.wraps(handler)
+  def wrapper(self, *args, **kwargs):
+    """Does the real legwork and calls the wrapped handler."""
+    def abort_auth(log_msg):
+      """Helper method to be an exit hatch when authentication fails."""
+      logging.warning(log_msg)
+      self.request.authenticated = None
+      handler(self, *args, **kwargs)
+
+    def finish_auth(log_msg):
+      """Helper method to be an exit hatch when authentication succeeds."""
+      logging.info(log_msg)
+      handler(self, *args, **kwargs)
+
+    if getattr(self.request, 'authenticated', None):
+      finish_auth('Already authenticated.')
+      return
+
+    # Get the id, time, and auth fields from the request.
+    client_id = self.request.get('id')
+    if not client_id:
+      abort_auth('No id in request.')
+      return
+    logging.debug('Request contained id: %s' % client_id)
+    authtoken = AuthToken.query(AuthToken.client_id == client_id).get()
+    if not authtoken:
+      abort_auth('No auth token stored for client.')
+      return
+    logging.debug('AuthToken is from client: %s' % authtoken.client)
+
+    then = int(self.request.get('t', '0'))
+    if not then:
+      abort_auth('No timestamp in request.')
+      return
+    logging.debug('Request generated at time: %s' % then)
+    now = int(time.time())
+    if (datetime.timedelta(seconds=abs(now - then)) >

[Vadim Sh., 2013/10/09 19:13:02]

nit: abs(now - then) > 60, it's shorter.
Also change '10 minutes' in a message below to 1 min. Or just remove it from the
message (i.e. just 'token expired').

[agable, 2013/10/09 19:23:51]

Done in patchset 6.

+        datetime.timedelta(minutes=1)):

[iannucci, 2013/10/09 20:34:55]

minutes=1? 10?

Should this be at least a GLOBAL in this file, if not a real setting somewhere
else?

[agable, 2013/10/09 22:36:04]

In general I'm opposed to GLOBALS except for where they help unify code. If
there are four methods that need to reference a magic name or number, definitely
put it in a global. This code is written so each constant you propose
(expiration time, hash algo, hex chars) is only used in one place. I also rather
doubt that these constants are the sorts of parameters that would be tuned in
different deployments of this code. 1 minute should be enough for anyone, and
there's definitely no reason to use anything other than sha256 and hexdigest. I
would rather keep their definitions at their uses for the sake of readability.

+      abort_auth('Time more than 10 minutes out of sync.')

[iannucci, 2013/10/09 20:34:55]

message should match actual interval

[agable, 2013/10/09 22:36:04]

Addressed vadim's comment.

+      return
+
+    auth = self.request.get('auth')
+    if not auth:
+      abort_auth('No auth in request.')
+      return
+    logging.debug('Request contained auth hash: %s' % auth)
+    if len(auth) != 64:  # 256 bits / 4 bits per hexadecimal char

[iannucci, 2013/10/09 20:34:55]

Instead of comment, maybe:

HEX_CHARS_PER_BYTE = 2
HASH_ALGO = hashlib.sha256

...

if len(auth) != HASH_ALGO().digest_size * HEX_CHARS_PER_BYTE:


I would also maybe make a  at the top of the file too.

[agable, 2013/10/09 22:36:04]

See response to your comment about the expiration time GLOBAL. However, rather
than compare directly to 64, which is leaking implementation details (hex
encoding and hash algo) I now compare the length of auth to the length of check.
That way the backing algorithms can be changed and we're just asserting that
they're the same on both sides, whatever they are.

+      abort_auth('Incorrect authentication (length mismatch).')
+      return
+

[iannucci, 2013/10/09 20:34:55]

I would additionally do binascii.unhexlify(auth), which will TypeError if 'auth'
isn't hex-encoded.

It would then make sense to do the same to check (or perhaps have GenerateHmac
return the hmac object which you can call either digest() or hexdigest() on),
and then do the constant time comparison on the binary strings instead of the
hex strings.

[agable, 2013/10/09 22:36:04]

Interesting. I guess I don't see the benefit? Especially with my changes higher
in the file, I think it's important that this method has no knowledge of what's
actually going on in GenerateHmac. It doesn't care if auth/check are hex, or
binary, or whatever, it'll still just work.

+    # Don't include the auth hmac itself in the check.
+    params = self.request.params.copy()
+    params.pop('auth')
+    check = GenerateHmac(authtoken, **params)
+    logging.debug('Expected auth hash is: %s' % check)
+
+    # Constant time comparison.
+    if reduce(operator.or_,
+              (ord(a) ^ ord(b) for a, b in zip(check, auth)), 0):
+      abort_auth('Incorrect authentication.')
+      return
+
+    # Hooray, they made it!
+    self.request.authenticated = 'hmac'
+    handler(self, *args, **kwargs)
+
+  return wrapper
+
+
+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'
+    t: Current Unix epoch time
+    auth: The hmac(key, id+time+parms, sha256), to authenticate the request,
+      where the key is the corresponding secret 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.')
+
+  now = str(int(time.time()))
+
+  ret = params.copy()
+  ret.update({'id': authtoken.client_id, 't': now,
+              'auth': GenerateHmac(authtoken, t=now, **params)})
+  return ret
+
+
+class AuthError(Exception):
+  pass
+
+
+# There needs to be one AuthToken in the datastore so it can be added or edited
+# from the admin console. Do this one-time setup when this module is imported.
+if not ndb.Key(AuthToken, 'self').get():

[Vadim Sh., 2013/10/09 19:13:02]

Are you sure ndb magic works outside of request handler? I'd put it into 

class AuthToken(...):
  @classmethod
  def get_self(cls.):
     return cls.get_or_insert('self', ...)

[agable, 2013/10/09 19:23:51]

Yep, it works (have confirmed via dev_appserver as well as both the appengine
and googleplex instances of the app). This happens as soon as the file is
imported, which happens on startup of each instance. I'd rather do it here than
in a classmethod a) so it only happens once per instance (rather than once per
outgoing request), and b) to make it more obvious that it's global
configuration. I'll probably move it to the handler for the admin key editing
page when that gets created.

+  AuthToken(key=ndb.Key(AuthToken, 'self')).put()

[iannucci, 2013/10/09 20:34:55]

Ew. Maybe just do this with a _ah/warmup handler?

[agable, 2013/10/09 22:36:04]

Tried that. Doesn't work. /_ah/warmup is only called when a new instance is spun
up *in anticipation of increased demand*. The first instance after a fresh
upload, or a replacement instance after one crashes, or an instance spun up for
a brand new request... none of those get /_ah/warmup called. So it's good for
warming up cache in the midst of heavy traffic, but not good for general app
initialization.