Add experimental framework token generation tool

tools/origin_trials/generate_token.py

+#!/usr/bin/env python
+# Copyright (c) 2016 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.
+
+"""Utility for generating experimental API tokens
+
+usage: generate_token.py [-h] [--key-file KEY_FILE]
+                         [--expire-days EXPIRE_DAYS |
+                          --expire-timestamp EXPIRE_TIMESTAMP]
+                         origin trial_name
+
+Run "generate_token.py -h" for more help on usage.
+"""
+import argparse
+import base64
+import re
+import os
+import sys
+import time
+import urlparse
+
+from third_party.ed25519 import ed25519

[Nico, 2016/01/26 17:27:46]

With a snippet like e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/v8/build/gyp_v8&l=46
you don't need all these __init__.py files I think. Can you give that a try?
Having random stuff in a "top-level" third_party dir seems unfortunate

[iclelland, 2016/02/12 22:00:50]

I hate to hack the import path; Python has a well-defined and understood module
import mechanism. I'll do it to try to cut down on the number of useless files
in the codebase, though.

+
+HOSTNAME_REGEX = re.compile(

[palmer, 2016/02/11 22:03:20]

http://stackoverflow.com/a/2532344 looks more correct.

[iclelland, 2016/02/16 20:25:12]

Certainly. It handles the 255 character limit, as well as the 63 character
per-label limit. I used this regex to bring it exactly in line with the
validation in the developer console, but I've now updated it to use essentially
that logic.

+  r"""^
+  # Zero or more components, where each one is either a single alphanumeric
+  (([a-z0-9]|
+  # Or multiple alphanumerics with internal hyphens
+  [a-z0-9][a-z0-9-]*[a-z0-9])
+  # Separated by periods
+  \.)*
+  # And followed by a final component.
+  ([a-z0-9]|[a-z0-9][a-z0-9-]*[a-z0-9])$""",
+  re.IGNORECASE | re.VERBOSE
+)
+
+def OriginFromArgs(arg):

[palmer, 2016/02/11 22:03:20]

Nit: The function name suggests that the argument is a sequence, but the name
|arg| and the code use it as a single value.

I see from reading further that it's a convention with argparse. Maybe rename
|arg| to |args|?

Not important, just a nit anyway.

[iclelland, 2016/02/12 22:00:50]

It is a convention, but you're right that it's a misleading one. It's actually a
custom type function; it takes a single command-line argument and constructs an
origin from it. I've renamed it to |OriginFromArg| to be clearer.

+  """Constructs the origin for the token from the command line arguments.
+
+  Returns None if this is not possible (neither a valid hostname nor a
+  valid origin URL was provided.)
+  """
+  # Does it look like a hostname?
+  if HOSTNAME_REGEX.match(arg):
+    return 'https://'+arg
+  # Try to construct an origin URL from the argument
+  origin = urlparse.urlparse(arg)
+  if not origin or not origin.scheme or not origin.netloc:
+    raise argparse.ArgumentTypeError("%s is not a hostname or a URL" % arg)
+  # Strip any other components and return the URL:
+  return urlparse.urlunparse(origin[:2] + ('', )*4)

[palmer, 2016/02/11 22:03:20]

It's a bit mysterious (to me, at least) what urlunparse really does.
Additionally, the origin[:2] + ('', )*4 is obscure.

It might be better to return an explicit construction, including one that always
includes an explicit port. E.g.:

if origin.scheme not in ('http', 'https'):
  raise argparse.ArgumentTypeError('%s does not use a recognized URL scheme' %
arg)

port = origin.port
if not port:
  port = {'http': 80, 'https': 443}[origin.scheme]

return origin.scheme + '://' + origin.hostname + ':' + port

[palmer, 2016/02/11 22:03:20]

Nit: consistently use spaces around operators.

[iclelland, 2016/02/12 22:00:50]

I would, but PEP8 says, "If operators with different priorities are used,
consider adding whitespace around the operators with the lowest priority(ies).
Use your own judgment; however, never use more than one space, and always have
the same amount of whitespace on both sides of a binary operator."

and calls out specifically
Yes:   c = (a+b) * (a-b)
No:    c = (a + b) * (a - b)

and I don't think that either Google or Chromium style overrides it.

[iclelland, 2016/02/16 20:25:12]

Unparse is just the opposite of parse -- parse breaks a string into a tuple;
unparse creates a string, given a tuple. Obviously .build() or .construct()
would have been more descriptive, but that's just how it's spelled in the
standard library.
I can easily break that up; you're right that it's terser than it needs to be.
I'd like to avoid the explicit port for now; I don't have sufficient evidence on
the chrome side to say that it will always match the origin when we do the
comparison in the renderer. Also, the output of this tool should really match
that of the developer console which is being written concurrently, as well as
the public docs (which currently say that 443 is assumed for https)
Done.

+
+def ExpiryFromArgs(args):
+  if args.expire_timestamp:
+    return int(args.expire_timestamp)
+  return ((int(time.time()) + (int(args.expire_days) * 86400)) * 1000)
+
+def GenerateTokenData(origin, apiName, expiry):
+  return "{0}|{1}|{2}".format(origin, apiName, expiry)
+
+def Sign(private_key, data):

[palmer, 2016/02/11 22:03:20]

Nit: Consistently use private_key or privateKey style throughout. (I don't know
which is Chromium-standard; use whichever one is.)

[iclelland, 2016/02/12 22:00:50]

Thanks. I missed apiName on my first pass. PEP8 standard uses private_key for
variables; Chromium style overrides that and prescribes MixedCaseNames for
functions and methods; apiName was just wrong.

+  return ed25519.signature(data, private_key[:32], private_key[32:])
+
+def FormatToken(signature, data):
+  return base64.b64encode(signature) + "|" + data

[palmer, 2016/02/11 22:03:20]

Nit: Use "..." or '...' strings throughout; whatever the standard says.

[iclelland, 2016/02/12 22:00:50]

Done. The standard explicitly makes no recommendation, other than to be
consistent.

+
+def main():
+  parser = argparse.ArgumentParser(
+      description="Generate tokens for enabling experimental APIs")
+  parser.add_argument('origin',
+                      help="Origin for which to enable the API. This can be a "

[palmer, 2016/02/11 22:03:20]

Nit: Punctuation and conciseness. How about:

Origin for which to enable the API. This can be either a hostname (default
scheme HTTPS, default port 443) or a URL.

[iclelland, 2016/02/12 22:00:50]

Done.

+                           "bare hostname (in which case https will be "
+                           "assumed,) or a valid origin URL (hostname, "
+                           "protocol and port can all be included.)",
+                      type=OriginFromArgs)
+  parser.add_argument('trial_name',
+                      help="Feature to enable. The current list of "
+                           "experimental feature trials can be found in "
+                           "RuntimeFeatures.in")
+  parser.add_argument('--key-file',
+                      help='Ed25519 private key file to sign the token with',
+                      default="eftest.key")
+  expiry_group = parser.add_mutually_exclusive_group()
+  expiry_group.add_argument('--expire-days',
+                            help='Days from now when the token should exipire',
+                            type=int,
+                            default=42)
+  expiry_group.add_argument('--expire-timestamp',
+                            help="Exact time (milliseconds since 1970-01-01 "
+                                 "00:00:00 UTC) when the token should exipire",
+                            type=int)
+
+  args = parser.parse_args()
+  expiry = ExpiryFromArgs(args)
+
+  key_file = open(os.path.expanduser(args.key_file))
+  private_key = key_file.read()

[palmer, 2016/02/11 22:03:20]

Maybe just read the 1st 64 bytes from the file, and/or trim whitespace, to be
more tolerant of non-fatal stuff in the file? People are likely to be copying
and pasting, perhaps?

I suppose if the key contains whitespace bytes, perhaps the only thing you can
do and still be correct is to read the 1st 64 bytes.

Just a thought.

[iclelland, 2016/02/12 22:00:50]

Yeah, the key is a binary file; it wouldn't make sense to wrap it in ASCII text
without encoding it somehow anyway -- not least because the first or last bytes
could very easily be 0x20, 0x0d, 0x0a, etc.; trying to extract that from an
arbitrary text file seems out-of-scope for a tool like this anyway; if you
really have to, you should run it through uuencode/uudecode or something like
that before using it here.

I'm finding it hard to imagine a case where someone manages to mangle the file
through cut-and-paste in a way that we can reliably recover from. Maybe we scan
the entire file looking for a 64-byte section that represents a legitimate
private key?

In any case, we still have to deal with the possibility that the key file is <
64 bytes long, so I don't see the code getting any shorter here. If you think it
avoids some realistic failure case, then I'll allow longer files, and truncate
to 64 bytes  on read.

+
+  # Validate that the key file read was a proper Ed25519 key -- running the
+  # publickey method on the first half of the key should return the second
+  # half.
+  if (len(private_key) != 64 or
+    ed25519.publickey(private_key[:32]) != private_key[32:]):
+    print("Unable to use the specified private key file.")
+    sys.exit(1)
+
+  token_data = GenerateTokenData(args.origin, args.trial_name, expiry)
+  signature = Sign(private_key, token_data)
+
+  # Verify that that the signature is correct before printing it.
+  try:
+    ed25519.checkvalid(signature, token_data, private_key[32:])
+  except Exception:
+    print "There was an error generating the signature."

[palmer, 2016/02/11 22:03:20]

Maybe print the exception in case it has anything useful? (Maybe it doesn't.)

[iclelland, 2016/02/12 22:00:50]

For diagnostics, sure. The exceptions are mostly not going to be helpful to an
end user, they really indicate that something wrong happened with our code. I'll
print it out anyway, as extra information.

+    sys.exit(1)
+
+  print FormatToken(signature, token_data)
+
+if __name__ == "__main__":
+  main()