Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772483002:
Commment signed webapks working and verified. (Closed)
DescriptionCommment signed webapks working with verification.
* In my test it took 11ms to verify the signature.
* Takes about 1ms to check if it's a webapk at all (vs 0.9ms
previously).
BUG=704213
Review-Url: https://codereview.chromium.org/2772483002
Cr-Commit-Position: refs/heads/master@{#464579}
Committed: https://chromium.googlesource.com/chromium/src/+/45e1d14b15dd10b4d08481d1797255c19558c824
Patch Set 1 #Patch Set 2 : Code cleanup created a duplicate static variable name. #Patch Set 3 : Fix some linter errors. #Patch Set 4 : Use the correct equals method. #Patch Set 5 : Correct override of equals. #Patch Set 6 : Add hashCode method. #Patch Set 7 : Remove unused import. #
Total comments: 2
Patch Set 8 : Wrong package order. #
Total comments: 58
Patch Set 9 : Respond to review comments. #Patch Set 10 : Respond to review comments. #
Total comments: 28
Patch Set 11 : Copy the arrays to prevent error in try bot. #
Total comments: 13
Patch Set 12 : Copy the arrays to prevent error in try bot. #Patch Set 13 : Handle review comments. #Patch Set 14 : Merge in change to not create a copy of the bytes. #
Total comments: 2
Patch Set 15 : Resond to Peter's comments. #
Total comments: 104
Patch Set 16 : Fix review comments. #Patch Set 17 : Handle reviews #Patch Set 18 : Gclient sync/merge. #Patch Set 19 : Cleanup merge. #
Total comments: 17
Patch Set 20 : Reply to review comments. #
Total comments: 38
Patch Set 21 : Resolve review comments. #
Total comments: 2
Patch Set 22 : Added flag and resolved review comments. #
Total comments: 2
Patch Set 23 : Handle comments. #Patch Set 24 : Add flag. #Patch Set 25 : Merge to head. #
Total comments: 5
Patch Set 26 : Fix setting of flags. #Messages
Total messages: 150 (115 generated)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scottkirkwood@chromium.org changed reviewers: + pkotwicz@chromium.org
Comment signed APK with public key check for WebAPKs (aka Enhanced A2HS)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still looking at this CL. Sorry for the delay https://codereview.chromium.org/2772483002/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:233: } This is a duplicate of lines 221 - 224?
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CL mostly looks. Mostly minor comments https://codereview.chromium.org/2772483002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java:7: /** Public keys and signatures for WebAPKs */ Nit: Singular. There is one public and one signature https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:150: PackageManager.GET_SIGNATURES | PackageManager.GET_META_DATA); Is this call slower when called with PackageManager.Get_SIGNATURES (I am unsure) https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:166: logTime(now, "isCommentSignedWebApk"); I think that it is useful to have a histogram which logs how long WebappLauncherActivity#isValidWebApk() takes. I don't think that the time logs here are useful. They are only useful for our own testing (we can easily build a custom version of Chrome with the logging) - The timing logs are not helpful when trying to debug bugs reported by users - The timing logs are not helpful in debugging a crash - The timing logs make Chrome bigger I suggest removing the timing logs https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { I don't think that this check is useful anymore. Isn't a comment signed WebAPK still valid if the user decides to sign it many many times? https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:200: private static boolean foundV1Signature(PackageInfo packageInfo) { You can probably merge isV1WebApk() and foundV1Signature() verifyV1WebApk() perhaps https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:284: } Is there a reason that we are copying the array? My understanding is that we are copying the array because Java does not offer const references the way that C++ does. I don't think that we need to modify the array because this class is not mutating the array https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:298: } catch (InvalidKeySpecException e) { Nit: You can probably catch the generic Exception instead and have a single catch() statement. You don't need to do any logging here because you already do logging in the caller of getCommentSignedPublicKey() https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:28: public class WebApkVerifySignature { Can you split this class into two. One class which does the generic ZIP file reading (reading the central directory) and one which does the verification and signature computation. ZipCentralDirectoryReader perhaps with two methods String ZipCentralDirectoryReader#getFileComment() ArrayList<ZipCentralDirectoryReader.Block> ZipCentralDirectoryReader#readEntries() https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:51: Pattern.compile("webapk:\\s*(\\d[^:]+:\\s*)?([a-fA-F0-9]*)"); Nit: webapk -> chrome-webapk Is there a place which describes the format of the signature? In particular what is the purpose of the characters before the second ':' is. It looks like WebApkVerifySignature.java ignores those characters It looks like this code assumes that the signature is at the end of the file comment. If this is the case you can probably allow the signature to have unicode values between 0-255 and simplify hexToBytes() We should also document that we assume that the signature is at the end of the file comment https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:186: byte[] bytes = new byte[block.mCompressedSize]; You might be allocating an array which is many kilobytes. - Should you call Signature#update() several times to avoid copying so much data? - Can you avoid the copy: - Does ByteBuffer#hasArray() return true? - If it doesn't you can probably use ByteBuffer#slice() combined with ByteBuffer#position() and ByteBuffer#limit() https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:190: sig.update(toUInt32BigEndian(filename.length)); Can you document why you are saving the length? https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:209: buffer.order(BIG_ENDIAN); I think that BIG_ENDIAN is the default for ByteBuffer. (Just double checking that you are calling ByteBuffer#order() for clarity) https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:270: // Signature(4), CreatorVersion(2), ReaderVersion(2), Flags(2), Method(2) Nit: Method(2) -> CompressionMethod(2) https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:273: seek(curr); It would be useful to have a seekDelta() method which does mBuffer.position(mBuffer.position() + delta) You can do this instead of the reads on lines 279 - 281 https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; I would store the extra size and the comment size as a field in the Block object. This way you separate the reading of the zip central directory (and whether this is a valid zip file) from checking our custom requirements for the zip file https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:299: continue; This logic belongs in calculateHash() https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:317: seek(curr); seekDelta() would be useful here too https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:319: int extraFieldLength = read2(); If we care about checking the size of the extra data shouldn't we check that the extra data length is the same as the length specified in the central directory entry? https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:323: Collections.sort(mBlocks); Can you move the sorting of the blocks to computeHash()? https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:370: * Nit: No new line https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:377: /** Read `length` many bytes into a string */ 'length' -> {@link length} https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:387: /** Convert a hex string into bytes. */ Can you make your comment more specific. In particular, why is String#getBytes() not good enough? https://codereview.chromium.org/2772483002/diff/140001/testing/android/junit/... File testing/android/junit/java/src/org/chromium/testing/local/TestDir.java (right): https://codereview.chromium.org/2772483002/diff/140001/testing/android/junit/... testing/android/junit/java/src/org/chromium/testing/local/TestDir.java:12: public class TestDir { This change should be in a separate CL
Fixed the duplicate code, I think I forgot to "send" mail, however. https://codereview.chromium.org/2772483002/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:233: } On 2017/03/24 at 04:43:59, pkotwicz wrote: > This is a duplicate of lines 221 - 224? Good catch.
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think I've addressed most of the comments. Thanks for the review! https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:150: PackageManager.GET_SIGNATURES | PackageManager.GET_META_DATA); On 2017/03/26 at 01:37:04, pkotwicz wrote: > Is this call slower when called with PackageManager.Get_SIGNATURES (I am unsure) Doesn't seem like much, although I only tested twice in each case was 0.73ms and now about 0.76ms or about 0.03ms slower. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:166: logTime(now, "isCommentSignedWebApk"); On 2017/03/26 at 01:37:04, pkotwicz wrote: > I think that it is useful to have a histogram which logs how long WebappLauncherActivity#isValidWebApk() takes. > > I don't think that the time logs here are useful. They are only useful for our own testing (we can easily build a custom version of Chrome with the logging) > > - The timing logs are not helpful when trying to debug bugs reported by users > - The timing logs are not helpful in debugging a crash > - The timing logs make Chrome bigger > > I suggest removing the timing logs I've removed the timing logs. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { On 2017/03/26 at 01:37:04, pkotwicz wrote: > I don't think that this check is useful anymore. Isn't a comment signed WebAPK still valid if the user decides to sign it many many times? If they have no signatures, that's bad. If they have more than 2 signatures they are probably doing something wrong as well. I've update the code to allow a limit of 5 META-INF files. Basically, dual signing plus MANIFEST.MF and return an error if it exceeds that. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:200: private static boolean foundV1Signature(PackageInfo packageInfo) { On 2017/03/26 at 01:37:04, pkotwicz wrote: > You can probably merge isV1WebApk() and foundV1Signature() > > verifyV1WebApk() perhaps Done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:284: } On 2017/03/26 at 01:37:04, pkotwicz wrote: > Is there a reason that we are copying the array? > > My understanding is that we are copying the array because Java does not offer const references the way that C++ does. I don't think that we need to modify the array because this class is not mutating the array The original code has Arrays.copyOf() for sExpectedSignature, I just copied what was done before. I'll change it and test it. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:298: } catch (InvalidKeySpecException e) { On 2017/03/26 at 01:37:04, pkotwicz wrote: > Nit: You can probably catch the generic Exception instead and have a single catch() statement. > > You don't need to do any logging here because you already do logging in the caller of getCommentSignedPublicKey() done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:28: public class WebApkVerifySignature { On 2017/03/26 at 01:37:04, pkotwicz wrote: > Can you split this class into two. One class which does the generic ZIP file reading (reading the central directory) and one which does the verification and signature computation. > > ZipCentralDirectoryReader perhaps with two methods > > String ZipCentralDirectoryReader#getFileComment() > ArrayList<ZipCentralDirectoryReader.Block> ZipCentralDirectoryReader#readEntries() I don't think it's that useful and am concerned someone might take the code to be a 'general' zip handling class, when it really is only useful for this one thing. I can still split if if you feel strongly about it. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:51: Pattern.compile("webapk:\\s*(\\d[^:]+:\\s*)?([a-fA-F0-9]*)"); On 2017/03/26 at 01:37:05, pkotwicz wrote: > Nit: webapk -> chrome-webapk I'm not sure if the "chrome-" part really makes sense, we may need to change this in the WAM server in the future, especially for x-browser. I think for now we should just search for "webapk:". > > Is there a place which describes the format of the signature? In particular what is the purpose of the characters before the second ':' is. It looks like WebApkVerifySignature.java ignores those characters You are right that I haven't updated our documentation (todo). The first number is the "algorithm version" and the second set of digits is the "key id". This was suggested by the security folks. Internally, b/34620500 is the bug for the crypto id. > > It looks like this code assumes that the signature is at the end of the file comment. If this is the case you can probably allow the signature to have unicode values between 0-255 and simplify hexToBytes() Actually, the code does allow for the signature to be anywhere in the comment. I prefer to use hex as some byte sequences aren't valid UTF-8 and many zip utilites don't support non-ascii characters very well. > > We should also document that we assume that the signature is at the end of the file comment There isn't really an assumption about where the signature is, but I've updated the pattern to be more clear? and added a few more tests. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:186: byte[] bytes = new byte[block.mCompressedSize]; On 2017/03/26 at 01:37:05, pkotwicz wrote: > You might be allocating an array which is many kilobytes. > > - Should you call Signature#update() several times to avoid copying so much data? > > - Can you avoid the copy: > - Does ByteBuffer#hasArray() return true? > - If it doesn't you can probably use ByteBuffer#slice() combined with ByteBuffer#position() and ByteBuffer#limit() A good optimization, thanks. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:190: sig.update(toUInt32BigEndian(filename.length)); On 2017/03/26 at 01:37:04, pkotwicz wrote: > Can you document why you are saving the length? internally b/34716053 discussed the reasons why. Added comments. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:209: buffer.order(BIG_ENDIAN); On 2017/03/26 at 01:37:04, pkotwicz wrote: > I think that BIG_ENDIAN is the default for ByteBuffer. > > (Just double checking that you are calling ByteBuffer#order() for clarity) Added a comment. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:270: // Signature(4), CreatorVersion(2), ReaderVersion(2), Flags(2), Method(2) On 2017/03/26 at 01:37:05, pkotwicz wrote: > Nit: Method(2) -> CompressionMethod(2) done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:273: seek(curr); On 2017/03/26 at 01:37:04, pkotwicz wrote: > It would be useful to have a seekDelta() method which does > > mBuffer.position(mBuffer.position() + delta) > > You can do this instead of the reads on lines 279 - 281 Thanks! Done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; On 2017/03/26 at 01:37:05, pkotwicz wrote: > I would store the extra size and the comment size as a field in the Block object. > > This way you separate the reading of the zip central directory (and whether this is a valid zip file) from checking our custom requirements for the zip file I believe the reason I did this is that the the file comment size and extra size is often 0 here yet when decoding the block it not zero, ugh. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:299: continue; On 2017/03/26 at 01:37:04, pkotwicz wrote: > This logic belongs in calculateHash() agreed, done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:317: seek(curr); On 2017/03/26 at 01:37:04, pkotwicz wrote: > seekDelta() would be useful here too done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:319: int extraFieldLength = read2(); On 2017/03/26 at 01:37:05, pkotwicz wrote: > If we care about checking the size of the extra data shouldn't we check that the extra data length is the same as the length specified in the central directory entry? It often isn't. I think some zip implementations are lazy and just put zeros in the central directory. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:323: Collections.sort(mBlocks); On 2017/03/26 at 01:37:04, pkotwicz wrote: > Can you move the sorting of the blocks to computeHash()? Agreed, done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:370: * On 2017/03/26 at 01:37:05, pkotwicz wrote: > Nit: No new line done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:377: /** Read `length` many bytes into a string */ On 2017/03/26 at 01:37:05, pkotwicz wrote: > 'length' -> {@link length} Done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:387: /** Convert a hex string into bytes. */ On 2017/03/26 at 01:37:05, pkotwicz wrote: > Can you make your comment more specific. In particular, why is String#getBytes() not good enough? Added a comment.
Description was changed from ========== Commment signed webapks working and verified. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 ========== to ========== Commment signed webapks working with verification. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 ==========
scottkirkwood@chromium.org changed reviewers: + agrieve@chromium.org, yfriedman@chromium.org
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Some more comments. Not quite done the second round of review https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:28: public class WebApkVerifySignature { I will defer to dominickn@ on this point https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:51: Pattern.compile("webapk:\\s*(\\d[^:]+:\\s*)?([a-fA-F0-9]*)"); I'm not sure if the "chrome-" part really makes sense, we may need to change this in the WAM server in the future, especially for x-browser. I think for now we should just search for "webapk:". Ok, for the sake of future proofing, let's make this webapk: instead of chrome-webapk (Let's update the other comments in this file appropriately) You are right that I haven't updated our documentation (todo). The first number is the "algorithm version" and the second set of digits is the "key id". This was suggested by the security folks. Internally, b/34620500 is the bug for the crypto id. According to the bug, the format is chrome-webapk:<algorithm_version>:<key_id>:<bytes> The matcher seems to be for the format: chrome-webapk:<algorithm_version>:<bytes> There isn't really an assumption about where the signature is, but I've updated the pattern to be more clear? and added a few more tests. If there is a hex value after the signature, won't the matcher assume that the hex value is part of the signature too? Can we make the format requirement stricter: - disallow whitespace between "algorithm version" and "hash" - make "algorithm version" block non optional https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; Thank you for the explanation. I think that we should check the extra and comment length in either both the "central directory record" and the "local file header" or in neither https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:32: private static final long EOCD_SIG = 0x06054b50; We might as well use the constants from ZipFile instead of redefining EOCD_SIG, CD_SIG and LFH_SIG here https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:78: public enum Error { I think that we have a policy against using enums in Java code in Chromium. (At least we used to). Yaron should be able to confirm or deny. https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:177: | UnsupportedEncodingException e) { Maybe simplify this to catch (Exception) {} https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:195: throws SignatureException, IllegalArgumentException, UnsupportedEncodingException { You can simplify this to "throws Exception" https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:236: private byte[] toUInt32BigEndian(int value) { Perhaps a better name for this function would be: intToBigEndianBytes() This function does not convert to an unsigned int. I suggest removing the uint part from the function name because Java does not have unsigned ints https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:237: ByteBuffer buffer = ByteBuffer.allocate(4); Can we make this little endian for the sake of consistency? (I don't think the endianness matters as long as the encoder logic and the decoder logic on the client match) https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:310: curr = mBuffer.position() + extraLen + fileCommentLength; You can replace this with seekDelta() too and move the seek() on line 293 outside of the for() loop. It would be nice if we can get rid of the |curr| variable entirely https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:328: curr = block.mPosition; Maybe do this: int headerStart = block.mPosition; and then on line 342: block.mHeaderSize = (mBuffer.position() - headerStart) + fileNameLength + extraFieldLength; https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:381: * Nit: remove new line https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:415: * like binary strings. */ Nit: "*/" on new line
https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:150: PackageManager.GET_SIGNATURES | PackageManager.GET_META_DATA); That seems negligible https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { We should let the user sign their APK however many times they want. We should not burden developers with requirements which are not required by the Play Store. If the Play Store enables a user to upload a WebAPK with no signatures there is no reason that we shouldn't allow the user to launch the WebAPK
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:207: InvalidKeySpecException, NoSuchAlgorithmException { This method should not throw any exceptions https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:236: verified = v.verifySignature(sCommentSignedPublicKey); According to http://stackoverflow.com/questions/65035/does-finally-always-execute-in-java you can make |verified| local to the try {} block and call return from within the try {} statement. Maybe rename |verified| to |result|. I would expect |verified| to be a boolean. It is confusing that it is not a boolean Note that the default value for |verified| is WebApkVerifySignature.Error.OK. Hence, if an exception is thrown inside the try{} statement this function returns true https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:240: buf.clear(); Is this necessary? Based on the documentation Buffer#clear() does very little https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:271: * Nit: Remove the new line https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:275: throws InvalidKeySpecException, NoSuchAlgorithmException { You can simplify this to "throws Exception" https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:138: public Error read() throws IllegalArgumentException { Can you post this function in a try/catch statement{} that way this function does not need to throw an Exception https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:375: private void seek(int offset) throws IllegalArgumentException { I don't think that the throws is useful here. IllegalArgumentException is an unchecked exception (The compiler does not complain if you don't handle it) Every single method in this file can throw an Exception. For instance, ByteBuffer#getInt() can throw BufferUnderflowException. I suggest not marking any functions in this file as throwing an Exception explicitly and have a try/catch block in both read() and verifySignature() You can do any appropriate logging in the catch{} block
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { On 2017/03/28 at 04:41:34, pkotwicz wrote: > We should let the user sign their APK however many times they want. We should not burden developers with requirements which are not required by the Play Store. If the Play Store enables a user to upload a WebAPK with no signatures there is no reason that we shouldn't allow the user to launch the WebAPK You can't upload an APK with no signatures and I'm pretty sure it will also won't let you have two or more signatures. Additionally, there's a small concern that a hacker may try stuffing code in META-INF, thus the extra checks. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:284: } On 2017/03/27 at 20:27:56, ScottK wrote: > On 2017/03/26 at 01:37:04, pkotwicz wrote: > > Is there a reason that we are copying the array? > > > > My understanding is that we are copying the array because Java does not offer const references the way that C++ does. I don't think that we need to modify the array because this class is not mutating the array > > The original code has Arrays.copyOf() for sExpectedSignature, I just copied what was done before. > I'll change it and test it. The try bots didn't like this change. I think the reason for this original copying is to appease the try bots and because we can't call include WebApkHostSignature from chromium.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks again for the review. Hopefully, I didn't mess up the merge. https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:207: InvalidKeySpecException, NoSuchAlgorithmException { On 2017/03/28 at 18:35:41, pkotwicz wrote: > This method should not throw any exceptions done https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:236: verified = v.verifySignature(sCommentSignedPublicKey); On 2017/03/28 at 18:35:41, pkotwicz wrote: > According to http://stackoverflow.com/questions/65035/does-finally-always-execute-in-java > you can make |verified| local to the try {} block and call return from within the try {} statement. > > Maybe rename |verified| to |result|. I would expect |verified| to be a boolean. It is confusing that it is not a boolean > > Note that the default value for |verified| is WebApkVerifySignature.Error.OK. Hence, if an exception is thrown inside the try{} statement this function returns true Returning here. https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:240: buf.clear(); On 2017/03/28 at 18:35:41, pkotwicz wrote: > Is this necessary? Based on the documentation Buffer#clear() does very little Guess not, thought it might be a hint to the gc. https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:275: throws InvalidKeySpecException, NoSuchAlgorithmException { On 2017/03/28 at 18:35:41, pkotwicz wrote: > You can simplify this to "throws Exception" done https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:138: public Error read() throws IllegalArgumentException { On 2017/03/28 at 18:35:41, pkotwicz wrote: > Can you post this function in a try/catch statement{} that way this function does not need to throw an Exception done https://codereview.chromium.org/2772483002/diff/200001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:375: private void seek(int offset) throws IllegalArgumentException { On 2017/03/28 at 18:35:41, pkotwicz wrote: > I don't think that the throws is useful here. IllegalArgumentException is an unchecked exception (The compiler does not complain if you don't handle it) > > Every single method in this file can throw an Exception. For instance, ByteBuffer#getInt() can throw BufferUnderflowException. I suggest not marking any functions in this file as throwing an Exception explicitly and have a try/catch block in both read() and verifySignature() > > You can do any appropriate logging in the catch{} block I think I've done what you suggest. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Commment signed webapks working with verification. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 ========== to ========== Commment signed webapks working with verification. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 ==========
test runner lgtm /w nit https://codereview.chromium.org/2772483002/diff/260001/build/android/pylib/lo... File build/android/pylib/local/machine/local_machine_junit_test_run.py (right): https://codereview.chromium.org/2772483002/diff/260001/build/android/pylib/lo... build/android/pylib/local/machine/local_machine_junit_test_run.py:33: self._test_instance.suite) whoops, random whitespace change.
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Mostly nits There are still some comments from a previous code review that you have not addressed. I have annotated these with "Ping on this comment" https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { I think that the checks that you have for the amount of files in META-INF is sufficient. Please remove the check for the number of signatures https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:32: private static final long EOCD_SIG = 0x06054b50; Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:177: | UnsupportedEncodingException e) { Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:236: private byte[] toUInt32BigEndian(int value) { Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:237: ByteBuffer buffer = ByteBuffer.allocate(4); Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:310: curr = mBuffer.position() + extraLen + fileCommentLength; Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:328: curr = block.mPosition; Ping on this comment https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:415: * like binary strings. */ Ping on this comment https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:80: * Returns whether installing WebAPKs from Google Play is possible. If {@link Keep {@link} on the same line https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:84: public static boolean canUseGooglePlayToInstallWebApk() { Why this change? https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (left): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:86: /** Why this change? https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:158: * ResolveInfos start with {@link WebApkConstants.WEBAPK_PACKAGE_PREFIX}. You probably need to update this comment https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:202: /** Tests {@link WebApkValidator.isValidWebApk} for comment signed webapks. */ Can you make the comment more descriptive. The comment for testIsValidWebApkCommentSigned() and testIsValidWebApkCommentSignedFailures() should not be identical https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:209: for (String filename : filenames) { Maybe you should call mPackageManager.removePackage() before adding a package with the exact same name. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:238: private static String testFilename(String fname) { Nit: testFilename() -> testFilePath() https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:250: private static PackageInfo newPackageInfo(String packageName, Signature[] signatures) { Nit: I would pass the sourceDir to this function (and pass null in the appropriate callers) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:56: byte[] bytes = hexStringToByteArray("decafbad"); Can you initialize bytes with the result of hexStringToByteArray() and remove hexStringToByteArray() https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:73: public static String testFilename(String fname) { testFilename() -> testFilePath() https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { Are the testVerifyFiles() and the testBadVerifyFiles() tests necessary? They look like duplicates of the tests in WebApkValidatorTest.java https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:135: * Nit: Remove the new line https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:178: return (startUrl == null || startUrl.isEmpty()); You can use TextUtils.isEmpty() to do this https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:216: MappedByteBuffer buf = null; Can |buf| be moved inside to try {} statement? https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:224: @WebApkVerifySignature.Error I haven't seen annotations on enum ints anywhere but in function parameters. Remove the annotation? https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:236: } catch (IllegalArgumentException | IOException e) { You can just check the generic exception here - catch (Exception e) {}. For instance, you are not catching FileNotFoundException which can be thrown when constructing the RandomAccessFile. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:258: * Nit: Remove new line https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:260: * @param v2PublicKeyBytes Remove @params if you are not documenting them https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:41: /** Max end-of-central-directory size, including variable length file comment.. */ How about: "Minimum end-of-central-directory size." The constant is for the minimum not the maximum https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:57: * character. Cool! The signature checking code has a "singing function". anywher -> anywhere "with a separator that doesn't look like a hex character." -> "non hex character." https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:60: Pattern.compile("webapk:(\\d+):([a-fA-F0-9]+)"); Can you remove the brackets around \\d+ (You don't need to extract the "key id" yet) Thank you. The pattern is a lot simpler now https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:68: /** The memory buffer we are going to read the zip from */ Nit: "." at the end of the sentence (Here and in other comments in this file) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:77: /** The zip archive comment as a UTF-8 strings */ "strings" -> "string." https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:92: public static final int ERROR_TOO_MANY_META_INF_FILES = 6; Nit: public static constants should be declared first https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:100: /** Block is the offset and size of a compressed zip entry. */ How about: Block contains meta data about a zip entry. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:109: /** added for Comparable, sort lexicographically. */ Nit: added -> Added https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:119: } Nit: New line https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:128: int mCompressedSize; Member variables should come before functions. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:131: /** CTOR simply 'connects' to buffer passed. */ CTOR -> Constructor https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:141: * Nit: remove new line (here and in other function comments) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:146: @Error I haven't seen other places which annotate ints which are enums in this way. (I have seen annotations in method parameters but nowhere else) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:149: Log.d(TAG, "Missing EOCD Signature"); Log is unnecessary because calling function will log error code https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:158: Log.d(TAG, "Error reading directory"); Log is unnecessary because calling function will log error code https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:173: * @throws SignatureException for various reasons. This function should not throw an exception. If an exception is thrown, the function should return an error code https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:190: | UnsupportedEncodingException e) { catch (Exception e) {} instead for the sake of sanity https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:202: * calculateHash goes through each file listed in blocks and calculates the SHA-256 calculateHash -> calculateHash() https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:208: throws SignatureException, IllegalArgumentException, UnsupportedEncodingException { This function should throw Exception only for the sake of sanity https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:227: // violation. Remove 'as well' https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:228: filename = block.mFilename.getBytes("UTF-8"); - Is it worth storing mFilename as a String? Can it be stored as an array of bytes instead? You can use Arrays#hashCode() to get the hash of the byte array in Block#hashCode() readString() would become byte[] readBytes(int numBytes) - You should be consistent. If you are passing the charset to String#getBytes() you should also pass the charset to String#String() in readString(). - Might as well move the declaration of the |filename| variable to here. byte[] filename = ... https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:236: slice = mBuffer.slice(); Might as well move the declaration of the |slice| variable to here. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:258: * "chrome-webapk:0000:<hexvalues>" comment followed by hex values. Currently we ignore the key How about: "The signature should have the format webapk:0000:<hexvalues>. Currently ..." https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:259: * id which is always "0000". Nit: Add quotes around "key id" https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:269: if (s.length() == 0) { Can you remove this conditional? - It should be impossible for |s| to be empty. - If |s| is empty, verifySignature() will fail anyways https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:276: * Reads the End of Central Directory Records, returns false if it can't find it. Records -> Record Remove the part of the comment with "returns false if ...". You are already documenting this in the @return part of the comment https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:288: read4(); // Size of central directory seekDelta(4) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:298: * purposes. How about: Reads the central directory and populates {@link mBlocks} with data about each entry. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:373: int offset = mBuffer.limit() - MIN_EOCD_SIZE; Nit: You need to handle |offset| being smaller than 0 (Though I guess you would get an exception when you do the seek.) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:375: for (int count = 0; count < MAX_HEADER_SIZE; count++) { Can you do this: int minSearchOffset = Math.max(0, offset - MAX_HEADER_SIZE); for (; offset >= minSearchOffset; offset--) { seek(offset); ... } This enables merging the seek on line 355 and 365 https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:438: Log.d(TAG, "Got an odd number of hex nibbles."); Can we remove this log message? We are already logging ERROR_SIGNATURE_NOT_FOUND https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:441: if (len == 0) { Can we remove this conditional? (For the reasons mentioned in parseCommentSignature())
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the detail review. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { On 2017/03/31 at 03:59:23, pkotwicz wrote: > I think that the checks that you have for the amount of files in META-INF is sufficient. Please remove the check for the number of signatures So the META-INF checks are only checked if it's a comment signed webapk. I think we may still want to check the number of signatures for v1 signed webapks, what do you think? https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:51: Pattern.compile("webapk:\\s*(\\d[^:]+:\\s*)?([a-fA-F0-9]*)"); On 2017/03/28 at 04:31:03, pkotwicz wrote: > I'm not sure if the "chrome-" part really makes sense, we may need to change > this in the WAM server in the future, especially for x-browser. I think for now > we should just search for "webapk:". > > Ok, for the sake of future proofing, let's make this webapk: instead of chrome-webapk (Let's update the other comments in this file appropriately) > > You are right that I haven't updated our documentation (todo). The first number > is the "algorithm version" and the second set of digits is the "key id". This > was suggested by the security folks. > Internally, b/34620500 is the bug for the crypto id. > > According to the bug, the format is > chrome-webapk:<algorithm_version>:<key_id>:<bytes> > The matcher seems to be for the format: > chrome-webapk:<algorithm_version>:<bytes> > > There isn't really an assumption about where the signature is, but I've updated > the pattern to be more clear? and added a few more tests. > > If there is a hex value after the signature, won't the matcher assume that the hex value is part of the signature too? > > Can we make the format requirement stricter: > - disallow whitespace between "algorithm version" and "hash" > - make "algorithm version" block non optional I've simplified the regex and am not forcing the key id, which is a number. The bug did mention having <algorithm version>:<key id>:<bytes>, but since the algorithm version was a 1 digit number we just put it altogether as one big four digit identifier. Since we control what we put in the comment it think it's ok to allow the bytes to trail off and assume there will be a separator character (added some more documentation). The chrome- prefix might make sense if we have a different private key used for chrome vs other browsers. However, with the desire supporting x-browsers I agree with you we should make it general and not have the chrome- prefix. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; On 2017/03/28 at 04:31:03, pkotwicz wrote: > Thank you for the explanation. I think that we should check the extra and comment length in either both the "central directory record" and the "local file header" or in neither Your are correct, I should check also in the local file header in case they lied earlier - done. https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:284: curr = mBuffer.position() + extraLen + fileCommentLength; On 2017/03/31 at 03:59:23, pkotwicz wrote: > Ping on this comment I fixed this earlier, guess I didn't publish in time? https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:32: private static final long EOCD_SIG = 0x06054b50; On 2017/03/28 at 04:31:03, pkotwicz wrote: > We might as well use the constants from ZipFile instead of redefining EOCD_SIG, CD_SIG and LFH_SIG here I'd rather not have a dependency, plus these constants won't change any time soon. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:80: * Returns whether installing WebAPKs from Google Play is possible. If {@link On 2017/03/31 at 03:59:24, pkotwicz wrote: > Keep {@link} on the same line done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:84: public static boolean canUseGooglePlayToInstallWebApk() { On 2017/03/31 at 03:59:24, pkotwicz wrote: > Why this change? I don't understand this change, unless it was a bad merge on my part? Reverted. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (left): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:86: /** On 2017/03/31 at 03:59:24, pkotwicz wrote: > Why this change? Google's vim auto formatter, apparently - reverted. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:158: * ResolveInfos start with {@link WebApkConstants.WEBAPK_PACKAGE_PREFIX}. On 2017/03/31 03:59:24, pkotwicz wrote: > You probably need to update this comment Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:202: /** Tests {@link WebApkValidator.isValidWebApk} for comment signed webapks. */ On 2017/03/31 at 03:59:24, pkotwicz wrote: > Can you make the comment more descriptive. The comment for testIsValidWebApkCommentSigned() and testIsValidWebApkCommentSignedFailures() should not be identical Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:209: for (String filename : filenames) { On 2017/03/31 at 03:59:24, pkotwicz wrote: > Maybe you should call mPackageManager.removePackage() before adding a package with the exact same name. Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:238: private static String testFilename(String fname) { On 2017/03/31 at 03:59:24, pkotwicz wrote: > Nit: testFilename() -> testFilePath() Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:250: private static PackageInfo newPackageInfo(String packageName, Signature[] signatures) { On 2017/03/31 03:59:24, pkotwicz wrote: > Nit: I would pass the sourceDir to this function (and pass null in the > appropriate callers) Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:56: byte[] bytes = hexStringToByteArray("decafbad"); On 2017/03/31 at 03:59:24, pkotwicz wrote: > Can you initialize bytes with the result of hexStringToByteArray() and remove hexStringToByteArray() Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:73: public static String testFilename(String fname) { On 2017/03/31 at 03:59:24, pkotwicz wrote: > testFilename() -> testFilePath() Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { On 2017/03/31 at 03:59:24, pkotwicz wrote: > Are the testVerifyFiles() and the testBadVerifyFiles() tests necessary? They look like duplicates of the tests in WebApkValidatorTest.java They increase our code coverage by testing things at a higher level. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:135: * On 2017/03/31 at 03:59:24, pkotwicz wrote: > Nit: Remove the new line done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:178: return (startUrl == null || startUrl.isEmpty()); On 2017/03/31 at 03:59:24, pkotwicz wrote:chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java > You can use TextUtils.isEmpty() to do this Thanks, I vaguely remember there was a tool for that. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:216: MappedByteBuffer buf = null; On 2017/03/31 at 03:59:24, pkotwicz wrote: > Can |buf| be moved inside to try {} statement? Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:224: @WebApkVerifySignature.Error On 2017/03/31 at 03:59:24, pkotwicz wrote: > I haven't seen annotations on enum ints anywhere but in function parameters. Remove the annotation? sure. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:236: } catch (IllegalArgumentException | IOException e) { On 2017/03/31 at 03:59:24, pkotwicz wrote: > You can just check the generic exception here - catch (Exception e) {}. For instance, you are not catching FileNotFoundException which can be thrown when constructing the RandomAccessFile. Thanks! https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:258: * On 2017/03/31 at 03:59:24, pkotwicz wrote: > Nit: Remove new line Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:260: * @param v2PublicKeyBytes On 2017/03/31 at 03:59:24, pkotwicz wrote: > Remove @params if you are not documenting them Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:41: /** Max end-of-central-directory size, including variable length file comment.. */ On 2017/03/31 at 03:59:25, pkotwicz wrote: > How about: "Minimum end-of-central-directory size." > > The constant is for the minimum not the maximum Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:57: * character. On 2017/03/31 at 03:59:26, pkotwicz wrote: > Cool! The signature checking code has a "singing function". > > anywher -> anywhere > > "with a separator that doesn't look like a hex character." -> "non hex character." Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:68: /** The memory buffer we are going to read the zip from */ On 2017/03/31 at 03:59:25, pkotwicz wrote: > Nit: "." at the end of the sentence (Here and in other comments in this file) Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:68: /** The memory buffer we are going to read the zip from */ On 2017/03/31 at 03:59:25, pkotwicz wrote: > Nit: "." at the end of the sentence (Here and in other comments in this file) Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:77: /** The zip archive comment as a UTF-8 strings */ On 2017/03/31 at 03:59:25, pkotwicz wrote: > "strings" -> "string." Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:92: public static final int ERROR_TOO_MANY_META_INF_FILES = 6; On 2017/03/31 at 03:59:24, pkotwicz wrote: > Nit: public static constants should be declared first Moved to the top. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:100: /** Block is the offset and size of a compressed zip entry. */ On 2017/03/31 at 03:59:26, pkotwicz wrote: > How about: Block contains meta data about a zip entry. Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:109: /** added for Comparable, sort lexicographically. */ On 2017/03/31 at 03:59:25, pkotwicz wrote: > Nit: added -> Added Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:119: } On 2017/03/31 at 03:59:24, pkotwicz wrote: > Nit: New line Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:128: int mCompressedSize; On 2017/03/31 at 03:59:26, pkotwicz wrote: > Member variables should come before functions. Getting confused with C++ :-) https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:131: /** CTOR simply 'connects' to buffer passed. */ On 2017/03/31 at 03:59:26, pkotwicz wrote: > CTOR -> Constructor Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:141: * On 2017/03/31 at 03:59:26, pkotwicz wrote: > Nit: remove new line (here and in other function comments) Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:146: @Error On 2017/03/31 at 03:59:25, pkotwicz wrote: > I haven't seen other places which annotate ints which are enums in this way. (I have seen annotations in method parameters but nowhere else) Removed. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:149: Log.d(TAG, "Missing EOCD Signature"); On 2017/03/31 at 03:59:25, pkotwicz wrote: > Log is unnecessary because calling function will log error code Removed. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:158: Log.d(TAG, "Error reading directory"); On 2017/03/31 at 03:59:26, pkotwicz wrote: > Log is unnecessary because calling function will log error code Removed. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:173: * @throws SignatureException for various reasons. On 2017/03/31 at 03:59:25, pkotwicz wrote: > This function should not throw an exception. If an exception is thrown, the function should return an error code Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:190: | UnsupportedEncodingException e) { On 2017/03/31 at 03:59:25, pkotwicz wrote: > catch (Exception e) {} instead for the sake of sanity Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:202: * calculateHash goes through each file listed in blocks and calculates the SHA-256 On 2017/03/31 at 03:59:24, pkotwicz wrote: > calculateHash -> calculateHash() Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:208: throws SignatureException, IllegalArgumentException, UnsupportedEncodingException { On 2017/03/31 at 03:59:25, pkotwicz wrote: > This function should throw Exception only for the sake of sanity Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:227: // violation. On 2017/03/31 at 03:59:25, pkotwicz wrote: > Remove 'as well' Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:228: filename = block.mFilename.getBytes("UTF-8"); On 2017/03/31 at 03:59:25, pkotwicz wrote: > - Is it worth storing mFilename as a String? Can it be stored as an array of bytes instead? > You can use Arrays#hashCode() to get the hash of the byte array in Block#hashCode() > readString() would become byte[] readBytes(int numBytes) > - You should be consistent. If you are passing the charset to String#getBytes() you should also pass the charset to String#String() in readString(). > > - Might as well move the declaration of the |filename| variable to here. > byte[] filename = ... It sounds like a good idea but ended up having some repercussions that made it not worthwhile, I think: * There's no compareTo() for byte arrays that I could find. * There's no startsWith(), although the binary search could work. * For the regular expression search for webapk:0000: etc. we'd have to convert it to a String. Instead I just used getBytes() instead of UTF-8 specifically. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:236: slice = mBuffer.slice(); On 2017/03/31 at 03:59:25, pkotwicz wrote: > Might as well move the declaration of the |slice| variable to here. Moved. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:258: * "chrome-webapk:0000:<hexvalues>" comment followed by hex values. Currently we ignore the key On 2017/03/31 at 03:59:25, pkotwicz wrote: > How about: "The signature should have the format webapk:0000:<hexvalues>. Currently ..." Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:259: * id which is always "0000". On 2017/03/31 at 03:59:25, pkotwicz wrote: > Nit: Add quotes around "key id" Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:269: if (s.length() == 0) { On 2017/03/31 at 03:59:24, pkotwicz wrote: > Can you remove this conditional? > > - It should be impossible for |s| to be empty. > - If |s| is empty, verifySignature() will fail anyways Good point. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:276: * Reads the End of Central Directory Records, returns false if it can't find it. On 2017/03/31 at 03:59:25, pkotwicz wrote: > Records -> Record > > Remove the part of the comment with "returns false if ...". You are already documenting this in the @return part of the comment Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:288: read4(); // Size of central directory On 2017/03/31 at 03:59:25, pkotwicz wrote: > seekDelta(4) Oops missed one,thanks. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:298: * purposes. On 2017/03/31 at 03:59:25, pkotwicz wrote: > How about: Reads the central directory and populates {@link mBlocks} with data about each entry. Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:373: int offset = mBuffer.limit() - MIN_EOCD_SIZE; On 2017/03/31 at 03:59:25, pkotwicz wrote: > Nit: You need to handle |offset| being smaller than 0 (Though I guess you would > get an exception when you do the seek.) My unit tests do cover this, however being explicit it probably good. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:375: for (int count = 0; count < MAX_HEADER_SIZE; count++) { On 2017/03/31 at 03:59:26, pkotwicz wrote: > Can you do this: > > int minSearchOffset = Math.max(0, offset - MAX_HEADER_SIZE); > for (; offset >= minSearchOffset; offset--) { > seek(offset); > ... > } > > This enables merging the seek on line 355 and 365 Done, much cleaner. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:438: Log.d(TAG, "Got an odd number of hex nibbles."); On 2017/03/31 at 03:59:25, pkotwicz wrote: > Can we remove this log message? > > We are already logging ERROR_SIGNATURE_NOT_FOUND Done. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:441: if (len == 0) { On 2017/03/31 at 03:59:25, pkotwicz wrote: > Can we remove this conditional? (For the reasons mentioned in parseCommentSignature()) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yfriedman@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek for security comments on signing mechanism - I believe this was already security reviewed but here's the CL adding it. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java:69: static final byte[] PUBLIC_KEY = new byte[] {48, 89, 48, 19, 6, 7, 42, -122, 72, -50, 61, 2, 1, Nit: comment https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:246: private static String testFilePath(String fname) { Nit: java, unlike go likes verbose, non-shortened names :) https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:161: return verifyCommentSignedWebApk(packageInfo); can we guard this behind an about:flag. Sam pointed out that it would be nice to have this defaulted to off since it would enable us to offer developer preview/publicly creation service while fleshing out launch-blocking work. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:216: file = new RandomAccessFile(packageFilename, "r"); this is potentially doing file io on the main thread so if we need to do it, you need to add a StrictMode exemptoin (e.g. https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/UiUt...) Along with other Strictmode violations, we should add a histogram to monitor this. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:230: Log.d(TAG, "File " + packageFilename + ": " + result); remove log statement https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:254: * @param expectedSignature Old WebAPK RSA signature. Please avoid using "old" and "new". They are two different signing schemes but V1 is unlikely to go away
Looks like there ares still some comments that you missed. I highlighted the missed comments with giant 🚨 to make them more visible https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/140001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:182: || packageInfo.signatures.length > 2) { I think that is reasonable. You have a separate check for the number of signatures in isV1WebApk() https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:32: private static final long EOCD_SIG = 0x06054b50; OK https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:236: private byte[] toUInt32BigEndian(int value) { 🚨🚨 Ping on this comment again 🚨🚨 https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:237: ByteBuffer buffer = ByteBuffer.allocate(4); 🚨🚨 Ping on this comment again 🚨🚨 https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:310: curr = mBuffer.position() + extraLen + fileCommentLength; 🚨🚨 Ping on this comment again 🚨🚨 https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:328: curr = block.mPosition; 🚨🚨 Ping on this comment again 🚨🚨 https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:415: * like binary strings. */ 🚨🚨 Ping on this comment again 🚨🚨 https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { I don't think that it increases code coverage. What code in WebApkVerifySignature does this test test that isn't tested by WebApkValidatorTest? https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/BUILD.gn (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/BUILD.gn:21: "//third_party/android_tools:android_support_annotations_java", You need to update chrome/android/webapk/shell_apk/DEPS You can check that the DEPS are correct by running: buildtools/checkdeps/checkdeps.py chrome/android/webapk/
Looks like there are still some comments that you missed. I highlighted the missed comments with giant 🚨 to make them more visible
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/04 18:19:43, pkotwicz wrote: > Looks like there are still some comments that you missed. I highlighted the > missed comments with giant 🚨 to make them more visible I've gone back to the old codereview version which seems to be clearer.
Thanks for the review, ptal. https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:310: curr = mBuffer.position() + extraLen + fileCommentLength; On 2017/04/04 18:19:36, pkotwicz wrote: > 🚨🚨 Ping on this comment again 🚨🚨 Done. https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:328: curr = block.mPosition; On 2017/04/04 18:19:36, pkotwicz wrote: > 🚨🚨 Ping on this comment again 🚨🚨 More or less did the same thing, but avoided the temporary. https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:381: * On 2017/03/28 04:31:03, pkotwicz wrote: > Nit: remove new line Done. https://codereview.chromium.org/2772483002/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:415: * like binary strings. */ On 2017/04/04 18:19:36, pkotwicz wrote: > 🚨🚨 Ping on this comment again 🚨🚨 Done. https://codereview.chromium.org/2772483002/diff/260001/build/android/pylib/lo... File build/android/pylib/local/machine/local_machine_junit_test_run.py (right): https://codereview.chromium.org/2772483002/diff/260001/build/android/pylib/lo... build/android/pylib/local/machine/local_machine_junit_test_run.py:33: self._test_instance.suite) On 2017/03/30 17:09:54, agrieve wrote: > whoops, random whitespace change. Fixed, but I've also split this to another cr/2788733002. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { On 2017/04/04 18:19:36, pkotwicz wrote: > I don't think that it increases code coverage. What code in > WebApkVerifySignature does this test test that isn't tested by > WebApkValidatorTest? Sorry, my mistake, it's the other way around - WebApkValidatorTest is testing more than this, so *it* is doing a bit more coverage. However, I think this test here is still useful for another reasons: * It should be slightly easier to debug things when broken as it doesn't get WebApkValidator involved. * There may be some additional test files I may add later that may give different error messages here and not in WebApkValidator. * If someone removes WebApkValidatorTest, then I won't have good coverage on this class. It's only test code. If the concern is wasting test CPU cycles, then that's another discussion. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHostSignature.java:69: static final byte[] PUBLIC_KEY = new byte[] {48, 89, 48, 19, 6, 7, 42, -122, 72, -50, 61, 2, 1, On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > Nit: comment Done. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/BUILD.gn (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/BUILD.gn:21: "//third_party/android_tools:android_support_annotations_java", On 2017/04/04 18:19:36, pkotwicz wrote: > You need to update chrome/android/webapk/shell_apk/DEPS > > You can check that the DEPS are correct by running: > buildtools/checkdeps/checkdeps.py chrome/android/webapk/ Done. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:246: private static String testFilePath(String fname) { On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > Nit: java, unlike go likes verbose, non-shortened names :) Then I would have written "f" :-) Fixed. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:161: return verifyCommentSignedWebApk(packageInfo); On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > can we guard this behind an about:flag. Sam pointed out that it would be nice to > have this defaulted to off since it would enable us to offer developer > preview/publicly creation service while fleshing out launch-blocking work. I don't think we need a flag as the existing of a "webapk:" comment is, in effect, the flag. I think that adding a flag will just add extra steps for testing/testers. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:216: file = new RandomAccessFile(packageFilename, "r"); On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > this is potentially doing file io on the main thread so if we need to do it, you > need to add a StrictMode exemptoin (e.g. > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/UiUt...) > > Along with other Strictmode violations, we should add a histogram to monitor > this. I've added a file StrictMode allowThreadDiskReads. I was unable to add the histogram because chrome/android/webapk/DEPS doesn't allow anything from //base. Maybe in another CL? https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:230: Log.d(TAG, "File " + packageFilename + ": " + result); On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > remove log statement I think in the short term this might be useful to make sure it's getting here and why it failed to verify (as the result is swallowed), what do you think? Added a TODO. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:254: * @param expectedSignature Old WebAPK RSA signature. On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > Please avoid using "old" and "new". They are two different signing schemes but > V1 is unlikely to go away I think that v1 *is* likely to go away, as we start creating WebAPKs on the WAM server that are singly signed (with the comment as the dual-signing). Eventually, we can have clank force a re-minting of WebApks that use the v1 signing scheme.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
L-G-T-M on my part once you address these comments https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { The main concern is preserving the hackability of Chrome. It takes more time to change two identical tests than to change a single test * It should be slightly easier to debug things when broken as it doesn't get WebApkValidator involved. - I don't think that debugging is any easier with WebApkVerifySignatureTest * There may be some additional test files I may add later that may give different error messages here and not in WebApkValidator. - We can add these tests back in when and if we add these extra error messages * If someone removes WebApkValidatorTest, then I won't have good coverage on this class. - In Chrome, it is common for tests not to be deleted even when they should be. I haven't heard of tests getting wrongfully deleted https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:204: * Tests {@link WebApkValidator.isValidWebApk} for valid comment signed webapks. Nit: WebApkValidator.isValidWebApk -> WebApkValidator#isValidWebApk() https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:223: * These WebAPKs were modified to fail in specific ways. Nits: - WebApkValidator.isValidWebApk -> WebApkValidator#isValidWebApk() - failing -> invalid https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:245: // Get the full test filename. Nit: "filename" -> "file path" https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:144: + "missing call to WebApkValidator.initWithBrowserHostSignature"); Shouldn't we return false here? https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:181: private static boolean verifyV1WebApk(PackageInfo packageInfo, String webappPackageName) { Nit: You can move the check for whether packageInfo.signatures is non null to here and delete the check for the number of signatures from isNotWebApkQuick() https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:55: private static final int MAX_HEADER_SIZE = 64 * 8192; Nit: This is likely more readable as: 512 * 1024 Please also mention the units. Is this the maximum size in bytes? My understanding is that the maximum header side is 2Mb https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:173: @Error Please remove the annotation https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:186: public String comment() { This method looks unused. 💣 it https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:232: private byte[] toUInt32BigEndian(int value) { Perhaps a better name for this function would be: intToBigEndianBytes() This function does not convert to an unsigned int. I suggest removing the uint part from the function name because Java does not have unsigned ints https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. Can we make this little endian for the sake of consistency? (I don't think the endianness matters as long as the encoder logic on the server and the decoder logic on the client match) https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:302: "Extra field too large for file %s: %d bytes", filename, extraLen)); Nit: You can remove this log. The log with the error code is printed in verifyCommentSignedWebApk() https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:308: fileCommentLength)); Nit: You can remove this log. The log with the error code is printed in verifyCommentSignedWebApk() https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:322: // ReaderVersion(2), Flags(2), Method(2), Nit: Method(2) -> CompressionMethod(2) https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:329: Log.w(TAG, String.format("Extra field too large: %d bytes", extraFieldLength)); Nit: You can remove this log. The log with the error code is printed in verifyCommentSignedWebApk() https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:350: } Nit: We can remove this if() statement now
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks again. https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/280001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:78: public void testVerifyFiles() throws Exception { On 2017/04/05 02:54:28, pkotwicz wrote: > The main concern is preserving the hackability of Chrome. It takes more time to > change two identical tests than to change a single test > > * It should be slightly easier to debug things when broken as it doesn't get > WebApkValidator involved. > - I don't think that debugging is any easier with WebApkVerifySignatureTest > * There may be some additional test files I may add later that may give > different error messages here and not in WebApkValidator. > - We can add these tests back in when and if we add these extra error messages > * If someone removes WebApkValidatorTest, then I won't have good coverage on > this class. > - In Chrome, it is common for tests not to be deleted even when they should be. > I haven't heard of tests getting wrongfully deleted Ok, removing this. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:204: * Tests {@link WebApkValidator.isValidWebApk} for valid comment signed webapks. On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: > WebApkValidator.isValidWebApk -> WebApkValidator#isValidWebApk() Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:223: * These WebAPKs were modified to fail in specific ways. On 2017/04/05 02:54:28, pkotwicz wrote: > Nits: > - WebApkValidator.isValidWebApk -> WebApkValidator#isValidWebApk() > - failing -> invalid Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:245: // Get the full test filename. On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: "filename" -> "file path" Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:144: + "missing call to WebApkValidator.initWithBrowserHostSignature"); On 2017/04/05 02:54:28, pkotwicz wrote: > Shouldn't we return false here? Guess I thought wtf would kill the app. Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:181: private static boolean verifyV1WebApk(PackageInfo packageInfo, String webappPackageName) { On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: You can move the check for whether packageInfo.signatures is non null to > here > > and delete the check for the number of signatures from isNotWebApkQuick() Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:55: private static final int MAX_HEADER_SIZE = 64 * 8192; On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: This is likely more readable as: > 512 * 1024 > > Please also mention the units. Is this the maximum size in bytes? > > My understanding is that the maximum header side is 2Mb Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:173: @Error On 2017/04/05 02:54:28, pkotwicz wrote: > Please remove the annotation Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:186: public String comment() { On 2017/04/05 02:54:28, pkotwicz wrote: > This method looks unused. 💣 it Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:232: private byte[] toUInt32BigEndian(int value) { On 2017/04/05 02:54:28, pkotwicz wrote: > Perhaps a better name for this function would be: > intToBigEndianBytes() > > This function does not convert to an unsigned int. I suggest removing the uint > part from the function name because Java does not have unsigned ints Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. On 2017/04/05 02:54:29, pkotwicz wrote: > Can we make this little endian for the sake of consistency? (I don't think the > endianness matters as long as the encoder logic on the server and the decoder > logic on the > client match) It matches the documentation and what the server does. It's a bit too much work to be worthwhile now. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:302: "Extra field too large for file %s: %d bytes", filename, extraLen)); On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: You can remove this log. The log with the error code is printed in > verifyCommentSignedWebApk() Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:308: fileCommentLength)); On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: You can remove this log. The log with the error code is printed in > verifyCommentSignedWebApk() Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:322: // ReaderVersion(2), Flags(2), Method(2), On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: Method(2) -> CompressionMethod(2) Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:329: Log.w(TAG, String.format("Extra field too large: %d bytes", extraFieldLength)); On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: You can remove this log. The log with the error code is printed in > verifyCommentSignedWebApk() Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:350: } On 2017/04/05 02:54:28, pkotwicz wrote: > Nit: We can remove this if() statement now Done.
https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:161: return verifyCommentSignedWebApk(packageInfo); On 2017/04/04 21:03:03, ScottK wrote: > On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > > can we guard this behind an about:flag. Sam pointed out that it would be nice > to > > have this defaulted to off since it would enable us to offer developer > > preview/publicly creation service while fleshing out launch-blocking work. > > I don't think we need a flag as the existing of a "webapk:" comment is, in > effect, the flag. > I think that adding a flag will just add extra steps for testing/testers. Explained off-thread why the flag might be advantageous in the short-term. Let's decide that so we can move forward here
https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:161: return verifyCommentSignedWebApk(packageInfo); On 2017/04/06 15:34:46, Yaron (limited availability) wrote: > On 2017/04/04 21:03:03, ScottK wrote: > > On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > > > can we guard this behind an about:flag. Sam pointed out that it would be > nice > > to > > > have this defaulted to off since it would enable us to offer developer > > > preview/publicly creation service while fleshing out launch-blocking work. > > > > I don't think we need a flag as the existing of a "webapk:" comment is, in > > effect, the flag. > > I think that adding a flag will just add extra steps for testing/testers. > > Explained off-thread why the flag might be advantageous in the short-term. Let's > decide that so we can move forward here Discussed with Sam off-thread and he proposed a good compromise: let's add a flag for just non-org.chromium.webapk webapks. Then we can decouple the signing logic from webapk policy
https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:245: // Get the full test filename. "filepath" -> "file path" https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:55: private static final int MAX_HEADER_SIZE = 64 * 8192; What are the units? https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. It matches which documentation? https://codereview.chromium.org/2772483002/diff/400001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/400001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:31: static PublicKey readPublicKey(String publicDER) throws Exception { This function is now unused?
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thank's, I've added a flag. https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2772483002/diff/360001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:161: return verifyCommentSignedWebApk(packageInfo); On 2017/04/06 15:51:50, Yaron (limited availability) wrote: > On 2017/04/06 15:34:46, Yaron (limited availability) wrote: > > On 2017/04/04 21:03:03, ScottK wrote: > > > On 2017/04/04 15:43:29, Yaron (limited availability) wrote: > > > > can we guard this behind an about:flag. Sam pointed out that it would be > > nice > > > to > > > > have this defaulted to off since it would enable us to offer developer > > > > preview/publicly creation service while fleshing out launch-blocking work. > > > > > > I don't think we need a flag as the existing of a "webapk:" comment is, in > > > effect, the flag. > > > I think that adding a flag will just add extra steps for testing/testers. > > > > Explained off-thread why the flag might be advantageous in the short-term. > Let's > > decide that so we can move forward here > > Discussed with Sam off-thread and he proposed a good compromise: let's add a > flag for just non-org.chromium.webapk webapks. Then we can decouple the signing > logic from webapk policy Added a new flag `any-webapk-package-name' https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:245: // Get the full test filename. On 2017/04/06 17:58:28, pkotwicz wrote: > "filepath" -> "file path" Done. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:55: private static final int MAX_HEADER_SIZE = 64 * 8192; On 2017/04/06 17:58:28, pkotwicz wrote: > What are the units? Done, I've also shortened to 64k which is more in line with other zip apps seek for plus our WebAPKs don't have many files so it should never get even close to that big. The reason I used the larger one is I did find a zip implementation that would search further for the end-of-central-directory, but I think it just makes it slower for files that aren't apks, so keeping it short. Also changing the name to be a little easier to understand it's purpose. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. On 2017/04/06 17:58:29, pkotwicz wrote: > It matches which documentation? go/webapk-comment-sig https://codereview.chromium.org/2772483002/diff/400001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java (right): https://codereview.chromium.org/2772483002/diff/400001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java:31: static PublicKey readPublicKey(String publicDER) throws Exception { On 2017/04/06 17:58:29, pkotwicz wrote: > This function is now unused? Good catch.
https://codereview.chromium.org/2772483002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:38: CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_ANY_WEBAPK_PACKAGE_NAME), Hmm, does this actually work when loaded in chrome? I don't think native is loaded at this point so you won't actually read the command line flag. Certainly will need to be changed when we add the about:flag. I think you may actually have to follow the caching-dance of |isEnabled| so that you can read it early enough and not depend on native being loaded. If you want to do all this as a follow-up that's fine with me but I'd be surprised if this works in chrome.
https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. Implementation time (i.e. now) is the perfect time to change this to make things less confusing
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks. https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:234: buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity. On 2017/04/10 14:43:58, pkotwicz wrote: > Implementation time (i.e. now) is the perfect time to change this to make things > less confusing Done. https://codereview.chromium.org/2772483002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:38: CommandLine.getInstance().hasSwitch(ChromeSwitches.ENABLE_ANY_WEBAPK_PACKAGE_NAME), On 2017/04/10 14:17:33, Yaron (limited availability) wrote: > Hmm, does this actually work when loaded in chrome? I don't think native is > loaded at this point so you won't actually read the command line flag. Certainly > will need to be changed when we add the about:flag. I think you may actually > have to follow the caching-dance of |isEnabled| so that you can read it early > enough and not depend on native being loaded. If you want to do all this as a > follow-up that's fine with me but I'd be surprised if this works in chrome. Done.
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:194: if (metaInfCount > MAX_META_INF_FILES) { Is it possible to know all the filenames that'd be in META-INF/ and whitelist those, rather than capping at a reasonable number?
lgtm https://codereview.chromium.org/2772483002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:133: boolean isAnyPackgeNameEnabled = isAnyPackageNameEnabled
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2772483002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2772483002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:133: boolean isAnyPackgeNameEnabled = On 2017/04/13 20:50:51, Yaron (limited availability) wrote: > isAnyPackageNameEnabled Done. https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:194: if (metaInfCount > MAX_META_INF_FILES) { On 2017/04/13 20:01:47, Robert Sesek wrote: > Is it possible to know all the filenames that'd be in META-INF/ and whitelist > those, rather than capping at a reasonable number? We could have a whitelist, I'd like to add that in another CL. There are more checks I could do here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2772483002/diff/480001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:194: if (metaInfCount > MAX_META_INF_FILES) { On 2017/04/13 21:00:48, ScottK wrote: > On 2017/04/13 20:01:47, Robert Sesek wrote: > > Is it possible to know all the filenames that'd be in META-INF/ and whitelist > > those, rather than capping at a reasonable number? > > We could have a whitelist, I'd like to add that in another CL. > There are more checks I could do here. I think it's fine to do that in a follow-up CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scottkirkwood@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, rsesek@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/2772483002/#ps500001 (title: "Fix setting of flags.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1492122229060370,
"parent_rev": "0509fea9753de15947d230e379e2cc2338e108dc", "commit_rev":
"45e1d14b15dd10b4d08481d1797255c19558c824"}
Message was sent while issue was closed.
Description was changed from ========== Commment signed webapks working with verification. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 ========== to ========== Commment signed webapks working with verification. * In my test it took 11ms to verify the signature. * Takes about 1ms to check if it's a webapk at all (vs 0.9ms previously). BUG=704213 Review-Url: https://codereview.chromium.org/2772483002 Cr-Commit-Position: refs/heads/master@{#464579} Committed: https://chromium.googlesource.com/chromium/src/+/45e1d14b15dd10b4d08481d17972... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/45e1d14b15dd10b4d08481d17972...
Message was sent while issue was closed.
lgtm |
