1103946 - Change test option --content-sandbox to --strict-content-sandbox

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Bug 928044 enables a weak content sandbox by default and adds a new pref to enable a more strict version.

The --content-sandbox mochitest and mozharness option need to be changed to reflect this.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Change --content-sandbox mach / mochitest option to --strict-content-sandbox.

I need to wait until bug 928044 lands to make sure the prefs don't change, but I thought I'd get the patches for this attached.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Change --content-sandbox mozharness option to --strict-content-sandbox.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 3: In buildbot config, change --content-sandbox mozharness option to --strict-content-sandbox.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 1: Change --content-sandbox mach / mochitest option to --strict-content-sandbox. v2

Joel, good to finally catch up with you at the airport. :)

This is just to bring this back into line with the changes from bug 928044, where I turned the Windows content sandbox on by default and added a pref to enable a more strict policy.

At some point I might flip this so that you run with the more strict sandbox by default, when running tests locally, but I think that's a step too far at the moment.

Chris, just wanted to make you aware of this.
I've run the various media tests locally, so hopefully I haven't broken anything this time. :)

Here's a try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b500789b8ccf

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8528306 [details] [diff] [review]
Part 2: Change --content-sandbox mozharness option to --strict-content-sandbox.

Armen, parts 2 and 3 are just the corresponding mozharness and buildbot changes.

Pity that we didn't actually meet last week.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8) (PTO back normal Nov 17)]

Comment on attachment 8533126 [details] [diff] [review]
Part 1: Change --content-sandbox mach / mochitest option to --strict-content-sandbox. v2

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

overall this patch looks pretty harmless.  A couple general concerns:
1) if we are going to have multiple levels of sandboxing I see this looking a bit more confusing in the manifests and the harness scripts.  
2) there are needs to do higher level test filtering of mochitest (which you are essentially doing).  Think of a use case where we run smoketests for 3 minutes, then everything else at a different tier depending on status and load.

Both of these above two scenarios are approaching the same problem space with a specific scenario being addressed.  While I don't believe we can come up with a generic solution that will work for all scenarios, the plumbing can be reused.  One thing I think would be better for csb is a level similar to how we have versions of the sdk for android.  This would allow for the manifest to do something like this:
skip-if = csb > 2 # bug xxxxxxx - this passes on csb 1 and 2, but strict mode fails

While the example solution might not be feasible or make sense, it gives us a single command line option and a single entity in the manifests that we can fiddle with.  That has a lot of value for simplicity.

The other longer term solution might be to use subsuites.  This would be similar to how we split out the devtools from browser-chrome.  These subsuites are filtered out while parsing the manifests.

Either way, what you have now is working and of no concern.  Lets keep this tangential randomness of review comments in mind as we move forward though :)

::: testing/mochitest/mach_commands.py
@@ +504,5 @@
>          help='Run tests with electrolysis preferences and test filtering enabled.')
>      func = this_chunk(func)
>  
> +    this_chunk = CommandArgument('--strict-content-sandbox', action='store_true',
> +        help='Run tests with a more strict content sandbox (Windows only).')

why are we assigning the variable this_chunk?  We use it above in --e10s and in general it seems to not make sense.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8533126 [details] [diff] [review]
Part 1: Change --content-sandbox mach / mochitest option to --strict-content-sandbox. v2

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

Tests work here for me on Win8.

Comment 8

[Armen [:armenzg]]

Comment on attachment 8528308 [details] [diff] [review]
Part 3: In buildbot config, change --content-sandbox mozharness option to --strict-content-sandbox.

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

AFAIK, this is only enabled on Holly so there's not much to worry about multiple branches and different configurations.

I have filed bug 1109139 to move this information into the tree.

Comment 9

[Armen [:armenzg]]

Comment on attachment 8528306 [details] [diff] [review]
Part 2: Change --content-sandbox mozharness option to --strict-content-sandbox.

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

Looks good!

Feel free to land. It will be merged to production and be live next time buildduty deploys it live.

It would have been very nice indeed to have met you!

Comment 10

[Bob Owen (:bobowen)]

Part 1 pushed to m-i:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/173bf96bed72

I'll wait until I'm sure this will land before I push the other two parts.

Comment 11

[Bob Owen (:bobowen)]

Part 2 pushed to mozharness default:
remote:   https://hg.mozilla.org/build/mozharness/rev/0a33dcc7043d

Comment 12

[Bob Owen (:bobowen)]

Part 3 push to buildbot-configs default:
remote:   https://hg.mozilla.org/build/buildbot-configs/rev/1a40e06b2054

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/173bf96bed72

Comment 14

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/buildbot-configs/rev/1a40e06b2054

Comment 15

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/0a33dcc7043d

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 17

[Bob Owen (:bobowen)]

This was completed by comment 15.

Comment 18

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.