Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25515004:
Add chromium-committers appengine app. (Closed)
|
Created:
7 years, 2 months ago by agable Modified:
7 years, 2 months ago CC:
chromium-reviews, cmp-cc_chromium.org Visibility:
Public. |
DescriptionAdd chromium-committers appengine app.
This app hosts the lists of all Chromium committers and will become the
golden source of truth for that list. It receives updates from an internal
google service. It displays the list only to logged-in users whose login email
is found in the list itself, or other apps that successfully authenticate with
it via hmac (such as the commit queue).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=227839
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 35
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 20
Patch Set 6 : #Patch Set 7 : #
Total comments: 3
Patch Set 8 : #
Messages
Total messages: 22 (0 generated)
PTAL. This is the external portion of the chromium-committers app (the internal portion living in googleplex).
https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:36: def CheckRequest(request): I would make this a decorator. Or a handler to subclass from. https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:61: if abs(now - then) > datetime.timedelta(minutes=10): I would make this 10 min a configuration option https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:75: if check.hexdigest() != auth: Need to do a constant-time comparison here. Assert len of check == len of auth, then OR the Bytewise XOR of all values in both strings into an accumulator then assert the accumulator is zero. https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:79: def CreateRequest(**params): decorator. Actually... when would this ever be used? The other party would have to know the secret key... you'd really want to use the server's pubkey and then use RSA (or ECC... one can dream) to sign.
https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:36: def CheckRequest(request): On 2013/10/03 03:05:02, iannucci wrote: > I would make this a decorator. Or a handler to subclass from. Then it couldn't be used the way it is in ChromiumHandler (in app.py), i.e. as one of two possible authentication mechanisms. https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:79: def CreateRequest(**params): On 2013/10/03 03:05:02, iannucci wrote: > decorator. > > Actually... when would this ever be used? The other party would have to know the > secret key... you'd really want to use the server's pubkey and then use RSA (or > ECC... one can dream) to sign. Uh, this is exactly what you, Vadim, and I were discussing yesterday. Yes, explicitly, both ends of the connection need to have the ID/key pair in their datastore. That's exactly what you were saying was sufficient for our purposes. This produces requests that are the exactly what the method above expects to receive. I can send you the internal CL to show how this is used to produce the requests received by the /update endpoint of this app.
https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:102: auth.update('%s=%s' % (param, value)) you need to hmac(time + id + all other stuff), and preferably with clean separators (like '\0') between fields.
https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:102: auth.update('%s=%s' % (param, value)) On 2013/10/03 17:04:53, Vadim Sh. wrote: > you need to hmac(time + id + all other stuff), and preferably with clean > separators (like '\0') between fields. That's exactly what .update() does. auth.update(a); auth.update(b) == auth.update(a + b) I'll add null separators, though.
https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:102: auth.update('%s=%s' % (param, value)) On 2013/10/03 17:46:30, Aaron Gable wrote: > On 2013/10/03 17:04:53, Vadim Sh. wrote: > > you need to hmac(time + id + all other stuff), and preferably with clean > > separators (like '\0') between fields. > > That's exactly what .update() does. > auth.update(a); auth.update(b) == auth.update(a + b) > > I'll add null separators, though. Oh whoops I see what you mean. Fixing.
PTAL https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:36: def CheckRequest(request): On 2013/10/03 15:26:40, Aaron Gable wrote: > On 2013/10/03 03:05:02, iannucci wrote: > > I would make this a decorator. Or a handler to subclass from. > > Then it couldn't be used the way it is in ChromiumHandler (in app.py), i.e. as > one of two possible authentication mechanisms. Ok, decoratorized. https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:75: if check.hexdigest() != auth: On 2013/10/03 03:05:02, iannucci wrote: > Need to do a constant-time comparison here. Assert len of check == len of auth, > then OR the Bytewise XOR of all values in both strings into an accumulator then > assert the accumulator is zero. Done.
lookin' good https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util.py File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/1/chromium-committers/auth_util... chromium-committers/auth_util.py:79: def CreateRequest(**params): On 2013/10/03 15:26:40, Aaron Gable wrote: > On 2013/10/03 03:05:02, iannucci wrote: > > decorator. > > > > Actually... when would this ever be used? The other party would have to know > the > > secret key... you'd really want to use the server's pubkey and then use RSA > (or > > ECC... one can dream) to sign. > > Uh, this is exactly what you, Vadim, and I were discussing yesterday. Yes, > explicitly, both ends of the connection need to have the ID/key pair in their > datastore. That's exactly what you were saying was sufficient for our purposes. > This produces requests that are the exactly what the method above expects to > receive. I can send you the internal CL to show how this is used to produce the > requests received by the /update endpoint of this app. Right, so the appengine code would never really call this, just clients of the appengine code. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.py File chromium-committers/app.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:47: if self.request.hmac_authenticated or self._can_see_list(user, emails): can the user auth be a decorator too? then this would be: if self.request.hmac_authenticated or self.request.user_authenticated: ... Actually just `if self.request.authenticated` (could be None, 'hmac', 'user', etc.) Then you could skip the user auth step if hmac already succeeded (or vice-versa). Order of decorators is order of auth preference. In each deco, if request.authenticated, then you get to skip the rest of the processing. or I guess authenticated could be a set() of authentication methods which have passed. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:72: setattr(self.request, 'hmac_authenticated', False) self.request.hmac_authenticated = False doesn't work? https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:105: separators=('\0', '=')) no! normal separators! blargh! https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:106: check = hmac.new(key, blob, hashlib.sha256).hexdigest() 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:108: if len(check) != len(auth): you can do this check before the dumps since you know that you're using sha256. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:151: separators=('\0'. '=')) I think this should be a comma, not a dot also no
https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.py File chromium-committers/app.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:33: email[:-11] + '@chromium.org' in committer_list): nit: replace 11 with len('@google.com') or even 'email.replace('@google.com', '@chromium.org') in committer_list'. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:37: @auth_util.CheckHmacAuth(should_403=False) Why CamelCase suddenly? Is there some code style rule to write decorators in came case I don't know about?.. @auth_util.check_hmac_auth(...) looks equally nice imho. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.yaml File chromium-committers/app.yaml (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.y... chromium-committers/app.yaml:30: builtins: why?.. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:39: def CheckHmacAuth(should_403=True): 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) https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:55: """The real decorator, conditioned on should_throw.""" should_403? https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:56: @functools.wrap(handler) wraps https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:70: self.response.write('403: Forbidden') 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). https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:78: abort_auth('No id in request.') 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:92: if abs(now - then) > datetime.timedelta(minutes=10): for inter app engine communication 10 min is forever, I believe app engine clocks are quite synchronized. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, I see you like functional languages a lot, Aaron :D Imho plain old grandfather's 'for' loop would be more readable. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:145: time = time.mktime(datetime.datetime.now().timetuple()) 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.
Refactored a bunch of stuff, turned user auth into another decorator, added a login link to the front page, and it all works. PTAL. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.py File chromium-committers/app.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:33: email[:-11] + '@chromium.org' in committer_list): On 2013/10/04 04:38:19, Vadim Sh. wrote: > nit: replace 11 with len('@google.com') > > or even 'email.replace('@google.com', '@chromium.org') in committer_list'. Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:37: @auth_util.CheckHmacAuth(should_403=False) On 2013/10/04 04:38:19, Vadim Sh. wrote: > Why CamelCase suddenly? Is there some code style rule to write decorators in > came case I don't know about?.. > > @auth_util.check_hmac_auth(...) looks equally nice imho. The google style guide says all top-level things (both classes and functions) should be CamelCase. That means that all the functions in auth_util are CamelCase. *shrug* https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... chromium-committers/app.py:47: if self.request.hmac_authenticated or self._can_see_list(user, emails): On 2013/10/03 22:20:51, iannucci wrote: > can the user auth be a decorator too? > > then this would be: > > if self.request.hmac_authenticated or self.request.user_authenticated: > ... > > Actually just `if self.request.authenticated` (could be None, 'hmac', 'user', > etc.) > > Then you could skip the user auth step if hmac already succeeded (or > vice-versa). Order of decorators is order of auth preference. In each deco, if > request.authenticated, then you get to skip the rest of the processing. > > or I guess authenticated could be a set() of authentication methods which have > passed. This makes the 403 logic much more complicated, as neither decorator can directly 403 because there might be another authentication method happening after them. It'd probably force the 403 logic into a third decorator that comes after both of the others, so it would be @auth_util.CheckHmacAuth @auth_util.CheckUserAuth @auth_util.Should403 def get(self): or something like that. Which actually isn't a terrible idea. I'll look into it. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.yaml File chromium-committers/app.yaml (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.y... chromium-committers/app.yaml:30: builtins: On 2013/10/04 04:38:19, Vadim Sh. wrote: > why?.. Good question. They're just part of the default app.yaml template I started with. Gone now. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:39: def CheckHmacAuth(should_403=True): On 2013/10/04 04:38:19, Vadim Sh. wrote: > 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) 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:55: """The real decorator, conditioned on should_throw.""" On 2013/10/04 04:38:19, Vadim Sh. wrote: > should_403? Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:56: @functools.wrap(handler) On 2013/10/04 04:38:19, Vadim Sh. wrote: > wraps Already done (I may have uploaded this patchset before beginning testing...) https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:70: self.response.write('403: Forbidden') On 2013/10/04 04:38:19, Vadim Sh. wrote: > 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). Thanks, didn't know about that! Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:72: setattr(self.request, 'hmac_authenticated', False) On 2013/10/03 22:20:51, iannucci wrote: > self.request.hmac_authenticated = False > > doesn't work? Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:78: abort_auth('No id in request.') On 2013/10/04 04:38:19, Vadim Sh. wrote: > 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. 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:92: if abs(now - then) > datetime.timedelta(minutes=10): On 2013/10/04 04:38:19, Vadim Sh. wrote: > for inter app engine communication 10 min is forever, I believe app engine > clocks are quite synchronized. 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:105: separators=('\0', '=')) On 2013/10/03 22:20:51, iannucci wrote: > no! normal separators! blargh! Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:108: if len(check) != len(auth): On 2013/10/03 22:20:51, iannucci wrote: > you can do this check before the dumps since you know that you're using sha256. Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, On 2013/10/03 22:20:51, iannucci wrote: > 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. Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, On 2013/10/04 04:38:19, Vadim Sh. wrote: > I see you like functional languages a lot, Aaron :D > > Imho plain old grandfather's 'for' loop would be more readable. 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. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:145: time = time.mktime(datetime.datetime.now().timetuple()) On 2013/10/04 04:38:19, Vadim Sh. wrote: > 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. Done. https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... chromium-committers/auth_util.py:151: separators=('\0'. '=')) On 2013/10/03 22:20:51, iannucci wrote: > I think this should be a comma, not a dot > > also no Already Done.
On 2013/10/04 21:10:14, Aaron Gable wrote: > Refactored a bunch of stuff, turned user auth into another decorator, added a > login link to the front page, and it all works. PTAL. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.py > File chromium-committers/app.py (right): > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... > chromium-committers/app.py:33: email[:-11] + '@chromium.org' in committer_list): > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > nit: replace 11 with len('@google.com') > > > > or even 'email.replace('@google.com', '@chromium.org') in committer_list'. > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... > chromium-committers/app.py:37: @auth_util.CheckHmacAuth(should_403=False) > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > Why CamelCase suddenly? Is there some code style rule to write decorators in > > came case I don't know about?.. > > > > @auth_util.check_hmac_auth(...) looks equally nice imho. > > The google style guide says all top-level things (both classes and functions) > should be CamelCase. That means that all the functions in auth_util are > CamelCase. *shrug* > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.p... > chromium-committers/app.py:47: if self.request.hmac_authenticated or > self._can_see_list(user, emails): > On 2013/10/03 22:20:51, iannucci wrote: > > can the user auth be a decorator too? > > > > then this would be: > > > > if self.request.hmac_authenticated or self.request.user_authenticated: > > ... > > > > Actually just `if self.request.authenticated` (could be None, 'hmac', 'user', > > etc.) > > > > Then you could skip the user auth step if hmac already succeeded (or > > vice-versa). Order of decorators is order of auth preference. In each deco, if > > request.authenticated, then you get to skip the rest of the processing. > > > > or I guess authenticated could be a set() of authentication methods which have > > passed. > > This makes the 403 logic much more complicated, as neither decorator can > directly 403 because there might be another authentication method happening > after them. It'd probably force the 403 logic into a third decorator that comes > after both of the others, so it would be > > @auth_util.CheckHmacAuth > @auth_util.CheckUserAuth > @auth_util.Should403 > def get(self): > > or something like that. Which actually isn't a terrible idea. I'll look into it. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.yaml > File chromium-committers/app.yaml (right): > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/app.y... > chromium-committers/app.yaml:30: builtins: > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > why?.. > > Good question. They're just part of the default app.yaml template I started > with. Gone now. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > File chromium-committers/auth_util.py (right): > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:39: def CheckHmacAuth(should_403=True): > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > 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) > > 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. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:55: """The real decorator, conditioned on > should_throw.""" > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > should_403? > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:56: @functools.wrap(handler) > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > wraps > > Already done (I may have uploaded this patchset before beginning testing...) > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:70: self.response.write('403: Forbidden') > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > 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). > > Thanks, didn't know about that! Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:72: setattr(self.request, 'hmac_authenticated', > False) > On 2013/10/03 22:20:51, iannucci wrote: > > self.request.hmac_authenticated = False > > > > doesn't work? > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:78: abort_auth('No id in request.') > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > 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. > > 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. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:92: if abs(now - then) > > datetime.timedelta(minutes=10): > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > for inter app engine communication 10 min is forever, I believe app engine > > clocks are quite synchronized. > > 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. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:105: separators=('\0', '=')) > On 2013/10/03 22:20:51, iannucci wrote: > > no! normal separators! blargh! > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:108: if len(check) != len(auth): > On 2013/10/03 22:20:51, iannucci wrote: > > you can do this check before the dumps since you know that you're using > sha256. > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, > On 2013/10/03 22:20:51, iannucci wrote: > > 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. > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:113: if reduce(lambda x,y: x or y, > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > I see you like functional languages a lot, Aaron :D > > > > Imho plain old grandfather's 'for' loop would be more readable. > > 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. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:145: time = > time.mktime(datetime.datetime.now().timetuple()) > On 2013/10/04 04:38:19, Vadim Sh. wrote: > > 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. > > Done. > > https://codereview.chromium.org/25515004/diff/11001/chromium-committers/auth_... > chromium-committers/auth_util.py:151: separators=('\0'. '=')) > On 2013/10/03 22:20:51, iannucci wrote: > > I think this should be a comma, not a dot > > > > also no > > Already Done. Ping.
Nuts... had these comments pending, didn't send them. Will review PS#5 the rest of the way https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:44: handler(self, *args, **kwargs) Should these return the value of handler (and we should return the calls of these functions)? Or is that never used? https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:48: return I would just make the else: on here set authenticated to None https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:69: finish_auth('User in allowed email list via google -> chromium map.') :(... this is teh hax... Can we at least have this as a separate filter decorator or something? auth_utils.ApplyDumbEmailDomainHacks ? Unless we really really want this behavior always for all users of this decorator... https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:90: if not getattr(self.request, 'authenticated', None): either this should be checking is not None, or the docstring should be updated to say it expects a non False-ish value. https://codereview.chromium.org/25515004/diff/28001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/28001/chromium-committers/hmac_... chromium-committers/hmac_util.py:48: logging.debug('Generating HMAC from blob: %s' % blob) could blob be superhuge? Is that a problem? DDOS on logs?
Will hold off on uploading patchset 6 until you finish reviewing patchset 5 to avoid confusion. https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... File chromium-committers/auth_util.py (right): https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:44: handler(self, *args, **kwargs) On 2013/10/09 18:57:30, iannucci wrote: > Should these return the value of handler (and we should return the calls of > these functions)? Or is that never used? webapp2 doesn't require or even want handlers to return. It waits for the handler method (get()) to exit and then just grabs the response object from the ResponseHandler object. All that's important is that self.request and self.response are maintained; no returns necessary. https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:48: return On 2013/10/09 18:57:30, iannucci wrote: > I would just make the else: on here set authenticated to None Not sure I agree with this. I think the current structure makes sense: If already authenticated, finish. Otherwise, either finish or abort. And each of those directly sets the authenticated field. Having it default to None here and then having finish and abort be asymmetric seems harder to read to me (less-linear control flow). But I'll change it if you insist. https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:69: finish_auth('User in allowed email list via google -> chromium map.') On 2013/10/09 18:57:30, iannucci wrote: > :(... this is teh hax... Can we at least have this as a separate filter > decorator or something? auth_utils.ApplyDumbEmailDomainHacks ? Unless we really > really want this behavior always for all users of this decorator... I believe this is the best way to do this. Later we will change how this works by using the *real* google -> chromium map (when this app hosts it) but for now this is the best. And I do think we will want at least behavior similar to this forever and always, since I think it is just silly and inconvenient to only let @chromium accounts see this list, or to add all @google accounts explicitly to the list too. https://codereview.chromium.org/25515004/diff/28001/chromium-committers/auth_... chromium-committers/auth_util.py:90: if not getattr(self.request, 'authenticated', None): On 2013/10/09 18:57:30, iannucci wrote: > either this should be checking is not None, or the docstring should be updated > to say it expects a non False-ish value. SG, will be done in patchset 6 (after you finish reviewing patchset 5 so we don't get even more out-of-sync). https://codereview.chromium.org/25515004/diff/28001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/28001/chromium-committers/hmac_... chromium-committers/hmac_util.py:48: logging.debug('Generating HMAC from blob: %s' % blob) On 2013/10/09 18:57:30, iannucci wrote: > could blob be superhuge? Is that a problem? DDOS on logs? The blob can be pretty big, but it hasn't given appengine a problem yet. Appengine truncates long log statements anyway. I don't think it's a problem.
https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:23: class AuthToken(ndb.Model): Can |client| or |client_id| be reused as a key of this model? https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:102: if (datetime.timedelta(seconds=abs(now - then)) > 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'). https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:166: if not ndb.Key(AuthToken, 'self').get(): 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', ...)
https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:23: class AuthToken(ndb.Model): On 2013/10/09 19:13:02, Vadim Sh. wrote: > Can |client| or |client_id| be reused as a key of this model? 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:102: if (datetime.timedelta(seconds=abs(now - then)) > On 2013/10/09 19:13:02, Vadim Sh. wrote: > 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'). Done in patchset 6. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:166: if not ndb.Key(AuthToken, 'self').get(): On 2013/10/09 19:13:02, Vadim Sh. wrote: > 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', ...) 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.
moar comments https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:31: client = ndb.StringProperty() 'name' ? 'client_name' ? https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:47: blob = urllib.urlencode(sorted(hmac_params.items())) I'm assuming this will throw if some of the parameters are non-ordered things? Like a set() or a dict()? https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:103: datetime.timedelta(minutes=1)): minutes=1? 10? Should this be at least a GLOBAL in this file, if not a real setting somewhere else? https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:104: abort_auth('Time more than 10 minutes out of sync.') message should match actual interval https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:112: if len(auth) != 64: # 256 bits / 4 bits per hexadecimal char 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:115: 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:167: AuthToken(key=ndb.Key(AuthToken, 'self')).put() Ew. Maybe just do this with a _ah/warmup handler?
https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:31: client = ndb.StringProperty() On 2013/10/09 20:34:55, iannucci wrote: > 'name' ? 'client_name' ? 'name' is a reserved keyword. will do client_name. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:47: blob = urllib.urlencode(sorted(hmac_params.items())) On 2013/10/09 20:34:55, iannucci wrote: > I'm assuming this will throw if some of the parameters are non-ordered things? > Like a set() or a dict()? I assumed so, but apparently not D: >>> f = {'asdf', 'qwer', 'zxcv'} >>> b = {'uiop', 'hjkl', 'vbnm'} >>> sorted([f, b]) [set(['zxcv', 'qwer', 'asdf']), set(['vbnm', 'uiop', 'hjkl'])] >>> sorted([b, f]) [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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:103: datetime.timedelta(minutes=1)): On 2013/10/09 20:34:55, iannucci wrote: > minutes=1? 10? > > Should this be at least a GLOBAL in this file, if not a real setting somewhere > else? 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:104: abort_auth('Time more than 10 minutes out of sync.') On 2013/10/09 20:34:55, iannucci wrote: > message should match actual interval Addressed vadim's comment. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:112: if len(auth) != 64: # 256 bits / 4 bits per hexadecimal char On 2013/10/09 20:34:55, iannucci wrote: > 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. 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:115: On 2013/10/09 20:34:55, iannucci wrote: > 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. 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. https://codereview.chromium.org/25515004/diff/37001/chromium-committers/hmac_... chromium-committers/hmac_util.py:167: AuthToken(key=ndb.Key(AuthToken, 'self')).put() On 2013/10/09 20:34:55, iannucci wrote: > Ew. Maybe just do this with a _ah/warmup handler? 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.
lgtm, let's ship this already :p https://codereview.chromium.org/25515004/diff/40001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/40001/chromium-committers/hmac_... chromium-committers/hmac_util.py:48: assert isinstance(obj, collections.Hashable) assert all(isinstance(i, collections.Hashable) for i in hmac_params.iteritems()) https://codereview.chromium.org/25515004/diff/40001/chromium-committers/hmac_... chromium-committers/hmac_util.py:122: abort_auth('Incorrect authentication (length mismatch).') I don't see why the exact length of the hash is teh sekrets. This is still leaking that it's a different length (since you can time the comparison and note that it doesn't enter the loop). This sgtm though.
lgtm too
https://codereview.chromium.org/25515004/diff/40001/chromium-committers/hmac_... File chromium-committers/hmac_util.py (right): https://codereview.chromium.org/25515004/diff/40001/chromium-committers/hmac_... chromium-committers/hmac_util.py:48: assert isinstance(obj, collections.Hashable) On 2013/10/09 23:06:49, iannucci wrote: > assert all(isinstance(i, collections.Hashable) for i in hmac_params.iteritems()) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agable@chromium.org/25515004/74001
Message was sent while issue was closed.
Change committed as 227839 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
