Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

> On Sept. 2, 2016, 12:35 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 251
> > 
> >
> > You might extract this as a helper function 
> > (`_entity_exists(returncode: int): bool`) but definitely use a tuple 
> > instead of a list to represent the immutable set.  Extracting just the set 
> > would do the trick, ~:
> > ```python
> > # Return codes from `useradd` and `groupadd` calls.
> > _USER_OR_GROUP_EXISTS = (
> >   4,  # uid/gid already exists.
> >   9   # user/group name exists.
> > )
> > ```
> 
> John Sirois wrote:
> Zhitao - your call on this tweak.  I'll patch this change in since Joshua 
> is away on travel and has handed over responsibility for this review to the 3 
> of us who have shipited.
> 
> Zhitao Li wrote:
> Thanks for the review. For the sake of explicitness I'd like to keep them 
> separate, and open the gate if we need to handle them separately.

SGTM, I'll patch this in presently.


- John


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


On Sept. 2, 2016, 8:11 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 2, 2016, 8:11 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Zhitao Li]

> On Sept. 2, 2016, 6:35 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 251
> > 
> >
> > You might extract this as a helper function 
> > (`_entity_exists(returncode: int): bool`) but definitely use a tuple 
> > instead of a list to represent the immutable set.  Extracting just the set 
> > would do the trick, ~:
> > ```python
> > # Return codes from `useradd` and `groupadd` calls.
> > _USER_OR_GROUP_EXISTS = (
> >   4,  # uid/gid already exists.
> >   9   # user/group name exists.
> > )
> > ```
> 
> John Sirois wrote:
> Zhitao - your call on this tweak.  I'll patch this change in since Joshua 
> is away on travel and has handed over responsibility for this review to the 3 
> of us who have shipited.

Thanks for the review. For the sake of explicitness I'd like to keep them 
separate, and open the gate if we need to handle them separately.


- Zhitao


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


On Sept. 2, 2016, 2:11 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 2, 2016, 2:11 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

> On Sept. 2, 2016, 12:35 p.m., John Sirois wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 251
> > 
> >
> > You might extract this as a helper function 
> > (`_entity_exists(returncode: int): bool`) but definitely use a tuple 
> > instead of a list to represent the immutable set.  Extracting just the set 
> > would do the trick, ~:
> > ```python
> > # Return codes from `useradd` and `groupadd` calls.
> > _USER_OR_GROUP_EXISTS = (
> >   4,  # uid/gid already exists.
> >   9   # user/group name exists.
> > )
> > ```

Zhitao - your call on this tweak.  I'll patch this change in since Joshua is 
away on travel and has handed over responsibility for this review to the 3 of 
us who have shipited.


- John


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


On Sept. 2, 2016, 8:11 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 2, 2016, 8:11 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

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


Ship it!





src/main/python/apache/aurora/executor/common/sandbox.py (line 251)


You might extract this as a helper function (`_entity_exists(returncode: 
int): bool`) but definitely use a tuple instead of a list to represent the 
immutable set.  Extracting just the set would do the trick, ~:
```python
# Return codes from `useradd` and `groupadd` calls.
_USER_OR_GROUP_EXISTS = (
  4,  # uid/gid already exists.
  9   # user/group name exists.
)
```


- John Sirois


On Sept. 2, 2016, 8:11 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 2, 2016, 8:11 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Stephan Erb]

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


Ship it!




Ship it!

I am not a fan of extensive mocking. Unfortuantely, given the problem at hand, 
I don't have a much better idea either.

- Stephan Erb


On Sept. 2, 2016, 4:11 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 2, 2016, 4:11 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Aurora ReviewBot]

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



Master (bd11b1c) is red with this patch.
  ./build-support/jenkins/build.sh

23:49:08 00:00 [parse]
   Executing tasks in goals: compile
23:49:08 00:00   [compile]
23:49:08 00:00 [compile-prep-command]
23:49:08 00:00 [compile]
23:49:08 00:00 [python-eval]
23:49:08 00:00 [pythonstyle]
23:49:09 00:01   [cache]  
   No cached artifacts for 42 targets.
   Invalidated 42 targets.
T100:ERROR   src/main/python/apache/aurora/executor/common/sandbox.py:222-224 
Indentation of 3 instead of 2
 |   raise self.CreationError(
 |  'Group id result %s from task image does not match expected 
name %s and id %s' % (
 |  result, group_name, group_id))

E501:ERROR   
PythonFile(src/main/python/apache/aurora/executor/common/sandbox.py):270 line 
too long (101 > 100 characters)
 |  self._verify_user_match_in_taskfs(pwent.pw_uid, pwent.pw_name, 
pwent.pw_gid, grent.gr_name)


E501:ERROR   
PythonFile(src/test/python/apache/aurora/executor/common/test_sandbox.py):160 
line too long (108 > 100 characters)
 |mock_check_output.assert_called_with(['chroot', 
sandbox._task_fs_root, 'getent', 'group', 'test-group'])

E501:ERROR   
PythonFile(src/test/python/apache/aurora/executor/common/test_sandbox.py):272 
line too long (104 > 100 characters)
 
|@mock.patch('apache.aurora.executor.common.sandbox.FileSystemImageSandbox._verify_user_match_in_taskfs')

E501:ERROR   
PythonFile(src/test/python/apache/aurora/executor/common/test_sandbox.py):279 
line too long (105 > 100 characters)
 
|@mock.patch('apache.aurora.executor.common.sandbox.FileSystemImageSandbox._verify_group_match_in_taskfs')

E501:ERROR   
PythonFile(src/test/python/apache/aurora/executor/common/test_sandbox.py):286 
line too long (104 > 100 characters)
 
|@mock.patch('apache.aurora.executor.common.sandbox.FileSystemImageSandbox._verify_user_match_in_taskfs')

E501:ERROR   
PythonFile(src/test/python/apache/aurora/executor/common/test_sandbox.py):293 
line too long (105 > 100 characters)
 
|@mock.patch('apache.aurora.executor.common.sandbox.FileSystemImageSandbox._verify_group_match_in_taskfs')


FAILURE: 7 Python Style issues found


23:49:30 00:22   [complete]
   FAILURE


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

- Aurora ReviewBot


On Sept. 1, 2016, 11:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Sept. 1, 2016, 11:38 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Zhitao Li]

> On Aug. 31, 2016, 10:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
> I'm not sure about step 4 above. Are you referring to the [setuid in 
> process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
>  If so, that setuid shouldn't be switching to root, it will be switching to 
> the user matching the role name on the host system, the uid set in your 
> docker/appc image wouldn't have any impact on that. Am I missing something?
> 
> John Sirois wrote:
> Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
> uid/uname and gid/gname pairs in the chroot match those of the host system 
> when we hit an exists condition in either direction.
> 
> Given:
> Job author only specifies a role name, in this example `jsirois`
> 
> Scenarios:
> 1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
> 2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
> 3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
>  
> A Job author can have task code that references the role name, for 
> example it might shell out a call to `id -g jsirois` where the role name is 
> `jsirois` to find the primary group id for the current role.  It seems then 
> that we must ensure the chroot has the role name available, and fwict, 
> besides the special case of uid 0, we don't really care what the uid is.  If 
> it matches that's fine, but since the chroot environment will share nothing 
> with the host, ids need not match (IIUC).
> 
> So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  
> Scenario 2 though should fail (we currently pass).
> 
> John Sirois wrote:
> > Joshua mentioned this in Slack/IRC, but I do think we need to ensure 
> the uid/uname and gid/gname pairs in the chroot match those of the host 
> system when we hit an exists condition in either direction.
> 
> Obviously I changed my thinking as I composed the 
> Given/Scenarios/Conclusion below that statement!
> 
> Stephan Erb wrote:
> Excellent idea to spell out the scenarios explicitly.
> 
> [W]e don't really care what the uid is.  If it matches that's fine, 
> but since the chroot environment will share nothing with the host, ids need 
> not match (IIUC).
> 
> I would disagree here. With container mounts, the host and a container 
> can share parts of the filesystem. As filesystem permissions are based on 
> IDs, we have to make sure those match inside and outside of the container.
> 
> This implies that 'Scenario 3' would be the only acceptable one. We have 
> to abort the container launch in Scenario 1 and 2.
> 
> John Sirois wrote:
> I concur.  If we'll (Aurora or Mesos) supply mounts into the chroot then 
> 3 is the only acceptable scenario.

tl'dr +1 for checking uid/gid matches between the image filesystem and host for 
now.

This is a quite complex problem, and IMO one of the aspects that Linux 
containerization and cluster management layer generally haven't really solved 
well.

1. In our private use case, because we control all images running, we actually 
made special arrangements to make sure relevent users have the same uid/gid 
between image and host system;
2. The above solution clearly doesn't scale for people who don't 100% control 
their build. user namespace support should the savior here, but unfortunately 
it often conflicts with network namespace, therefore neither docker daemon nor 
mesos really support it well;
3. For your quote of `With container mounts, the host and a container can share 
parts of the filesystem. ` However, only certain parts we want the container to 
share with host system (pretty much everything mounted into the container) will 
indeed be shared. This also depending on how we want volume support for Mesos 
containerizer be.
4. Also, I think the risk you mentioned right now is also possible if people 
use DockerContainerizer and specify a malicious `--volume` argument value.


- Zhitao


---
This is an automatically generated e-mail. To reply, visit:

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

> On Aug. 31, 2016, 4:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
> I'm not sure about step 4 above. Are you referring to the [setuid in 
> process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
>  If so, that setuid shouldn't be switching to root, it will be switching to 
> the user matching the role name on the host system, the uid set in your 
> docker/appc image wouldn't have any impact on that. Am I missing something?
> 
> John Sirois wrote:
> Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
> uid/uname and gid/gname pairs in the chroot match those of the host system 
> when we hit an exists condition in either direction.
> 
> Given:
> Job author only specifies a role name, in this example `jsirois`
> 
> Scenarios:
> 1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
> 2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
> 3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
>  
> A Job author can have task code that references the role name, for 
> example it might shell out a call to `id -g jsirois` where the role name is 
> `jsirois` to find the primary group id for the current role.  It seems then 
> that we must ensure the chroot has the role name available, and fwict, 
> besides the special case of uid 0, we don't really care what the uid is.  If 
> it matches that's fine, but since the chroot environment will share nothing 
> with the host, ids need not match (IIUC).
> 
> So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  
> Scenario 2 though should fail (we currently pass).
> 
> John Sirois wrote:
> > Joshua mentioned this in Slack/IRC, but I do think we need to ensure 
> the uid/uname and gid/gname pairs in the chroot match those of the host 
> system when we hit an exists condition in either direction.
> 
> Obviously I changed my thinking as I composed the 
> Given/Scenarios/Conclusion below that statement!
> 
> Stephan Erb wrote:
> Excellent idea to spell out the scenarios explicitly.
> 
> [W]e don't really care what the uid is.  If it matches that's fine, 
> but since the chroot environment will share nothing with the host, ids need 
> not match (IIUC).
> 
> I would disagree here. With container mounts, the host and a container 
> can share parts of the filesystem. As filesystem permissions are based on 
> IDs, we have to make sure those match inside and outside of the container.
> 
> This implies that 'Scenario 3' would be the only acceptable one. We have 
> to abort the container launch in Scenario 1 and 2.

I concur.  If we'll (Aurora or Mesos) supply mounts into the chroot then 3 is 
the only acceptable scenario.


- John


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


On Aug. 31, 2016, 2:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 2:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Stephan Erb]

> On Sept. 1, 2016, 12:17 a.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
> I'm not sure about step 4 above. Are you referring to the [setuid in 
> process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
>  If so, that setuid shouldn't be switching to root, it will be switching to 
> the user matching the role name on the host system, the uid set in your 
> docker/appc image wouldn't have any impact on that. Am I missing something?
> 
> John Sirois wrote:
> Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
> uid/uname and gid/gname pairs in the chroot match those of the host system 
> when we hit an exists condition in either direction.
> 
> Given:
> Job author only specifies a role name, in this example `jsirois`
> 
> Scenarios:
> 1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
> 2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
> 3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
>  
> A Job author can have task code that references the role name, for 
> example it might shell out a call to `id -g jsirois` where the role name is 
> `jsirois` to find the primary group id for the current role.  It seems then 
> that we must ensure the chroot has the role name available, and fwict, 
> besides the special case of uid 0, we don't really care what the uid is.  If 
> it matches that's fine, but since the chroot environment will share nothing 
> with the host, ids need not match (IIUC).
> 
> So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  
> Scenario 2 though should fail (we currently pass).
> 
> John Sirois wrote:
> > Joshua mentioned this in Slack/IRC, but I do think we need to ensure 
> the uid/uname and gid/gname pairs in the chroot match those of the host 
> system when we hit an exists condition in either direction.
> 
> Obviously I changed my thinking as I composed the 
> Given/Scenarios/Conclusion below that statement!

Excellent idea to spell out the scenarios explicitly.

[W]e don't really care what the uid is.  If it matches that's fine, but 
since the chroot environment will share nothing with the host, ids need not 
match (IIUC).

I would disagree here. With container mounts, the host and a container can 
share parts of the filesystem. As filesystem permissions are based on IDs, we 
have to make sure those match inside and outside of the container.

This implies that 'Scenario 3' would be the only acceptable one. We have to 
abort the container launch in Scenario 1 and 2.


- Stephan


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


On Aug. 31, 2016, 10:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 10:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

> On Aug. 31, 2016, 4:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
> I'm not sure about step 4 above. Are you referring to the [setuid in 
> process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
>  If so, that setuid shouldn't be switching to root, it will be switching to 
> the user matching the role name on the host system, the uid set in your 
> docker/appc image wouldn't have any impact on that. Am I missing something?
> 
> John Sirois wrote:
> Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
> uid/uname and gid/gname pairs in the chroot match those of the host system 
> when we hit an exists condition in either direction.
> 
> Given:
> Job author only specifies a role name, in this example `jsirois`
> 
> Scenarios:
> 1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
> 2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
> 3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
>  
> A Job author can have task code that references the role name, for 
> example it might shell out a call to `id -g jsirois` where the role name is 
> `jsirois` to find the primary group id for the current role.  It seems then 
> that we must ensure the chroot has the role name available, and fwict, 
> besides the special case of uid 0, we don't really care what the uid is.  If 
> it matches that's fine, but since the chroot environment will share nothing 
> with the host, ids need not match (IIUC).
> 
> So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  
> Scenario 2 though should fail (we currently pass).

> Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
> uid/uname and gid/gname pairs in the chroot match those of the host system 
> when we hit an exists condition in either direction.

Obviously I changed my thinking as I composed the Given/Scenarios/Conclusion 
below that statement!


- John


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


On Aug. 31, 2016, 2:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 2:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[John Sirois]

> On Aug. 31, 2016, 4:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.
> 
> Joshua Cohen wrote:
> I'm not sure about step 4 above. Are you referring to the [setuid in 
> process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
>  If so, that setuid shouldn't be switching to root, it will be switching to 
> the user matching the role name on the host system, the uid set in your 
> docker/appc image wouldn't have any impact on that. Am I missing something?

Joshua mentioned this in Slack/IRC, but I do think we need to ensure the 
uid/uname and gid/gname pairs in the chroot match those of the host system when 
we hit an exists condition in either direction.

Given:
Job author only specifies a role name, in this example `jsirois`

Scenarios:
1. host (uid=1000, uname=jsirois) chroot (uid=500, uname=jsirois)
2. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=fred)
3. host (uid=1000, uname=jsirois) chroot (uid=1000, uname=jsirois)
 
A Job author can have task code that references the role name, for example it 
might shell out a call to `id -g jsirois` where the role name is `jsirois` to 
find the primary group id for the current role.  It seems then that we must 
ensure the chroot has the role name available, and fwict, besides the special 
case of uid 0, we don't really care what the uid is.  If it matches that's 
fine, but since the chroot environment will share nothing with the host, ids 
need not match (IIUC).

So it seems to me scenarios 1 and 3 are OK - the sandbox can move along.  
Scenario 2 though should fail (we currently pass).


- John


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


On Aug. 31, 2016, 2:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 2:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Joshua Cohen]

> On Aug. 31, 2016, 10:17 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, line 239
> > 
> >
> > This changes seems to come with a severe security risk. As an normal 
> > user, I can now gain root on any agent:
> > 
> > * Prepare a docker/appc container with a manually crafted user with UID 
> > 0 but with my role name.
> > * Launch the container with said role name.
> > * The sandbox code will bail out early here and don't proceed to create 
> > an unpriviledged user
> > * Setuid will switch from root to my prepare custom user with root 
> > permissions
> > * Game over  
> > 
> > Unless someone can correct me here, that would be a -1 from my end.

I'm not sure about step 4 above. Are you referring to the [setuid in 
process.py](https://github.com/apache/aurora/blob/master/src/main/python/apache/thermos/core/process.py#L369-L380)?
 If so, that setuid shouldn't be switching to root, it will be switching to the 
user matching the role name on the host system, the uid set in your docker/appc 
image wouldn't have any impact on that. Am I missing something?


- Joshua


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


On Aug. 31, 2016, 8:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Stephan Erb]

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




src/main/python/apache/aurora/executor/common/sandbox.py (line 239)


This changes seems to come with a severe security risk. As an normal user, 
I can now gain root on any agent:

* Prepare a docker/appc container with a manually crafted user with UID 0 
but with my role name.
* Launch the container with said role name.
* The sandbox code will bail out early here and don't proceed to create an 
unpriviledged user
* Setuid will switch from root to my prepare custom user with root 
permissions
* Game over  

Unless someone can correct me here, that would be a -1 from my end.


- Stephan Erb


On Aug. 31, 2016, 10:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 10:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Aurora ReviewBot]

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


Ship it!




Master (bd11b1c) 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. 31, 2016, 8:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Zameer Manji]

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


Ship it!




Ship It!

- Zameer Manji


On Aug. 31, 2016, 1:56 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 1:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Zhitao Li]

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

(Updated Aug. 31, 2016, 8:56 p.m.)


Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.


Changes
---

Comment line length fix.


Bugs: AURORA-1761
https://issues.apache.org/jira/browse/AURORA-1761


Repository: aurora


Description
---

Allow E_NAME_IN_USE in useradd/groupadd.


Diffs (updated)
-

  src/main/python/apache/aurora/executor/common/sandbox.py 
a172691e164cf64792f65f049d698f9758336542 
  src/test/python/apache/aurora/executor/common/test_sandbox.py 
57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 

Diff: https://reviews.apache.org/r/51564/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Aurora ReviewBot]

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



Master (bd11b1c) is red with this patch.
  ./build-support/jenkins/build.sh

  Running setup.py bdist_wheel for twitter.common.lang: finished with status 
'done'
  Stored in directory: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/.home/.cache/pip/wheels/b4/91/6b/e0a5ae9722528a67f600d436e0b97b30a040a65a5a35978272
  Running setup.py bdist_wheel for twitter.common.log: started
  Running setup.py bdist_wheel for twitter.common.log: finished with status 
'done'
  Stored in directory: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/.home/.cache/pip/wheels/26/36/82/e6a6b7ca739262435cecee1cde7950bf60950f1aaa2ce21093
  Running setup.py bdist_wheel for cov-core: started
  Running setup.py bdist_wheel for cov-core: finished with status 'done'
  Stored in directory: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/.home/.cache/pip/wheels/86/e1/c2/9ff8cfe9773ce07003f2c2be096e169af4614c2f634671d49b
  Running setup.py bdist_wheel for twitter.common.options: started
  Running setup.py bdist_wheel for twitter.common.options: finished with status 
'done'
  Stored in directory: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/.home/.cache/pip/wheels/60/c2/40/54b323809df9598cc125f02527f93ff743cd9bd979f4a1737d
Successfully built pantsbuild.pants ansicolors setproctitle 
twitter.common.collections pathspec twitter.common.dirutil pystache scandir 
psutil pywatchman Markdown Pygments docutils twitter.common.confluence coverage 
pytest pytest-cov lmdb twitter.common.lang twitter.common.log cov-core 
twitter.common.options
Installing collected packages: ansicolors, setproctitle, twitter.common.lang, 
twitter.common.collections, six, pathspec, twitter.common.dirutil, requests, 
pystache, scandir, psutil, pywatchman, futures, setuptools, pex, Markdown, 
Pygments, docutils, twitter.common.options, twitter.common.log, 
twitter.common.confluence, monotonic, fasteners, coverage, py, pytest, 
cov-core, pytest-cov, lmdb, pantsbuild.pants
  Found existing installation: setuptools 21.2.1
Uninstalling setuptools-21.2.1:
  Successfully uninstalled setuptools-21.2.1
Successfully installed Markdown-2.1.1 Pygments-1.4 ansicolors-1.0.2 
cov-core-1.15.0 coverage-3.7.1 docutils-0.12 fasteners-0.14.1 futures-3.0.5 
lmdb-0.89 monotonic-1.2 pantsbuild.pants-1.1.0rc7 pathspec-0.3.4 pex-1.1.10 
psutil-4.3.0 py-1.4.31 pystache-0.5.3 pytest-2.6.4 pytest-cov-1.8.1 
pywatchman-1.3.0 requests-2.5.3 scandir-1.2 setproctitle-1.1.10 
setuptools-5.4.1 six-1.10.0 twitter.common.collections-0.3.7 
twitter.common.confluence-0.3.7 twitter.common.dirutil-0.3.7 
twitter.common.lang-0.3.7 twitter.common.log-0.3.7 twitter.common.options-0.3.7

20:48:27 00:00 [main]
   (To run a reporting server: ./pants server)
20:48:27 00:00   [setup]
20:48:27 00:00 [parse]
   Executing tasks in goals: compile
20:48:28 00:01   [compile]
20:48:28 00:01 [compile-prep-command]
20:48:28 00:01 [compile]
20:48:28 00:01 [python-eval]
20:48:28 00:01 [pythonstyle]
20:48:28 00:01   [cache]  
   No cached artifacts for 42 targets.
   Invalidated 42 targets.
E501:ERROR   
PythonFile(src/main/python/apache/aurora/executor/common/sandbox.py):193 line 
too long (102 > 100 characters)
 |  # returncode from a `useradd` or `groupadd` call indicating that the 
user/group name already exists.


FAILURE: 1 Python Style issues found


20:48:46 00:19   [complete]
   FAILURE


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

- Aurora ReviewBot


On Aug. 31, 2016, 8:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 8:38 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Zhitao Li]

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

(Updated Aug. 31, 2016, 8:38 p.m.)


Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.


Changes
---

Fix the test.


Bugs: AURORA-1761
https://issues.apache.org/jira/browse/AURORA-1761


Repository: aurora


Description
---

Allow E_NAME_IN_USE in useradd/groupadd.


Diffs (updated)
-

  src/main/python/apache/aurora/executor/common/sandbox.py 
a172691e164cf64792f65f049d698f9758336542 
  src/test/python/apache/aurora/executor/common/test_sandbox.py 
57ab39e2444100c3a689bb0ff745c62f7bc2f1a6 

Diff: https://reviews.apache.org/r/51564/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Aurora ReviewBot]

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



Master (bd11b1c) is red with this patch.
  ./build-support/jenkins/build.sh

 
.pants.d/python-setup/chroots/ba7cfc27699a9911c627544a2439af78a22bc2df/.deps/mock-1.0.1-py2-none-any.whl/mock.py:1618:
 in _inner
 return f(*args, **kw)
 
src/test/python/apache/aurora/executor/common/test_sandbox.py:190: in 
test_uid_exists
 assert_create_user_and_group(mock_check_call, 
False, True)
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 mock_check_call = 
 gid_exists = False, uid_exists = True
 
 def assert_create_user_and_group(mock_check_call, 
gid_exists, uid_exists):
   mock_pwent = pwd.struct_passwd((
 'someuser',# login name
 'hunter2', # password
 834,   # uid
 835,   # gid
 'Some User',   # user name
 '/home/someuser',  # home directory
 '/bin/sh'))# login shell
 
   mock_grent = grp.struct_group((
 'users',# group name
 '*',# password
 835,# gid
 ['someuser']))  # members
 
   exception = subprocess.CalledProcessError(
 > 
returncode=FileSystemImageSandbox._USER_OR_GROUP_ID_EXISTS,
   cmd='some command',
   output=None)
 E AttributeError: type object 
'FileSystemImageSandbox' has no attribute '_USER_OR_GROUP_ID_EXISTS'
 
 
src/test/python/apache/aurora/executor/common/test_sandbox.py:161: 
AttributeError
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  2 failed, 695 passed, 6 skipped, 1 warnings in 
218.37 seconds 
 
FAILURE


19:43:41 04:06   [complete]
   FAILURE


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

- Aurora ReviewBot


On Aug. 31, 2016, 7:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 7:29 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51564: Allow E_NAME_IN_USE in useradd/groupadd.

[Joshua Cohen]

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



Please add a test case to cover this (can use the existing tests for uid/gid 
exists as a reference).

- Joshua Cohen


On Aug. 31, 2016, 7:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51564/
> ---
> 
> (Updated Aug. 31, 2016, 7:29 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, and Zameer Manji.
> 
> 
> Bugs: AURORA-1761
> https://issues.apache.org/jira/browse/AURORA-1761
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow E_NAME_IN_USE in useradd/groupadd.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> a172691e164cf64792f65f049d698f9758336542 
> 
> Diff: https://reviews.apache.org/r/51564/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>