Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Joshua Cohen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146582
---


Ship it!




This makes me a little nervous, but I don't think we're exposing ourselves to 
anything terrible by doing this.

- Joshua Cohen


On Aug. 23, 2016, 8:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 8:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Stephan Erb]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146578
---


Ship it!




Ship It!

- Stephan Erb


On Aug. 23, 2016, 10:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 10:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Stephan Erb]

> On Aug. 23, 2016, 11:20 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/helper.py, lines 104-109
> > 
> >
> > In the non-docker usecase, Thermos will by default run as uid 0 and 
> > setuid launched processes to the role user. 
> > 
> > This means, we will always satisfy the `uid ==0` condition here. This 
> > will make the entire function `this_is_really_our_pid` useless, as we will 
> > now happily kill everything.
> > 
> > To make this safe, we have to pass in the the uid that we where 
> > supposed to setuid to (i.e the role user in the default usecase). Only if 
> > this is `None`, we may enter the block you have added here.
> 
> Zameer Manji wrote:
> `uid` is not the uid of thermos. `uid` comes from the `RunnerHeader` 
> struct that thermos uses. This header is created by the runner when it is 
> first launched and it is the `uid` of the desired role. See 
> `_initialize_ckpt_header` in `thermos/core/runner.py`.

Got it.


- Stephan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146574
---


On Aug. 23, 2016, 10:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 10:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Zameer Manji]

> On Aug. 23, 2016, 2:20 p.m., Stephan Erb wrote:
> > src/main/python/apache/thermos/core/helper.py, lines 104-109
> > 
> >
> > In the non-docker usecase, Thermos will by default run as uid 0 and 
> > setuid launched processes to the role user. 
> > 
> > This means, we will always satisfy the `uid ==0` condition here. This 
> > will make the entire function `this_is_really_our_pid` useless, as we will 
> > now happily kill everything.
> > 
> > To make this safe, we have to pass in the the uid that we where 
> > supposed to setuid to (i.e the role user in the default usecase). Only if 
> > this is `None`, we may enter the block you have added here.

`uid` is not the uid of thermos. `uid` comes from the `RunnerHeader` struct 
that thermos uses. This header is created by the runner when it is first 
launched and it is the `uid` of the desired role. See `_initialize_ckpt_header` 
in `thermos/core/runner.py`.


- Zameer


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146574
---


On Aug. 23, 2016, 1:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 1:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Stephan Erb]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146574
---




src/main/python/apache/thermos/core/helper.py (lines 104 - 109)


In the non-docker usecase, Thermos will by default run as uid 0 and setuid 
launched processes to the role user. 

This means, we will always satisfy the `uid ==0` condition here. This will 
make the entire function `this_is_really_our_pid` useless, as we will now 
happily kill everything.

To make this safe, we have to pass in the the uid that we where supposed to 
setuid to (i.e the role user in the default usecase). Only if this is `None`, 
we may enter the block you have added here.


- Stephan Erb


On Aug. 23, 2016, 10:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 10:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51348: Fix thermos killing heuristic to permit setuid(2).

[Aurora ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51348/#review146571
---


Ship it!




Master (c115ac6) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Aug. 23, 2016, 8:45 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51348/
> ---
> 
> (Updated Aug. 23, 2016, 8:45 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1753
> https://issues.apache.org/jira/browse/AURORA-1753
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously this process killing heuristic would not allow killing of a process
> if the uid it was launched with differs from the real uid of the currently
> running process. The logic is too conservative because it doesn't factor in
> that a process launched as root can use `setuid(2)` to change it's real uid.
> 
> This patch fixes the heuristic by permitting killing of a process launched as
> root but the real uid is now not root.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/core/helper.py 
> dda40ed71bf8d26255cdb76eae29a5978a120d41 
>   src/test/python/apache/thermos/core/test_helper.py 
> 35397abd3ec769788f166088e5455c28bb120459 
> 
> Diff: https://reviews.apache.org/r/51348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>