978132 - OCSP xpcshell tests fail on Android and B2G because GenerateOCSPResponses executable cannot be found

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #676972 +++

TEST-UNEXPECTED-FAIL | head_psm.js | false == true - See following stack:
head_psm.js :: _getBinaryUtil :: line 304
head_psm.js :: generateOCSPResponses :: line 361
test_ocsp_stapling_expired.js :: <TOP_LEVEL> :: line 36

head_psm.js | false == true - See following stack:
head_psm.js :: _getBinaryUtil :: line 304
head_psm.js :: generateOCSPResponses :: line 361
test_ocsp_required.js :: run_test :: line 23

I imagine all the other tests that generate OCSP responses would fail due to this problem, if they hadn't already failed due to bug 978120.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Note: line 304 in head_psm.js in the stack traces above is do_check_true(utilBin.exists()):

    function _getBinaryUtil(binaryUtilName) {
      let directoryService = Cc["@mozilla.org/file/directory_service;1"]
                               .getService(Ci.nsIProperties);
    
      let utilBin = directoryService.get("CurProcD", Ci.nsILocalFile);
      utilBin.append(binaryUtilName + (gIsWindows ? ".exe" : ""));
      // If we're testing locally, the above works. If not, the server executable
      // is in another location.
      if (!utilBin.exists()) {
        utilBin = directoryService.get("CurWorkD", Ci.nsILocalFile);
        while (utilBin.path.indexOf("xpcshell") != -1) {
          utilBin = utilBin.parent;
        }
        utilBin.append("bin");
        utilBin.append(binaryUtilName + (gIsWindows ? ".exe" : ""));
      }
304   do_check_true(utilBin.exists());
      return utilBin;
    }

Comment 2

[Geoff Brown [:gbrown]]

Brian -- thanks for looking into this. I am pretty sure that remotexpcshelltests.py only pushes the xpcshell binary to the device -- not GenerateOCSPResponse or any other executables. Would BadCertServer and OCSPStaplingServer also be needed? Others??

I think those executables should be pushed to remotexpcshelltests.py's "remoteBinDir", alongside xpcshell. Then hopefully the "CurProcD" case in _getBinaryUtil will work.

I'll try to find time to put together a patch.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I would think anything in TEST_HARNESS_BINS ( https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#109 ) would be needed, but certainly BadCertServer, GenerateOCSPResponse, and OCSPStaplingServer (as well as anything like these added in the future).

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

These tests are also going to fail on Android with BadCertServer/OCSPStaplingServer/etc. failing with "NSS_Init failed: PR_LOAD_LIBRARY_ERROR" due to the fact that the certificate databases that are checked into the tree are in DBM-based cert8.db/key3.db format. On Android and B2G we build with NSS_DISABLE_DBM=1 so DBM isn't available, and thus we can't read the cert8.db/key3.db format. We're going to have to come up with a different solution for storing/generating the test certificates/keys.

It is unclear to me whether it is better to have BadCertServer/OCSPStaplingServer/etc. copied to the target device or whether it is better to have them running on the host device. They need to bind to a network address. When I was trying this stuff out, I was getting a lot of "address in use" errors when running on an Android device. That makes me wonder whether such servers can actually work on Android or not.

It seems like the solutions to all these problems are inter-related so we should think about whether any potential solution for one problem will be problematic for the other problems.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: wip - copy local bin dir to device

On try this outputs:

Pushing /builds/tegra-166/test/build/tests/bin/xpcshell..
Pushing /builds/tegra-166/test/build/tests/bin/pk12util..
Pushing /builds/tegra-166/test/build/tests/bin/fix_stack_using_bpsyms.py..
Pushing /builds/tegra-166/test/build/tests/bin/GenerateOCSPResponse..
Pushing /builds/tegra-166/test/build/tests/bin/OCSPStaplingServer..
Pushing /builds/tegra-166/test/build/tests/bin/ssltunnel..
Pushing /builds/tegra-166/test/build/tests/bin/BadCertServer..
Pushing /builds/tegra-166/test/build/tests/bin/certutil..
Pushing /builds/tegra-166/test/build/tests/bin/fix-linux-stack.pl..

That seems right but needs more testing.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8384339 [details] [diff] [review]
wip - copy local bin dir to device

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

::: testing/xpcshell/remotexpcshelltests.py
@@ +368,5 @@
> +                local = os.path.join(self.localBin, file)
> +                if os.path.isfile(local):
> +                    print >> sys.stderr, "Pushing %s.." % local
> +                    remoteFile = remoteJoin(self.remoteBinDir, file)
> +                    self.device.pushFile(local, remoteFile)

Thanks for working on this! What you are doing here seems reasonable. However, it may be better to use $TEST_HARNESS_BINS instead of os.listdir(self.localBin), as David mentioned in comment 3. Otherwise, you are likely going to be copying too much stuff. As you know, copying stuff to the device is slow so we should do it as little as possible.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Geoff,

Are you still working on this? The reason I ask is that I'm trying to help David and Camilo land the https://bugzilla.mozilla.org/show_bug.cgi?id=976961 for FF 31, and this is listed as a blocker.

Thanks,
Monica

Comment 8

[Geoff Brown [:gbrown]]

I had forgotten about this, but should be able to look at it over the next few days.

Comment 9

[Geoff Brown [:gbrown]]

Attachment: copy additional binaries to xpcshell bin directory

My previous patch copied the entire local bin directory to the remote xpcshell binary directory. That works fine on a test slave, but when running tests against a local build the local bin directory contains a lot of additional files -- wasted setup time and wasted space on the device.

I tried filtering the local bin directory to only copy executable arm binaries, but even that includes a lot of extra files, such as all of the cpp unit test files.

As noted earlier, there is a makefile variable in testing/mochitest/Makefile.in that contains the (approximate) list of required files. But I cannot think of a way to leverage that here: We would not have access to make variables when running on tbpl. Even if we did, we would need to pass the list to remotexpcshelltests.py, making manual invocation even more cumbersome.

I think this is the best approach: Embed a list of well-known binary names in remotexpcshelltests.py, find and push each to the device.

This patch only concerns itself with getting the missing binaries to the device. We need to solve additional problems to get the OCSP tests passing. One step at a time...

Comment 10

[Geoff Brown [:gbrown]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> These tests are also going to fail on Android with
> BadCertServer/OCSPStaplingServer/etc. failing with "NSS_Init failed:
> PR_LOAD_LIBRARY_ERROR" due to the fact that the certificate databases that
> are checked into the tree are in DBM-based cert8.db/key3.db format. 

:briansmith -- With the binaries copied to device, I found an issue in _getBinaryUtil; once I modified that to find binaries on Android, I hit PR_LOAD_LIBRARY_ERROR, as you anticipated. Have you thought about this more? Where should we go from here?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I suggest that we convert all the cert8.db/key3.db files under security/manager/ssl/tests/unit/* to cert9.db/key4.db format. This should let Android's and B2G's NSS read the files. Desktop NSS will also be able to read the files but we may need to set an environment variable *only for those executables* (and *NOT for the Gecko-based program under test) to force the expectation/usage of the cert9/key4 (a.k.a. SQLITE) database format.

NSS's certutil can, IIRC, convert cert8/key3 to cert9/key4. But, I don't know how to do it. I suggest posting on dev-tech-crypto and/or working with cviecco and dkeeler on this as I won't have much more time to spend on it.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8412138 [details] [diff] [review]
copy additional binaries to xpcshell bin directory

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

nice and clean.

Comment 13

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0db52e1d0ab

Comment 14

[Geoff Brown [:gbrown]]

:cviecco -- My patch will put the missing executables on the device, but there's more to do -- see Comments 10 and 11. Can you take over here? Or if not, give me clear direction on what to do next?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0db52e1d0ab

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

David wrote the original generate_certs.sh so maybe he knows.

Comment 17

[Camilo Viecco (:cviecco)]

1. migrate the db to the new format is 'just' a: 'certutil --upgrade-merge -d sql:/tmp/foobar/new --source-dir /tmp/foobar/old --upgrade-id 1'.
2. Change the binaries (or figure out a flag) so that they can be run using the new format (I can take a look at that, but not until late tomorrow)
3. Profit?

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I think what we need to do for this bug is done (thanks, Geoff!)
I filed bug 1004270 to modify generate_certs.sh and the test binaries to use the sql databases.