Issue 6291 - Use client certificate for Windows Store uploads

sitescripts/extensions/bin/createNightlies.py

Index: sitescripts/extensions/bin/createNightlies.py
diff --git a/sitescripts/extensions/bin/createNightlies.py b/sitescripts/extensions/bin/createNightlies.py
index 4bd0f7fe3792361c1211c7049bf6f09ac0acb264..3a1d3e2b91e219dae19693d46bf7b470eb680725 100644
--- a/sitescripts/extensions/bin/createNightlies.py
+++ b/sitescripts/extensions/bin/createNightlies.py
@@ -25,7 +25,9 @@ Nightly builds generation script
 
 import argparse
 import ConfigParser
+import binascii
 import base64
+import datetime
 import hashlib
 import hmac
 import json
@@ -39,12 +41,17 @@ import subprocess
 import sys
 import tempfile
 import time
+import uuid
 from urllib import urlencode
 import urllib2
 import urlparse
 import zipfile
 import contextlib
 
+from Crypto.PublicKey import RSA
+from Crypto.Signature import PKCS1_v1_5
+import Crypto.Hash.SHA256
+
 from xml.dom.minidom import parse as parseXml
 
 from sitescripts.extensions.utils import (
@@ -350,7 +357,7 @@ class NightlyBuild(object):
                 env = dict(env, SPIDERMONKEY_BINARY=spiderMonkeyBinary)
 
             command = [os.path.join(self.tempdir, 'build.py')]
-            if self.config.type == 'safari':
+            if self.config.type in {'safari', 'edge'}:

[tlucas, 2018/04/13 13:06:04]

The branch 'edge' is built from still uses the old version of buildtools, pre
#6021

[Sebastian Noack, 2018/04/14 02:47:30]

I would rather see a dependency update landing in the "edge" branch. That way we
won't have to bother about changing the code here again, once we merge "master"
back into the "edge" branch.

[tlucas, 2018/04/14 08:55:46]

Yeah, i agree.
@Ollie - what do you think about this? I guess we'll have a separate issue for
that?

[Sebastian Noack, 2018/04/14 09:40:54]

Alternatively, we could add a hack to build.py:

  args = sys.argv[:]
  if '-t' in args:
      index_opt = args.index('-t')
      index_val = index_opt + 1
      if index_val < len(args):
          args.insert(1, args.pop(index_opt))
          args.insert(2, args.pop(index_val))
  buildtools.build.processArgs(BASE_DIR, *args)

This might be less problematic than a dependency update which might require more
changes that will be redundant once we merge "master" back into "edge".

We could even apply the same hack in the "safari" branch and remove this code
path here altogether, and also avoid confusion about inconsistent expected order
of arguments when using build.py during development and release.

[Oleksandr, 2018/04/16 04:37:07]

I would rather merge `master` into `edge` first. That's what my
https://hg.adblockplus.org/buildtools/rev/a3db4a1a49e8 was for. I don't think we
need an issue for that. I have merged as a `Noissue` commit before. There's
currently no conflicts, however the build still fails on Windows because
adblockplusui guys decided they can rely on existence of `bash` everywhere :/
See https://issues.adblockplus.org/ticket/6587

[Sebastian Noack, 2018/04/16 05:45:50]

Wouldn't this rather be a reason to go with my suggested workaround for now?
So that we can move forward here, and just remove it again, once issue 6587 got
sorted, the adblockplusui and buildtools dependencies got updated, and "master"
got merged back into "next".

[Sebastian Noack, 2018/04/16 05:47:19]

Sorry, I meant "edge" (not "next").

[tlucas, 2018/04/16 10:06:07]

All of the above does IMHO encourage a workaround - and i guess
adblockpluschrome @edge is the right place to do it. I'll file a review soon,
thank you!

[tlucas, 2018/04/16 10:25:14]

https://codereview.adblockplus.org/29753557/
https://codereview.adblockplus.org/29753560/

[tlucas, 2018/04/16 14:50:09]

Done.

                 command.extend(['-t', self.config.type, 'build'])
             else:
                 command.extend(['build', '-t', self.config.type])
@@ -649,21 +656,72 @@ class NightlyBuild(object):
         if any(status not in ('OK', 'ITEM_PENDING_REVIEW') for status in response['status']):
             raise Exception({'status': response['status'], 'statusDetail': response['statusDetail']})
 
+    def generate_certificate_token_request(self, url, private_key):
+        # Construct the token request according to
+        # https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-certificate-credentials
+        def base64url_encode(data):
+            return base64.urlsafe_b64encode(data).replace(b'=', b'')

[Sebastian Noack, 2018/04/14 02:47:30]

How about .rstrip(b'=')?

[tlucas, 2018/04/14 08:55:46]

see below

[tlucas, 2018/04/16 14:50:09]

FWIW, there's no stripping / replacing done anymore (turns out it wasn't
necessary for our setup)

+
+        segments = []
+
+        hex_val = binascii.a2b_hex(self.config.thumbprint)
+        x5t = base64.urlsafe_b64encode(hex_val).decode()
+
+        now = datetime.datetime.now()

[Sebastian Noack, 2018/04/14 02:47:30]

It seems you are relying on the fact that our servers run with UTC timezone. It
would be better to request UTC time explicitly. Also you can create a timetuple
right away, without first creating a datetime object.

  now = int(time.mktime(time.gmtime()))
  expires = now + 600

[tlucas, 2018/04/14 08:55:46]

I'm actually thinking about refactoring parts of this in to
generate_jwt_request(), where no stripping (above comment) is done and which
already uses mktime().

[tlucas, 2018/04/16 14:50:09]

Done, almost: mktime() expects a time_struct in local_time format and produces
UTC-time out of it, so i'll use time.localtime() instead.

[Sebastian Noack, 2018/04/16 16:13:29]

You are right. But time.time() gives the same result.

[tlucas, 2018/04/16 16:50:24]

Done.

+        minutes = datetime.timedelta(0, 0, 0, 0, 10)
+        expires = now + minutes
+
+        # generate the full jwt body
+        jwt_payload = {
+            'aud': url,
+            'iss': self.config.clientID,
+            'sub': self.config.clientID,
+            'nbf': int(time.mktime(now.timetuple())),
+            'exp': int(time.mktime(expires.timetuple())),
+            'jti': str(uuid.uuid4()),
+        }
+
+        jwt_headers = {'typ': 'JWT', 'alg': 'RS256', 'x5t': x5t}
+
+        # sign the jwt body with the given private key
+        key = RSA.importKey(private_key)
+
+        segments.append(base64url_encode(json.dumps(jwt_headers)))
+        segments.append(base64url_encode(json.dumps(jwt_payload)))

[Sebastian Noack, 2018/04/14 02:47:30]

Since we don't append to segments above, how about defining the list here?

  segments = [base64url_encode(json.dumps(jwt_headers)),
              base64url_encode(json.dumps(jwt_payload))]

[tlucas, 2018/04/14 08:55:46]

Acknowledged.

[tlucas, 2018/04/16 14:50:09]

Done.

+
+        body = b'.'.join(segments)
+        signature = PKCS1_v1_5.new(key).sign(Crypto.Hash.SHA256.new(body))
+
+        segments.append(base64url_encode(signature))
+        signed_jwt = b'.'.join(segments)
+
+        # generate oauth parameters for login.microsoft.com
+        oauth_params = {
+            'grant_type': 'client_credentials',
+            'client_id': self.config.clientID,
+            'resource': 'https://graph.windows.net',
+            'client_assertion_type': 'urn:ietf:params:oauth:client-assertion-'
+                                     'type:jwt-bearer',
+            'client_assertion': signed_jwt,
+        }
+
+        request = urllib2.Request(url, urlencode(oauth_params))
+        request.get_method = lambda: 'POST'
+
+        return request
+
     def get_windows_store_access_token(self):
-        # use refresh token to obtain a valid access token
-        # https://docs.microsoft.com/en-us/azure/active-directory/active-directory-protocols-oauth-code#refreshing-the-access-tokens
-        server = 'https://login.microsoftonline.com'
-        token_path = '{}/{}/oauth2/token'.format(server, self.config.tenantID)
+        # use client certificate to obtain a valid access token
+        url = 'https://login.microsoftonline.com/{}/oauth2/token'.format(
+            self.config.tenantID
+        )
+
+        with open(self.config.privateKey, 'r') as fp:
+            private_key = fp.read()
 
         opener = urllib2.build_opener(HTTPErrorBodyHandler)
-        post_data = urlencode([
-            ('refresh_token', self.config.refreshToken),
-            ('client_id', self.config.clientID),
-            ('client_secret', self.config.clientSecret),
-            ('grant_type', 'refresh_token'),
-            ('resource', 'https://graph.windows.net')
-        ])
-        request = urllib2.Request(token_path, post_data)
+        request = self.generate_certificate_token_request(url, private_key)
+
         with contextlib.closing(opener.open(request)) as response:
             data = json.load(response)
             auth_token = '{0[token_type]} {0[access_token]}'.format(data)
@@ -814,7 +872,7 @@ class NightlyBuild(object):
                 self.uploadToMozillaAddons()
             elif self.config.type == 'chrome' and self.config.clientID and self.config.clientSecret and self.config.refreshToken:
                 self.uploadToChromeWebStore()
-            elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID:
+            elif self.config.type == 'edge' and self.config.clientID and self.config.tenantID and self.config.privateKey and self.config.thumbprint:
                 self.upload_to_windows_store()
 
         finally: