You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/04/13 20:21:59 UTC
Review Request 58428: Added tests for failed executor authorization.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch adds new tests to verify that HTTP executors cannot
subscribe or launch nested containers when HTTP executor
authentication is enabled, authorization is enabled, and they
do not provide a valid executor authentication token
Diffs
-----
src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
Diff: https://reviews.apache.org/r/58428/diff/1/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 19, 2017, 12:33 a.m., Vinod Kone wrote:
> > While these tests are good I'm wondering if they are realistic, because the assumption is that someone knows the agent's secret key but doesn't know the container id of the executor they want to attack. In reality it's the opposite; they know the container id of the executor they want to attack but not the agent's key. Just a thought.
True, it's a contrived scenario, but it's the only way for us to test the authorization logic. A token generated with an incorrect key will fail the authentication step. We don't have any end-to-end authentication tests for HTTP executors, so we could add those instead of/in addition to these tests.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/#review172293
-----------------------------------------------------------
On April 14, 2017, 9:18 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58428/
> -----------------------------------------------------------
>
> (Updated April 14, 2017, 9:18 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds new tests to verify that HTTP executors cannot
> subscribe or launch nested containers when HTTP executor
> authentication is enabled, authorization is enabled, and they
> do not provide a valid executor authentication token
>
>
> Diffs
> -----
>
> src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
>
>
> Diff: https://reviews.apache.org/r/58428/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 19, 2017, 12:33 a.m., Vinod Kone wrote:
> > src/tests/slave_authorization_tests.cpp
> > Lines 1070 (patched)
> > <https://reviews.apache.org/r/58428/diff/3/?file=1692836#file1692836line1070>
> >
> > don't need terminate/wait slave here like the above test?
Nope, this test uses `cluster::Slave` instead of a spawned `MockSlave`, so the cleanup is done automatically in the destructor.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/#review172293
-----------------------------------------------------------
On April 19, 2017, 6:48 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58428/
> -----------------------------------------------------------
>
> (Updated April 19, 2017, 6:48 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds new tests to verify that HTTP executors cannot
> subscribe or launch nested containers when HTTP executor
> authentication is enabled, authorization is enabled, and they
> do not provide a valid executor authentication token
>
>
> Diffs
> -----
>
> src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
>
>
> Diff: https://reviews.apache.org/r/58428/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/#review172293
-----------------------------------------------------------
Fix it, then Ship it!
While these tests are good I'm wondering if they are realistic, because the assumption is that someone knows the agent's secret key but doesn't know the container id of the executor they want to attack. In reality it's the opposite; they know the container id of the executor they want to attack but not the agent's key. Just a thought.
src/tests/slave_authorization_tests.cpp
Lines 28-30 (original), 30-34 (patched)
<https://reviews.apache.org/r/58428/#comment245396>
order alphabetically.
src/tests/slave_authorization_tests.cpp
Line 32 (original), 36-37 (patched)
<https://reviews.apache.org/r/58428/#comment245397>
order alphabetically.
src/tests/slave_authorization_tests.cpp
Lines 739 (patched)
<https://reviews.apache.org/r/58428/#comment245399>
s/badRequest/error/
src/tests/slave_authorization_tests.cpp
Lines 1070 (patched)
<https://reviews.apache.org/r/58428/#comment245400>
don't need terminate/wait slave here like the above test?
- Vinod Kone
On April 14, 2017, 9:18 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58428/
> -----------------------------------------------------------
>
> (Updated April 14, 2017, 9:18 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds new tests to verify that HTTP executors cannot
> subscribe or launch nested containers when HTTP executor
> authentication is enabled, authorization is enabled, and they
> do not provide a valid executor authentication token
>
>
> Diffs
> -----
>
> src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
>
>
> Diff: https://reviews.apache.org/r/58428/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/#review172687
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 19, 2017, 6:48 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58428/
> -----------------------------------------------------------
>
> (Updated April 19, 2017, 6:48 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds new tests to verify that HTTP executors cannot
> subscribe or launch nested containers when HTTP executor
> authentication is enabled, authorization is enabled, and they
> do not provide a valid executor authentication token
>
>
> Diffs
> -----
>
> src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
>
>
> Diff: https://reviews.apache.org/r/58428/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/
-----------------------------------------------------------
(Updated April 19, 2017, 6:48 a.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch adds new tests to verify that HTTP executors cannot
subscribe or launch nested containers when HTTP executor
authentication is enabled, authorization is enabled, and they
do not provide a valid executor authentication token
Diffs (updated)
-----
src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
Diff: https://reviews.apache.org/r/58428/diff/4/
Changes: https://reviews.apache.org/r/58428/diff/3-4/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/
-----------------------------------------------------------
(Updated April 14, 2017, 9:18 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch adds new tests to verify that HTTP executors cannot
subscribe or launch nested containers when HTTP executor
authentication is enabled, authorization is enabled, and they
do not provide a valid executor authentication token
Diffs (updated)
-----
src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
Diff: https://reviews.apache.org/r/58428/diff/3/
Changes: https://reviews.apache.org/r/58428/diff/2-3/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/
-----------------------------------------------------------
(Updated April 14, 2017, 9:16 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch adds new tests to verify that HTTP executors cannot
subscribe or launch nested containers when HTTP executor
authentication is enabled, authorization is enabled, and they
do not provide a valid executor authentication token
Diffs (updated)
-----
src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
Diff: https://reviews.apache.org/r/58428/diff/2/
Changes: https://reviews.apache.org/r/58428/diff/1-2/
Testing
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 58428: Added tests for failed executor
authorization.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58428/#review171942
-----------------------------------------------------------
Patch looks great!
Reviews applied: [58327, 58328, 58251, 58252, 58253, 58254, 58255, 58258, 58428]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 13, 2017, 8:21 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58428/
> -----------------------------------------------------------
>
> (Updated April 13, 2017, 8:21 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds new tests to verify that HTTP executors cannot
> subscribe or launch nested containers when HTTP executor
> authentication is enabled, authorization is enabled, and they
> do not provide a valid executor authentication token
>
>
> Diffs
> -----
>
> src/tests/slave_authorization_tests.cpp 3657e0a3d19d75cef92e5bf90b65ef00c291b032
>
>
> Diff: https://reviews.apache.org/r/58428/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>