Status

Core Graveyard
DOM: Apps
4 years ago
6 months ago

People

(Reporter: fabrice, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 1016421

Comment 1

4 years ago
Created attachment 8495316 [details] [diff] [review]
[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.
Attachment #8495316 - Flags: feedback?(fabrice)
(Reporter)

Comment 2

4 years ago
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.
Attachment #8495316 - Flags: feedback?(fabrice) → feedback+

Comment 3

4 years ago
Created attachment 8496060 [details] [diff] [review]
[WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

Updated WIP.
Basically it's the permission check of the installed app that's new.
Attachment #8495316 - Attachment is obsolete: true
Attachment #8496060 - Flags: feedback?(fabrice)

Comment 4

4 years ago
Created attachment 8496936 [details] [diff] [review]
[WIP]-Bug-1071085-Add-proper-tests-for-THA-1.patch

More test cases...
Attachment #8496936 - Flags: review?(fabrice)
(Reporter)

Updated

4 years ago
Attachment #8496060 - Attachment is obsolete: true
Attachment #8496060 - Flags: feedback?(fabrice)
(Reporter)

Comment 5

4 years ago
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
Attachment #8496936 - Flags: review?(fabrice)

Comment 6

4 years ago
Created attachment 8497568 [details] [diff] [review]
[WIP]-Bug-1071085-Add-proper-tests-for-THA.patch

The biggest change from the previous WIP is the test_manifest.sjs
Attachment #8497568 - Flags: feedback?(fabrice)

Updated

4 years ago
Attachment #8496936 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8497568 - Flags: feedback?(fabrice) → feedback+

Comment 7

4 years ago
Created attachment 8498142 [details] [diff] [review]
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
Attachment #8497568 - Attachment is obsolete: true
Attachment #8498142 - Flags: review?(fabrice)
(Reporter)

Comment 8

4 years ago
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.
Attachment #8498142 - Flags: review?(fabrice)

Comment 9

4 years ago
Created attachment 8498989 [details] [diff] [review]
Bug-1071085-Add-proper-tests-for-THA.patch

Updated the makefile so that you no longer need to run the script manually :)
Attachment #8498142 - Attachment is obsolete: true
Attachment #8498989 - Flags: review?(fabrice)
(Reporter)

Comment 10

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

Comment 11

4 years ago
(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

4 years ago
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.

Updated

4 years ago
Blocks: 1077529
(Reporter)

Updated

4 years ago
Attachment #8498989 - Flags: review?(fabrice)
(Reporter)

Comment 13

4 years ago
Mike, feel free to redirect to someone else familiar with this part of the build system (see comment 12). Thanks!
Flags: needinfo?(mh+mozilla)

Updated

4 years ago
No longer blocks: 1077529

Updated

4 years ago
Blocks: 1079915
(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.
Flags: needinfo?(mh+mozilla)

Comment 15

4 years ago
Created attachment 8505398 [details] [diff] [review]
Bug-1071085-Add-proper-tests-for-THA.patch

Thank you Mike.

@Fabrice: If you think that the changes looks good, please do another try.
Attachment #8498989 - Attachment is obsolete: true
Attachment #8505398 - Flags: review?(fabrice)
(Reporter)

Comment 16

4 years ago
Hm, are you sure it worked locally?
https://tbpl.mozilla.org/?tree=Try&rev=49d9b06fe74a
(Reporter)

Updated

4 years ago
Attachment #8505398 - Flags: review?(fabrice)

Comment 17

4 years ago
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.
Flags: needinfo?(mh+mozilla)
Add the following at the end of config/recurse.mk, before the last endif:

security/app/target: config/external/nss/target
Flags: needinfo?(mh+mozilla)

Comment 19

4 years ago
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?
Flags: needinfo?(mh+mozilla)
(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...
Flags: needinfo?(mh+mozilla)

Comment 21

4 years ago
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? Is there any way to copy the sig-files in the build process so they're available to the tests?
Attachment #8505398 - Attachment is obsolete: true
Attachment #8527759 - Flags: feedback?(mh+mozilla)
Attachment #8527759 - Flags: feedback?(fabrice)
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?
Attachment #8527759 - Flags: feedback?(mh+mozilla) → feedback-
(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 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

3 years ago
Created attachment 8530327 [details] [diff] [review]
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!
Attachment #8527759 - Attachment is obsolete: true
Attachment #8527759 - Flags: feedback?(fabrice)
Attachment #8530327 - Flags: feedback?(mh+mozilla)
Attachment #8530327 - Flags: feedback?(fabrice)
> 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.
Attachment #8530327 - Flags: feedback?(mh+mozilla)
(Reporter)

Updated

3 years ago
Attachment #8530327 - Flags: feedback?(fabrice)

Comment 27

3 years ago
Hi Fabrice,
Since 2.1 already CC long ago. Not sure we still want this for future release?
Thanks!
Flags: needinfo?(fabrice)
(Reporter)

Comment 28

3 years ago
(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.
Flags: needinfo?(fabrice)

Updated

6 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.