1071085 - Add proper tests for THA

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

We need to serve manifests from https and do the pinning + signature verification.

According to Camilo, it's easy! We just have to:

1. you will need to add the mochites PGO ca pinning value which is:
   hXweb81C3HnmM2Ai1dnUzFba40UJMhuu8qZmvN/6WWc=

2.
 gSSService.setKeyPins("WHATEVERDOMAININMIOCHITEST", false, 1000, 2,
                        [PGO_ROOT_KEY_HASH]);

3. optinonally modify build/pgo/server-locations.txt to have a new domain

And we probably don't need a new domain.

Comment 1

[Markus Nilsson]

Attachment: [WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

Attached WIP-patch.
To make the tests pass you'll need to re-sign valid_manifest.webapp to match your manifest-signing-test-root.inc.
You can use the /security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh script to create a new sig-file, just replace the contents of the manifest.webapp file in that folder with the valid_manifest.webapp content.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8495316 [details] [diff] [review]
[WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

Review of attachment 8495316 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/tests/mochitest.ini
@@ -8,4 @@
>    file_cached_app.template.appcache
>    file_cached_app.template.webapp
>    file_hosted_app.template.webapp
> -  file_trusted_app.template.webapp

since you remove this file, update the test that was referring to it.

::: dom/apps/tests/test_trusted_app_install.js
@@ +11,5 @@
> +  let thaDomain = 'example.com';
> +  let thaPath = '/tests/dom/apps/tests/tha_test_files/';
> +
> +  SpecialPowers.setAllAppsLaunchable(true);
> +  SpecialPowers.setBoolPref('dom.mozBrowserFramesEnabled', true);

use  SpecialPowers.pushPrefEnv instead.

@@ +53,5 @@
> +    finish();
> +  }
> +
> +  function runTest() {
> +    let manifestUrl = 'https://' + thaDomain + thaPath + 'valid_manifest.sjs';

since the manifest is static, no need to serve it from a sjs script.

@@ +96,5 @@
> +      app.ondownloaderror = mozAppsError;
> +      yield undefined;
> +    }
> +    is(app.installState, 'installed', 'Trusted App is installed');
> +    is(app.manifest.type, 'trusted', 'App is trusted');

Testing that the app is trusted would be better done by adding a permission in the manifest that is granted to tha but not other hosted apps and check that we set it properly.

@@ +103,5 @@
> +    checkAppState(app, true, 0, continueTest);
> +    yield undefined;
> +
> +    // Check for updates. The current infrastructure always returns a new
> +    // appcache manifest, so there should always be an update.

there is no appcache_path in the manifest so I don't think this portion makes any sense for now.

Comment 3

[Markus Nilsson]

Attachment: [WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

Updated WIP.
Basically it's the permission check of the installed app that's new.

Comment 4

[Zoran Jovanovic]

Attachment: [WIP]-Bug-1071085-Add-proper-tests-for-THA-1.patch

More test cases...

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8496936 [details] [diff] [review]
[WIP]-Bug-1071085-Add-proper-tests-for-THA-1.patch

Review of attachment 8496936 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

One thing that I'd like to see fixed is to not replicate the same code in all the .sjs files, but use a single one with a parameter to switch from one manifest to another.
I also can't give r+ yet since I can't test locally by just applying the patch or push it to try.

::: dom/apps/tests/test_trusted_app_install.js
@@ +248,5 @@
> +    domParent.appendChild(ifr);
> +  }
> +
> +  function finish() {
> +    SpecialPowers.clearUserPref('dom.mozBrowserFramesEnabled');

that is useless since it's set by pushPrefEnv() that will restore the initial value.

::: dom/apps/tests/tha_test_files/index_external.html
@@ +2,5 @@
> +<html>
> +<head>
> +	<script
> +		type="application/javascript;version=1.8"
> +		src="https://w3c-test.org/tests/dom/apps/tests/tha_test_files/index.js">

strange attribute indentation

Comment 6

[Markus Nilsson]

Attachment: [WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

The biggest change from the previous WIP is the test_manifest.sjs

Comment 7

[Markus Nilsson]

Attachment: Bug-1071085-Add-proper-tests-for-THA.patch

The tests should now build and pass. To run them, start with a clean slate and execute the following:
./security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh --regenerate-test-certs
./mach clobber
./mach build
./mach mochitest dom/apps/tests/test_trusted_app_install.html

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8498142 [details] [diff] [review]
Bug-1071085-Add-proper-tests-for-THA.patch

Review of attachment 8498142 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that would be r+ if we could land that without having to run create_test_files.sh manually.

Comment 9

[Markus Nilsson]

Attachment: Bug-1071085-Add-proper-tests-for-THA.patch

Updated the makefile so that you no longer need to run the script manually :)

Comment 10

[[:fabrice] Fabrice Desré]

I pushed the patch to try, it's not ready yet:
https://tbpl.mozilla.org/?tree=Try&rev=97d0871c902d

Comment 11

[Zoran Jovanovic]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> I pushed the patch to try, it's not ready yet:
> https://tbpl.mozilla.org/?tree=Try&rev=97d0871c902d

What's the version of python running on those servers?

Comment 12

[Markus Nilsson]

I think that we need help from someone with better knowledge of the build system.
As far as I can tell there are two different problems:

1) [sign_b2g_manifest.py line 8]: ImportError: No module named argparse
The argparse module that we try to import should be included in python 2.7 and looking through the build log from one of the builds that fails with this problem it looks like python 2.7 is used. (Search for "Creating Python environment" in e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=49450900&tree=Try&full=1#error0)

2) [nss_ctypes.py line 22]: plc4 library can't be found
It looks like the library is used when building other parts of gecko, but I don't know how to make it available here.

Comment 13

[[:fabrice] Fabrice Desré]

Mike, feel free to redirect to someone else familiar with this part of the build system (see comment 12). Thanks!

Comment 14

[Mike Hommey [:glandium]]

(In reply to Markus Nilsson from comment #12)
> I think that we need help from someone with better knowledge of the build
> system.
> As far as I can tell there are two different problems:
> 
> 1) [sign_b2g_manifest.py line 8]: ImportError: No module named argparse
> The argparse module that we try to import should be included in python 2.7
> and looking through the build log from one of the builds that fails with
> this problem it looks like python 2.7 is used. (Search for "Creating Python
> environment" in e.g.
> https://tbpl.mozilla.org/php/getParsedLog.
> php?id=49450900&tree=Try&full=1#error0)

(Note you also have "certutil: command not found" messages too)

The problem here is that the script is running "python", when it should be running $PYTHON, which would need to be exported from the environment (export PYTHON in the corresponding Makefile)

> 2) [nss_ctypes.py line 22]: plc4 library can't be found
> It looks like the library is used when building other parts of gecko, but I
> don't know how to make it available here.

There hasn't been a plc4 library for months. All NSS and NSPR is folded in libnss3.dll.

Comment 15

[Markus Nilsson]

Attachment: Bug-1071085-Add-proper-tests-for-THA.patch

Thank you Mike.

@Fabrice: If you think that the changes looks good, please do another try.

Comment 16

[[:fabrice] Fabrice Desré]

Hm, are you sure it worked locally?
https://tbpl.mozilla.org/?tree=Try&rev=49d9b06fe74a

Comment 17

[Markus Nilsson]

Hi again Mike.

Sadly I'm still stuck at this build problem.
As you noted, the certutil program isn't immediately available at the build servers.
I figured out that it's built and that a symlink ends up in $(DIST)/bin/
I therefore tried to export it to create_test_files.sh in the same way as $(PYTHON) and added certutil as a prerequisite to ensure-signingDB.
Sadly that didn't work since certutil's makefile and the security/apps/Makefile doesn't seem to be executed by the same make process and therefore can't depend on each other.
I then tried to delay the execution of the steps that uses create_test_files.sh until certutil had been built by moving those steps to the "misc" build tier.
At first that seemed to work, but when I built with a completely clean workspace it failed since security/apps/AppTrustDomain.cpp has a dependency to manifest-signing-test-root.inc which in turn depends on trusted_ca1.der which is generated by the create_test_files.sh script...

So in conclusion, I need certutil to be available in the security/app/Makefile but I don't know how.

Comment 18

[Mike Hommey [:glandium]]

Add the following at the end of config/recurse.mk, before the last endif:

security/app/target: config/external/nss/target

Comment 19

[Markus Nilsson]

Hi again.

I tried your suggestion, but when that didn't seem to do anything I realized that you probably intended:

security/apps/export: config/external/nss/target

I then got a build error:
./nsprpub/pr/src/md/unix/unix.c:3049: undefined reference to `clock_gettime'
the make chain was:
[realbuild]->[default]->[export]->[config/external/nss/target]->[libs-nss/lib]->[libs]->[/{PROJ-DIR}/obj-x86_64-unknown-linux-gnu/security/nss/lib/util/libnssutil3.so]

This error seems to be quite *nix specific, probably it will manifest itself in other ways on the other platforms.
It seems that the "export" make step doesn't have access to the same libraries as the "compile" step?

I tried to add the following to config/external/nss/moz.build in hopes of getting the required lib:

OS_LIBS += ['-lrt']

but that didn't work. Any thoughts/suggestions on how this can be solved?

Comment 20

[Mike Hommey [:glandium]]

(In reply to Markus Nilsson from comment #19)
> Hi again.
> 
> I tried your suggestion, but when that didn't seem to do anything I realized
> that you probably intended:
> 
> security/apps/export: config/external/nss/target

No, I really meant security/apps/export. You need to change that "export::" in security/apps/Makefile.in to be "target::" instead.

> I then got a build error:
> ./nsprpub/pr/src/md/unix/unix.c:3049: undefined reference to `clock_gettime'
> the make chain was:
> [realbuild]->[default]->[export]->[config/external/nss/target]->[libs-nss/
> lib]->[libs]->[/{PROJ-DIR}/obj-x86_64-unknown-linux-gnu/security/nss/lib/
> util/libnssutil3.so]

Seems to me completely unrelated, and would equally happen without your patch. Or you're doing other things, but there's no patch to look at, so...

Comment 21

[Markus Nilsson]

Attachment: 0001-WIP-Bug-1071085-Add-proper-tests-for-THA.patch

Thanks to Mike the updated patch now builds on most versions of Linux, but the trusted hosted test fails:
https://tbpl.mozilla.org/?tree=Try&rev=a1b49a83504a

It looks like the test_manifest.sjs-file can't find and copy the sig-files that were created during the build.
I see that the mochitests seem to run from the directory /builds/slave/test/build/tests/mochitest/tests/ but the build seems to be occur in /builds/slave/try-lx-00000000000000000000000/.
Do you know how these directories are related to each other? Is there any way to copy the sig-files in the build process so they're available to the tests?

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8527759 [details] [diff] [review]
0001-WIP-Bug-1071085-Add-proper-tests-for-THA.patch

Review of attachment 8527759 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/apps/Makefile.in
@@ +9,3 @@
>  GEN_CERT_HEADER = $(srcdir)/gen_cert_header.py
>  TEST_SSL_PATH = $(srcdir)/../manager/ssl/tests/unit/test_signed_manifest/
> +MANIFEST_PATH = $(srcdir)/../../dom/apps/tests/tha_test_files/

might as well use $(topsrcdir)/dom/apps...

@@ +14,5 @@
> +define SIGNING_template
> +
> +@cp $(MANIFEST_PATH)$(1).webapp $(TEST_SSL_PATH)manifest.webapp
> +cd $(TEST_SSL_PATH); ./create_test_files.sh
> +@cp $(TEST_SSL_PATH)testValidSignedManifest/manifest.sig $(MANIFEST_PATH)$(1).sig

Do *not* create files in the source directory.

@@ +56,5 @@
> +sign-test-manifests: ensure-signingDB
> +	@mv $(TEST_SSL_PATH)manifest.webapp $(TEST_SSL_PATH)manifest.~webapp
> +	$(foreach manifest,$(MANIFESTS),$(call SIGNING_template,$(manifest)))
> +	@cp $(MANIFEST_PATH)invalid_csp_manifest.sig $(MANIFEST_PATH)valid_manifest_invalid_signature.sig
> +	@mv $(TEST_SSL_PATH)manifest.~webapp $(TEST_SSL_PATH)manifest.webapp

Why the mv, do something, mv back dance?

@@ +73,4 @@
>    manifest-signing-root.inc \
>    manifest-signing-test-root.inc \
>    xpcshell.inc \
> +  sign-test-manifests \

This is not needed to build the C++ sources, is it?

Comment 23

[Mike Hommey [:glandium]]

(In reply to Markus Nilsson from comment #21)
> Created attachment 8527759 [details] [diff] [review]
> 0001-WIP-Bug-1071085-Add-proper-tests-for-THA.patch
> 
> Thanks to Mike the updated patch now builds on most versions of Linux, but
> the trusted hosted test fails:
> https://tbpl.mozilla.org/?tree=Try&rev=a1b49a83504a
> 
> It looks like the test_manifest.sjs-file can't find and copy the sig-files
> that were created during the build.
> I see that the mochitests seem to run from the directory
> /builds/slave/test/build/tests/mochitest/tests/ but the build seems to be
> occur in /builds/slave/try-lx-00000000000000000000000/.
> Do you know how these directories are related to each other?

They are entirely unrelated. Tests don't even run on the same machines!

> Is there any
> way to copy the sig-files in the build process so they're available to the
> tests?

You need to make them part of the test package. Since your files are generated, and should be in the objdir, not the source dir (contrary to what your patch does), you need to list them in support-files and generated-files in the mochitest.ini. I think you also need to copy them to 
_tests/testing/mochitest/tests/$(relativesrcdir). See layout/style/test/Makefile.in for example.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8527759 [details] [diff] [review]
0001-WIP-Bug-1071085-Add-proper-tests-for-THA.patch

Review of attachment 8527759 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/apps/Makefile.in
@@ +60,5 @@
> +	@mv $(TEST_SSL_PATH)manifest.~webapp $(TEST_SSL_PATH)manifest.webapp
> +
> +include $(topsrcdir)/config/config.mk
> +
> +$(CPPSRCS): \

Err, that should be CPPOBJS, not CPPOBJS. Sorry for the confusion.

Comment 25

[Markus Nilsson]

Attachment: WIP-Bug-1071085-Add-proper-tests-for-THA-updates.patch

Added an updated patch that produces the following try result:
https://tbpl.mozilla.org/?tree=Try&rev=6e116b5f1b6e

After further help from Mike, the .sig files no longer end up in the src folder and the THA tests now pass. The comments Mike had should be resolved except changing CPPSRCS to CPPOBJS, since that resulted in compiler error.

So now the compiler errors on other platforms remain. Any help/suggestions with those would be appreciated!

Comment 26

[Mike Hommey [:glandium]]

> The comments Mike had should be resolved except changing CPPSRCS to CPPOBJS, since that resulted in compiler error.

Ah, you need to replace the config.mk include with rules.mk.

Comment 27

[Josh Cheng [:josh]]

Hi Fabrice,
Since 2.1 already CC long ago. Not sure we still want this for future release?
Thanks!

Comment 28

[[:fabrice] Fabrice Desré]

(In reply to Josh Cheng [:josh] from comment #27)
> Hi Fabrice,
> Since 2.1 already CC long ago. Not sure we still want this for future
> release?
> Thanks!

No we don't really care about that anymore.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.