Issue 29751598: Issue 6291 - Use client certificate for Windows Store uploads (Closed)
|
Created:
April 13, 2018, 12:56 p.m. by tlucas Modified:
April 30, 2018, 5:16 p.m. Base URL:
https://hg.adblockplus.org/abpssembly/file/a67d8f0e66b2 Visibility:
Public. |
DescriptionIssue 6291 - Use client certificate for Windows Store uploads
NOTE:
sitescripts.ini in abp-builds-1 needs to be updated before this lands, a hub-ticket will be created for this in-between the LGTM's and landing this, containing the following content:
* add values `abpedge_privateKey` and `abpedge_thumbprint` -> the private key has to be created by someone with the corresponding permissions @ Azure AD, the thumbprint is returned by Azure AD's upload interface (see https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-certificate-credentials )
* Copy the privateKey to the above configured path
Patch Set 1 #
Total comments: 21
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : NOISSUE rebase against https://codereview.adblockplus.org/29753611/ #
Total comments: 6
Patch Set 6 : Addressing non-functional nits #Patch Set 7 : NO CHANGE rebase against https://codereview.adblockplus.org/29756646/ #
MessagesTotal messages: 19
Patch Set 1 * Replaced refresh_token authentication for the Windows store with client certificate authentication https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: The branch 'edge' is built from still uses the old version of buildtools, pre #6021
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/13 13:06:04, tlucas wrote: > The branch 'edge' is built from still uses the old version of buildtools, pre > #6021 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. https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:663: return base64.urlsafe_b64encode(data).replace(b'=', b'') How about .rstrip(b'=')? https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:670: now = datetime.datetime.now() 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 https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:690: segments.append(base64url_encode(json.dumps(jwt_payload))) 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))]
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/14 02:47:30, Sebastian Noack wrote: > On 2018/04/13 13:06:04, tlucas wrote: > > The branch 'edge' is built from still uses the old version of buildtools, pre > > #6021 > > 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. Yeah, i agree. @Ollie - what do you think about this? I guess we'll have a separate issue for that? https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:663: return base64.urlsafe_b64encode(data).replace(b'=', b'') On 2018/04/14 02:47:30, Sebastian Noack wrote: > How about .rstrip(b'=')? see below https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:670: now = datetime.datetime.now() On 2018/04/14 02:47:30, Sebastian Noack wrote: > 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 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(). https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:690: segments.append(base64url_encode(json.dumps(jwt_payload))) On 2018/04/14 02:47:30, Sebastian Noack wrote: > 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))] Acknowledged.
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/14 08:55:46, tlucas wrote: > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > On 2018/04/13 13:06:04, tlucas wrote: > > > The branch 'edge' is built from still uses the old version of buildtools, > pre > > > #6021 > > > > 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. > > Yeah, i agree. > @Ollie - what do you think about this? I guess we'll have a separate issue for > that? 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.
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/14 09:40:54, Sebastian Noack wrote: > On 2018/04/14 08:55:46, tlucas wrote: > > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > > On 2018/04/13 13:06:04, tlucas wrote: > > > > The branch 'edge' is built from still uses the old version of buildtools, > > pre > > > > #6021 > > > > > > 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. > > > > Yeah, i agree. > > @Ollie - what do you think about this? I guess we'll have a separate issue for > > that? > > 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. 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
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/16 04:37:07, Oleksandr wrote: > On 2018/04/14 09:40:54, Sebastian Noack wrote: > > 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. > > 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 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".
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/16 05:45:50, Sebastian Noack wrote: > and "master got merged back into "next". Sorry, I meant "edge" (not "next").
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/16 05:47:19, Sebastian Noack wrote: > On 2018/04/16 05:45:50, Sebastian Noack wrote: > > and "master got merged back into "next". > > Sorry, I meant "edge" (not "next"). 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!
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/16 10:06:07, tlucas wrote: > On 2018/04/16 05:47:19, Sebastian Noack wrote: > > On 2018/04/16 05:45:50, Sebastian Noack wrote: > > > and "master got merged back into "next". > > > > Sorry, I meant "edge" (not "next"). > > 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! https://codereview.adblockplus.org/29753557/ https://codereview.adblockplus.org/29753560/
Patch Set 2 * Use UTC-time for generating JWT * Refactor the generation of jwt's for mozilla and azure into a more generic approach * Addressed Sebastian's comments were still applicable https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:360: if self.config.type in {'safari', 'edge'}: On 2018/04/16 10:25:14, tlucas wrote: > On 2018/04/16 10:06:07, tlucas wrote: > > On 2018/04/16 05:47:19, Sebastian Noack wrote: > > > On 2018/04/16 05:45:50, Sebastian Noack wrote: > > > > and "master got merged back into "next". > > > > > > Sorry, I meant "edge" (not "next"). > > > > 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! > > https://codereview.adblockplus.org/29753557/ > https://codereview.adblockplus.org/29753560/ Done. https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:663: return base64.urlsafe_b64encode(data).replace(b'=', b'') On 2018/04/14 08:55:46, tlucas wrote: > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > How about .rstrip(b'=')? > > see below FWIW, there's no stripping / replacing done anymore (turns out it wasn't necessary for our setup) https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:670: now = datetime.datetime.now() On 2018/04/14 08:55:46, tlucas wrote: > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > 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 > > 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(). 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. https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:690: segments.append(base64url_encode(json.dumps(jwt_payload))) On 2018/04/14 08:55:46, tlucas wrote: > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > 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))] > > Acknowledged. Done.
https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:670: now = datetime.datetime.now() On 2018/04/16 14:50:09, tlucas wrote: > On 2018/04/14 08:55:46, tlucas wrote: > > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > > 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 > > > > 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(). > > 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. You are right. But time.time() gives the same result. https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:359: self.buildNum]) Nit: The indentation is off by one space here. https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:467: return ( Nit: It looks weird to use different flavor of indentation here and when returning the tuple from the method above. https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:680: Nit: This blank line looks redundant.
Patch Set 3 * Addressed Sebastian's nits https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:359: self.buildNum]) On 2018/04/16 16:13:30, Sebastian Noack wrote: > Nit: The indentation is off by one space here. Done. https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:467: return ( On 2018/04/16 16:13:29, Sebastian Noack wrote: > Nit: It looks weird to use different flavor of indentation here and when > returning the tuple from the method above. Done. https://codereview.adblockplus.org/29751598/diff/29753579/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:680: On 2018/04/16 16:13:30, Sebastian Noack wrote: > Nit: This blank line looks redundant. IMHO it doesn't, since the above two lines are not related at all to importing the privateKey. Although this blank line is merely to group the flow: compute x5t header -> import the private key -> compute the signed jwt-body.
Patch Set 4: * Addressed previously overseen nit https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29751599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:670: now = datetime.datetime.now() On 2018/04/16 16:13:29, Sebastian Noack wrote: > On 2018/04/16 14:50:09, tlucas wrote: > > On 2018/04/14 08:55:46, tlucas wrote: > > > On 2018/04/14 02:47:30, Sebastian Noack wrote: > > > > 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 > > > > > > 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(). > > > > 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. > > You are right. But time.time() gives the same result. Done.
LGTM
Hi Tristan, I haven't looked into details of the client certificate flow but overall the logic makes sense and I assume you've already tested this against the real thing. I only have a couple of nits to add (up to you whether you go with my suggestions). Overall LGTM. Cheers, Vasily https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:53: from xml.dom.minidom import parse as parseXml Nit: this should be above together with other stdlib imports. https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:465: lambda s, m: PKCS1_v1_5.new(s).sign(Crypto.Hash.SHA256.new(m)) Nit: According to the recently landed coding style update, there should be a comma at the end of this line. Ditto for line 471. https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:706: url = 'https://login.microsoftonline.com/{}/oauth2/token'.format( Preexisting version of this was kind of a hack to avoid splitting lines, so probably good riddance, but this "one-liner" now takes 3 lines :/ Perhaps one of those cases where strict line length rules are more of a pain than benefit. What do you think about this version? url_template = 'https://login.microsoftonline.com/{}/oauth2/token' url = url_template.format(self.config.tenantID)
Patch Set 6 * Addressing final, non-functional nits https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:53: from xml.dom.minidom import parse as parseXml On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > Nit: this should be above together with other stdlib imports. Done. https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:465: lambda s, m: PKCS1_v1_5.new(s).sign(Crypto.Hash.SHA256.new(m)) On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > Nit: According to the recently landed coding style update, there should be a > comma at the end of this line. Ditto for line 471. Done. https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:706: url = 'https://login.microsoftonline.com/{}/oauth2/token'.format( On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > Preexisting version of this was kind of a hack to avoid splitting lines, so > probably good riddance, but this "one-liner" now takes 3 lines :/ Perhaps one of > those cases where strict line length rules are more of a pain than benefit. > > What do you think about this version? > > url_template = 'https://login.microsoftonline.com/{}/oauth2/token' > url = url_template.format(self.config.tenantID) Done.
On 2018/04/18 12:58:31, tlucas wrote: > Patch Set 6 > * Addressing final, non-functional nits > > https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... > File sitescripts/extensions/bin/createNightlies.py (right): > > https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:53: from xml.dom.minidom import > parse as parseXml > On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > > Nit: this should be above together with other stdlib imports. > > Done. > > https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:465: lambda s, m: > PKCS1_v1_5.new(s).sign(Crypto.Hash.SHA256.new(m)) > On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > > Nit: According to the recently landed coding style update, there should be a > > comma at the end of this line. Ditto for line 471. > > Done. > > https://codereview.adblockplus.org/29751598/diff/29753614/sitescripts/extensi... > sitescripts/extensions/bin/createNightlies.py:706: url = > 'https://login.microsoftonline.com/{}/oauth2/token'.format( > On 2018/04/17 17:43:54, Vasily Kuznetsov wrote: > > Preexisting version of this was kind of a hack to avoid splitting lines, so > > probably good riddance, but this "one-liner" now takes 3 lines :/ Perhaps one > of > > those cases where strict line length rules are more of a pain than benefit. > > > > What do you think about this version? > > > > url_template = 'https://login.microsoftonline.com/{}/oauth2/token' > > url = url_template.format(self.config.tenantID) > > Done. LGTMness increased!
Still LGTM
On 2018/04/18 14:43:26, Sebastian Noack wrote: > Still LGTM Alright. I mailed ops (+ Sebastian and Ollie) about the necessary changes on abp-builds-1, they confirmed receival and will start doing it / contacting us soon. Cheers! 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
