725180 - Create test suite for libmar

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

This bug is to create a test suite for modules/libmar.

This test suite should cover:
- Creating MARs
- Extracting MARs
- Signing MARs
- Verifying signed MARs
- Stripping MAR signatures
- Refreshing Product Info Blocks

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: WIP patch

Work in progress patch.
- Created test suite build files
- Added 4 reference MARs and code for creating 4 MARs from source files, and comparing to verify they are what they should be.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

- Tests for Creating MARs
- Tests for Extraction MARs
- Tests for Signing, Verifying, and stripping signatures for MARs, and extracting from signed MARs.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Updated last Windows only patch to work on all platforms (except android).

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3

Fixed some build problems and a test problem on Linux and OSX.

In particular for test changes, the verify signmar command line differs on Windows vs other platforms since verification is done via DER file vs NSS config directory.  

In particular for build changes, had to add a prefix.

Also I expanded the MAR product, channel, version checks to be only enabled when the define is enabled in confvars.sh like verification of signed mars.  So windows only for now.

Verified tests are passing now on both OS X and Windows.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Added new tests for:
- For a manually crafted MAR where I manually adjusted the version number of the MAR with a hex editor.  Asserts that signature verification should fail.
- For extracting old no product info block MARs
- For extracting old no product info block MARs that were signed (in use today)
- For verifying old product info block MARs that were signed

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/Makefile.in b/modules/libmar/Makefile.in
>--- a/modules/libmar/Makefile.in
>+++ b/modules/libmar/Makefile.in
>@@ -38,11 +38,16 @@
> 
> DEPTH		= ../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
>-DIRS		= sign verify src tool
>+DIRS = sign verify src tool
>+MODULE = libmar_test
>+
>+ifdef ENABLE_TESTS
>+DIRS += test
nit: I think the current preferred dir name is tests.

>+endif
> 
> include $(topsrcdir)/config/rules.mk
>diff --git a/modules/libmar/TEST/Makefile.in b/modules/libmar/TEST/Makefile.in
Why uppercase?

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/TEST/unit/head_libmar.js.in b/modules/libmar/TEST/unit/head_libmar.js.in
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/TEST/unit/head_libmar.js.in
>@@ -0,0 +1,136 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+const BIN_SUFFIX = "@BIN_SUFFIX@";
>+
>+/**
>+ * Compares the binary data of 2 files.
Just "Compares binary data of two parameters and throws if they aren't the same." since the files aren't involved at this point

Might as well add arr1 and arr2

>+ * Throws on mismatch, does nothing on match.
>+*/
>+function compareBinaryData(arr1, arr2) {
>+  do_check_eq(arr1.length, arr2.length);
>+    for (let i = 0; i < arr1.length; i++) {
>+      if (arr1[i] != arr2[i]) {
>+        throw "Data differs at index " + i;
indentation is off on the above 3 lines

>+    }
>+  }
>+}

Comment 8

[Brian R. Bondy [:bbondy]]

> include $(topsrcdir)/config/rules.mk
>diff --git a/modules/libmar/TEST/Makefile.in b/modules/libmar/TEST/Makefile.in
> Why uppercase?

I can't explain why it went uppercase, my local patch that has since been qrefresh'ed no longer has it uppercase.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/TEST/unit/head_libmar.js.in b/modules/libmar/TEST/unit/head_libmar.js.in
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/TEST/unit/head_libmar.js.in
>@@ -0,0 +1,136 @@
>...
>+/** 
>+ * Reads a file's data and returns it
>+ *
>+ * @param file The file to read the data from
>+ * @return a byte array for the data in the file.
>+*/
>+function getBinaryFileData(file) {
>+  let fileStream = Components.classes[
nit I'd go with
const Cc = Components.classes;
const Ci = Components.interfaces;
etc. to lessen the additional lines to stay under 80

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 599461 [details] [diff] [review]
Patch v4.

Could you split out the c++ code changes and their makefiles into a separate patch?
>diff --git a/modules/libmar/tool/Makefile.in b/modules/libmar/tool/Makefile.in
>--- a/modules/libmar/tool/Makefile.in
>+++ b/modules/libmar/tool/Makefile.in
>@@ -49,16 +49,20 @@ USE_STATIC_LIBS = 1
> endif
> 
> # The mar executable is output into dist/host/bin since it is something that
> # would only be used by our build system and should not itself be included in a
> # Mozilla distribution.
> HOST_PROGRAM = mar$(HOST_BIN_SUFFIX)
> PROGRAM = signmar$(BIN_SUFFIX)
> 
>+# Don't link the updater against libmozglue. See bug 687139
Copy and paste error. This isn't the updater

>+MOZ_GLUE_LDFLAGS =
>+MOZ_GLUE_PROGRAM_LDFLAGS =
>+

r=me for the c++ code changes and their makefiles. I'm still reviewing the tests but if you could please resubmit with the comments addressed so far.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Libmar test suite. Patch v5.

Updated review comments and removed C++ parts and related Makefiles.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Misc libmar fixes found from tests. Patch v1.

Marking the C++ parts and their Makefile changes as r+ as per comment 10:

rs:
> r=me for the c++ code changes and their makefiles.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Fix error handling Patch v1.

When testing the error handling for cert verification errors, I found that we will try to re-use the service for the new service specific security error codes after re-downloading the full MAR.  So this error handling should follow the same as the other service specific error codes (fallback to pending before trying to re-download the MAR).

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 600167 [details] [diff] [review]
Libmar test suite.  Patch v5.

>diff --git a/modules/libmar/tests/unit/test_sign_verify.js b/modules/libmar/tests/unit/test_sign_verify.js
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/tests/unit/test_sign_verify.js
>@@ -0,0 +1,187 @@
>...
>+  function verifyMAR(signedMAR, wantSuccess) {
>+    if (wantSuccess === undefined) {
>+      wantSuccess = true;
>+    }
>+    // Get a process to the signmar binary from the dist/bin directory.
>+    let process = Cc["@mozilla.org/process/util;1"].
>+                  createInstance(Ci.nsIProcess);
>+    let signmarBin = do_get_file("signmar" + BIN_SUFFIX);
>+
>+    // Make sure the signmar binary exists and is an executable.
>+    do_check_true(signmarBin.exists());
>+    do_check_true(signmarBin.isExecutable());
>+
>+    let DERFile = do_get_file("data/mycert.der");
>+
>+    // Will reference the arguments to use for verification in signmar
>+    let args;
>+
>+    // The XPCShell test wiki indicates this is the preferred way for 
>+    // Windows detection.
>+    var isWindows = ("@mozilla.org/windows-registry-key;1" 
>+                     in Cc);
nit: why 2 lines?

I'm a little concerned that there will be times where Windows file in use will prevent the directory removals but it would be better to fix those if they come up.

Looks good!

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 600265 [details] [diff] [review]
Fix error handling Patch v1.

This will need to be backed out at the same time that these checks are made when not using the service.

Comment 16

[Brian R. Bondy [:bbondy]]

Forgot the 2 line -> 1 line nit before landing but I'll fix this once I do the patch for doing the security checks non service, made a local note.

Comment 17

[Robert Strong (they/them - no direct email)]

Just a nit... not a problem :)

Comment 18

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/bfb987083bcb
http://hg.mozilla.org/mozilla-central/rev/4d9778e932ba

Comment 19

[Brian R. Bondy [:bbondy]]

and...
http://hg.mozilla.org/mozilla-central/rev/2b1a53905350

Comment 20

[Brian R. Bondy [:bbondy]]

> I'm a little concerned that there will be times where Windows file in use 
> will prevent the directory removals but it would be better to fix those if 
> they come up.

By the way I'm not sure why some updater tests are like that but it's something that we shouldn't need to worry about. I think there is something else going on  there (like an unclosed handle somewhere) but I'll look into that at a future date if I have more time :)

Comment 21

[Robert Strong (they/them - no direct email)]

I wouldn't worry about it unless they come up. Also, we've seen cases where we'll run into file in use on opt but not debug with update and other tests.

Comment 22

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97