Commment signed webapks working and verified.

chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkVerifySignatureTest.java

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.webapk.lib.client;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+import org.chromium.testing.local.TestDir;
+
+import java.io.RandomAccessFile;
+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.security.KeyFactory;
+import java.security.PublicKey;
+import java.security.SignatureException;
+import java.security.spec.X509EncodedKeySpec;
+
+/** Unit tests for WebApkVerifySignature for Android. */
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class WebApkVerifySignatureTest {
+    /** Elliptical Curves, Digital Signature Algorithm */
+    private static final String KEY_FACTORY = "EC";
+
+    private static final String TEST_DATA_DIR = "/webapks/";
+
+    static PublicKey readPublicKey(String publicDER) throws Exception {
+        return createPublicKey(Files.readAllBytes(Paths.get(publicDER)));
+    }
+
+    private static PublicKey createPublicKey(byte[] bytes) throws Exception {
+        return KeyFactory.getInstance(KEY_FACTORY).generatePublic(new X509EncodedKeySpec(bytes));
+    }
+
+    @Test
+    public void testHexToBytes() throws Exception {
+        assertEquals(null, WebApkVerifySignature.hexToBytes(""));
+        byte[] test = {(byte) 0xFF, (byte) 0xFE, 0x00, 0x01};
+        assertArrayEquals(test, WebApkVerifySignature.hexToBytes("fffe0001"));
+        assertArrayEquals(test, WebApkVerifySignature.hexToBytes("FFFE0001"));
+        assertEquals(null, WebApkVerifySignature.hexToBytes("f")); // Odd number of nibbles.
+    }
+
+    @Test
+    public void testCommentHash() throws Exception {
+        byte[] bytes = hexStringToByteArray("decafbad");

[pkotwicz, 2017/03/31 03:59:24]

Can you initialize bytes with the result of hexStringToByteArray() and remove
hexStringToByteArray()

[ScottK, 2017/04/03 17:44:18]

Done.

+        assertEquals(null, WebApkVerifySignature.parseCommentSignature("weapk:decafbad"));
+        assertEquals(null, WebApkVerifySignature.parseCommentSignature("webapk:"));
+        assertEquals(null, WebApkVerifySignature.parseCommentSignature("webapk:decafbad"));
+        assertArrayEquals(
+                bytes, WebApkVerifySignature.parseCommentSignature("webapk:12345:decafbad"));
+        assertArrayEquals(
+                bytes, WebApkVerifySignature.parseCommentSignature("XXXwebapk:0000:decafbadXXX"));
+        assertArrayEquals(
+                bytes, WebApkVerifySignature.parseCommentSignature("\n\nwebapk:0000:decafbad\n\n"));
+        assertArrayEquals(bytes,
+                WebApkVerifySignature.parseCommentSignature("chrome-webapk:000:decafbad\n\n"));
+        assertArrayEquals(bytes,
+                WebApkVerifySignature.parseCommentSignature(
+                        "prefixed: chrome-webapk:000:decafbad :suffixed"));
+    }
+
+    public static String testFilename(String fname) {

[pkotwicz, 2017/03/31 03:59:24]

testFilename() -> testFilePath()

[ScottK, 2017/04/03 17:44:18]

Done.

+        return TestDir.getTestFilePath(TEST_DATA_DIR + fname);
+    }
+
+    @Test
+    public void testVerifyFiles() throws Exception {

[pkotwicz, 2017/03/31 03:59:24]

Are the testVerifyFiles() and the testBadVerifyFiles() tests necessary? They
look like duplicates of the tests in WebApkValidatorTest.java

[ScottK, 2017/04/03 17:44:18]

They increase our code coverage by testing things at a higher level.

[pkotwicz, 2017/04/04 18:19:36]

I don't think that it increases code coverage. What code in
WebApkVerifySignature does this test test that isn't tested by
WebApkValidatorTest?

[ScottK, 2017/04/04 21:03:02]

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.

[pkotwicz, 2017/04/05 02:54:28]

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

[ScottK, 2017/04/05 20:14:27]

Ok, removing this.

+        PublicKey pub = readPublicKey(testFilename("public.der"));
+        String[] filenames = {"example.apk", "java-example.apk"};
+        for (String filename : filenames) {
+            RandomAccessFile file = new RandomAccessFile(testFilename(filename), "r");
+            FileChannel inChannel = file.getChannel();
+            MappedByteBuffer buf =
+                    inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
+            buf.load();
+            WebApkVerifySignature v = new WebApkVerifySignature(buf);
+            assertEquals("for file " + filename, WebApkVerifySignature.ERROR_OK, v.read());
+            assertEquals(
+                    "for file " + filename, WebApkVerifySignature.ERROR_OK, v.verifySignature(pub));
+            buf.clear();
+            inChannel.close();
+            file.close();
+        }
+    }
+
+    @Test
+    public void testBadVerifyFiles() throws Exception {
+        PublicKey pub = readPublicKey(testFilename("public.der"));
+        String[] filenames = {
+                "bad-sig.apk", "bad-utf8-fname.apk", "empty.apk", "extra-len-too-large.apk",
+                "fcomment-too-large.apk", "no-cd.apk", "no-comment.apk", "no-eocd.apk",
+                "no-lfh.apk", "not-an.apk", "too-many-metainf.apk", "truncated.apk", "zeros.apk",
+                "zeros-at-end.apk",
+        };
+        for (String filename : filenames) {
+            RandomAccessFile file = new RandomAccessFile(testFilename(filename), "r");
+            FileChannel inChannel = file.getChannel();
+            MappedByteBuffer buf =
+                    inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
+            buf.load();
+            WebApkVerifySignature v = new WebApkVerifySignature(buf);
+            try {
+                if (v.read() == WebApkVerifySignature.ERROR_OK) {
+                    assertTrue(v.verifySignature(pub) != WebApkVerifySignature.ERROR_OK);
+                }
+            } catch (SignatureException | IllegalArgumentException e) {
+                // expected
+            }
+            buf.clear();
+            inChannel.close();
+            file.close();
+        }
+    }
+
+    private static byte[] hexStringToByteArray(String s) {
+        int len = s.length();
+        if (len % 2 == 1) {
+            // Expect an even number of nibbles.
+            return null;
+        }
+        byte[] data = new byte[len / 2];
+        for (int i = 0; i < len; i += 2) {
+            data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4)
+                    | Character.digit(s.charAt(i + 1), 16));
+        }
+        return data;
+    }
+}