763497 - Need to be able to tell SUTAgent to execute commands as privileged user

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Comment 1

[William Lachance (:wlach)]

Eugh, filing fail.

In discussing bug 754873, it came out that some commands that we might like to execute on a device require root permission (besides logcat, screencap, monkey, and my new orangatun come to mind). While theoretically you can do this with devicemanagerSUT by passing su -c to the command, in practice the quoting rules are annoying and difficult to get right. It would be better if we just executed things as root inside the agent.

Comment 2

[William Lachance (:wlach)]

So jmaher made devicemanagerSUT pass commands as root, and I fixed up the quoting some in bug 772595. However, it looks like we still want this, as running "su" resets the environment, which will cause commands like "am" and "logcat" to segfault on ICS-based devices like the Galaxy Nexus and Pandaboards.

Comment 3

[William Lachance (:wlach)]

Adding a few more folks who might care about this.

Comment 4

[William Lachance (:wlach)]

Attachment: Patch to make exec use su by default

This was actually remarkably easy to implement and seems to work well for the basic cases I've tested (getting logcat, running fennec).

Joel: Is there a way we could test this new agent with reftest, mochitest, etc. to make sure nothing breaks before I land this? I'm pretty positive it's working well.

Comment 5

[Justin Wood (:Callek)]

(In reply to William Lachance (:wlach) from comment #4)
> Joel: Is there a way we could test this new agent with reftest, mochitest,
> etc. to make sure nothing breaks before I land this? I'm pretty positive
> it's working well.

I can deploy this to a subset of tegras if you provide the SUTAgent apk (with the same version would be easiest to test small number of tegras with, hardest to deploy/rollback -- with newer version most annoying to test with [because we only have so much space for staging] but easiest to rollback/deploy)

I'll leave it to joel if he wants us to stage this seperately from a whole SUTagent upgrade.

Comment 6

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

Comment on attachment 640835 [details] [diff] [review]
Patch to make exec use su by default

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

hmm, would we need to remove the  devicemanager part of this?

Comment 7

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

regarding sutagent rollout, I would prefer to do a local staging run.  My biggest gripe about a staging run is we (as in myself or wlach) do not have a way to determine what is running the new agent and how to track the runs that it does.  Also this would need to be in production instead of the 'staging' pool as history proves that staging pool is less reliable than production.

Comment 8

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #6)
> Comment on attachment 640835 [details] [diff] [review]
> Patch to make exec use su by default
> 
> Review of attachment 640835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hmm, would we need to remove the  devicemanager part of this?

That would be the ultimate goal (since it will fix the use of shell() on ICS-based devices), but in my limited testing I haven't seen a need to land both changse simultaneously. We should be able to update the agent, then land a patch to dmSUT later.

Comment 9

[William Lachance (:wlach)]

Ok, sounds like this should be ok. We'll test this change along with whatever else in staging before deployment, of course.

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8c1913e669

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d8c1913e669

Comment 11

[William Lachance (:wlach)]

So in further testing, it looks like the way we quote in devicemanager (") breaks when passed down to "su" (it just uses the first part of the command). The ideal solution would be to just remove the call to su altogether, but we can't do that until we're using the new agent in production. Attaching another patch to use the old-style quoting, which seems to work great for both launching fennec and doing a logcat (the two main things we are currently using exec for)

Comment 12

[William Lachance (:wlach)]

Attachment: Use a different quoting strategy when calling exec with su

Followup patch to use a differnet quoting strategy.

Comment 13

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #12)
> Created attachment 643623 [details] [diff] [review]
> Use a different quoting strategy when calling exec with su
> 
> Followup patch to use a differnet quoting strategy.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=c05cad2a9246

Comment 14

[William Lachance (:wlach)]

Ok, actually did some testing myself and found that this patch breaks logcat when using the old agent. :( I think the right thing here is to suffer through the old behaviour for now, wait for a new agent with my patch applied to be deployed, then remove the "su -c" part entirely. For now, the one thing that we need su for (logcat) seems to work ok with both versions of the agent.

Comment 15

[William Lachance (:wlach)]

Attachment: Patch to remove su call from devicemanagerSUT

Followup patch. Shouldn't be applied until we're using a new devicemanagerSUT

Comment 16

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

Comment on attachment 643649 [details] [diff] [review]
Patch to remove su call from devicemanagerSUT

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

nice!

Comment 17

[Armen [:armenzg]]

Blocking bug 769428 since it tracks stability improvements for the panda images.

Sorry for the noise.

Comment 18

[William Lachance (:wlach)]

Reverted patch to sutagent: https://hg.mozilla.org/integration/mozilla-inbound/rev/a672c266059f

It was causing problems with pandas and tegras because of the way their su command works. For the pandas, we need to patch "su" to not prompt by default (example of this here: https://github.com/wlach/orangutan/blob/master/su.c). I am not sure what the problem is on the pandas, will have to debug further.

Comment 19

[William Lachance (:wlach)]

Moving to Testing/SUTAgent.

Comment 21

[(no longer active)]

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

Comment 22

[William Lachance (:wlach)]

Attachment: Patch to add optional "execsu" command

Ok, so it's looking like my earlier strategy caused breakage on the tegras or pandas or something (feedback from jmaher). I suspect the issue is with quoting, though I'm not sure. Unfortunately we just don't have time or resources to properly debug that.

Let's take a different approach: add variants of the "exec" command that execute all commands as root, and make it possible to specify their use by passing a special argument to shell(). This should allow us to get access to these special privileges without trouble where we need it, and preserve backwards compatibility where required.

Comment 23

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

Comment on attachment 655595 [details] [diff] [review]
Patch to add optional "execsu" command

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

this approach is fine.  I haven't tested this, but you could test this on try and later today or tomorrow I could run it on my circuit board farm.

::: build/mobile/devicemanagerSUT.py
@@ +271,5 @@
> +    cmd = "exec"
> +    if cwd:
> +        cmd += "cwd"
> +    if root:
> +        cmd += "su"

please document that we are executing: 'exec', 'execsu', 'execcwd' or 'execcwdsu'

Comment 24

[Mozilla RelEng Bot]

Try run for 128a0a829aaa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=128a0a829aaa
Results (out of 2 total builds):
    exception: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-128a0a829aaa

Comment 25

[William Lachance (:wlach)]

Attachment: 4th and hopefully final take on the patch

This patch adds the comment requested in the review, and also backwards compatibility with the old agent (so we don't have to gate committing this code on an updated agent)

Comment 26

[William Lachance (:wlach)]

Doing a try run: https://tbpl.mozilla.org/?tree=Try&rev=4b2f7621cf96

Comment 27

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

Comment on attachment 655607 [details] [diff] [review]
4th and hopefully final take on the patch

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

a few logic issues in here and a few nits.

::: build/mobile/devicemanagerSUT.py
@@ +55,5 @@
>      if self.getDeviceRoot() == None:
> +      raise BaseException("Failed to connect to SUT Agent and retrieve the device root.")
> +    try:
> +      verstring = self.runCmds([{ 'cmd': 'ver' }])
> +      self.agentVersion = re.sub('SUTAgentAndroid Version ', '', verstring)

does this have any \n at the end or spaces we need to look out for?

@@ +273,5 @@
>      cmdline = self._escapedCommandLine(cmd)
>      if env:
>        cmdline = '%s %s' % (self.formatEnvString(env), cmdline)
>  
> +    haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12'))

1.13, 1.12 is already deployed

@@ +291,3 @@
>      try:
>        if cwd:
> +        self.sendCmds([{ 'cmd': '%s %s %s' % (cmd, cwd, cmdline) }], outputfile, timeout)

what about execcwdsu?  That should be limited to version 1.13 or higher.

@@ +294,3 @@
>        else:
> +        if not root or haveExecSu:
> +          self.sendCmds([{ 'cmd': '%s %s"' % (cmd, cmdline) }], outputfile, timeout)

this will always execute if root=True, that won't work for backwards compatibility.

::: build/mobile/sutagent/android/DoCommand.java
@@ +3820,4 @@
>              "run [cmdline]                - start program no wait\n" +
>              "exec [env pairs] [cmdline]   - start program no wait optionally pass env\n" +
>              "                               key=value pairs (comma separated)\n" +
> +            "execsu [env pairs] [cmdline] - start program as privileged user\n" +

can you add execcwdsu ?

Comment 28

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #27)
> Comment on attachment 655607 [details] [diff] [review]
> 4th and hopefully final take on the patch
> 
> Review of attachment 655607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few logic issues in here and a few nits.
> 
> ::: build/mobile/devicemanagerSUT.py
> @@ +55,5 @@
> >      if self.getDeviceRoot() == None:
> > +      raise BaseException("Failed to connect to SUT Agent and retrieve the device root.")
> > +    try:
> > +      verstring = self.runCmds([{ 'cmd': 'ver' }])
> > +      self.agentVersion = re.sub('SUTAgentAndroid Version ', '', verstring)
> 
> does this have any \n at the end or spaces we need to look out for?

No.

>>> import droid
>>> dm = droid.DroidSUT('192.168.1.11')
reconnecting socket
>>> dm.agentVersion
'1.11'
>>> dm.runCmds([{ 'cmd': 'ver' }])
'SUTAgentAndroid Version 1.11'


> @@ +273,5 @@
> >      cmdline = self._escapedCommandLine(cmd)
> >      if env:
> >        cmdline = '%s %s' % (self.formatEnvString(env), cmdline)
> >  
> > +    haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12'))
> 
> 1.13, 1.12 is already deployed

Oops. Will fix.

> @@ +291,3 @@
> >      try:
> >        if cwd:
> > +        self.sendCmds([{ 'cmd': '%s %s %s' % (cmd, cwd, cmdline) }], outputfile, timeout)
> 
> what about execcwdsu?  That should be limited to version 1.13 or higher.

It is, if you look at the diff. The command is created by concatenating the string information, and 'su' will only be appended if we are running the required version.

> @@ +294,3 @@
> >        else:
> > +        if not root or haveExecSu:
> > +          self.sendCmds([{ 'cmd': '%s %s"' % (cmd, cmdline) }], outputfile, timeout)
> 
> this will always execute if root=True, that won't work for backwards
> compatibility.

If my understanding of boolean logic and python is still correct, that's not true. :) 

>>> haveExecSu = False
>>> root = True
>>> not root or haveExecSu
False

I think maybe you're thinking that the not gets applied to the whole expression, when it only gets applied to the first one. We could rewrite it as "if (not root) or haveExecSu" if it makes things clearer for you. I normally hate unnecessary parens, but I think it might be ok in this one case.

> ::: build/mobile/sutagent/android/DoCommand.java
> @@ +3820,4 @@
> >              "run [cmdline]                - start program no wait\n" +
> >              "exec [env pairs] [cmdline]   - start program no wait optionally pass env\n" +
> >              "                               key=value pairs (comma separated)\n" +
> > +            "execsu [env pairs] [cmdline] - start program as privileged user\n" +
> 
> can you add execcwdsu ?

Sure. I'll add execcwd at the same time.

Comment 29

[William Lachance (:wlach)]

Attachment: Fix issues pointed out by review

Two key changes:
* Change `if not root or haveExecSu` in dmSUT's shell to `if (not root) or haveExecSu` for clarity
* Updated help in SUTAgent for execcwd and execcwdsu (had to reformat stuff so it would still fit)

Comment 30

[Mozilla RelEng Bot]

Try run for 4b2f7621cf96 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4b2f7621cf96
Results (out of 19 total builds):
    success: 17
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4b2f7621cf96

Comment 31

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

Comment on attachment 655640 [details] [diff] [review]
Fix issues pointed out by review

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

ok, I am finally getting rid of the cobwebs that are causing me to misunderstand this, looks good.

::: build/mobile/devicemanagerSUT.py
@@ +285,5 @@
> +    cmd = "exec"
> +    if cwd:
> +        cmd += "cwd"
> +    if root and haveExecSu:
> +        cmd += "su"

nit: 2 space indent to match the file.

Comment 32

[William Lachance (:wlach)]

Attachment: Additional fixes

Unfortunately I found some additional problems in testing this, in that quoted sections of the output (e.g. "foo bar baz") would get lost when run through su. This patch fixes that, along with the nits pointed out by jmaher last time. I also fixed some formatting goofs in the help.

Comment 33

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

Comment on attachment 656178 [details] [diff] [review]
Additional fixes

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

we just might get this landed!

::: build/mobile/devicemanagerSUT.py
@@ +273,5 @@
>      cmdline = self._escapedCommandLine(cmd)
>      if env:
>        cmdline = '%s %s' % (self.formatEnvString(env), cmdline)
>  
> +    haveExecSu = (StrictVersion(self.agentVersion) >= StrictVersion('1.12'))

1.13

::: build/mobile/sutagent/android/DoCommand.java
@@ +117,2 @@
>          EXECCWD ("execcwd"),
> +        EXECCWDSU ("execcwd"),

should this be execcwdsu?

Comment 34

[William Lachance (:wlach)]

Inbound with jmaher's fixes, plus a few more minor issues (stray quote, formatting problems) fixed: 

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

Comment 35

[William Lachance (:wlach)]

Renaming bug for clarity.

Comment 36

[William Lachance (:wlach)]

Pushed to mozbase as well: https://github.com/mozilla/mozbase/commit/e5c30fe84221a0e5b92a10b73299fc6d04e688e5

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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