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/07 03:33:01 UTC
Review Request 58253: Added a ContainerID to 'ObjectApprover::Object'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/
-----------------------------------------------------------
Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-7014
https://issues.apache.org/jira/browse/MESOS-7014
Repository: mesos
Description
-------
This patch adds a new member, `container_id` to the
`ObjectApprover::Object` to facilitate implicit executor
authorization.
Diffs
-----
include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
Diff: https://reviews.apache.org/r/58253/diff/1/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171480
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 7, 2017, 3:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 11, 2017, 9:28 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 57 (patched)
> > <https://reviews.apache.org/r/58253/diff/1/?file=1686347#file1686347line57>
> >
> > Good question. I wonder if there's a more generic data structure (not `ContainerInfo`, I guess) that we could use that still includes ContainerID. `ContainerState` looks promising, but I'm not sure if we have all of that when we're trying to authorize.
> > Can you elaborate on what (new) action will use this object type, and how, and from where?
The LAUNCH_NESTED_CONTAINER, LAUNCH_NESTED_CONTAINER_SESSION, WAIT_NESTED_CONTAINER, KILL_NESTED_CONTAINER, and REMOVE_NESTED_CONTAINER actions will use this member. The `ContainerID` is added to the `authorization::Object` in the handlers for those actions in 'src/slave/http.cpp'. It looks like `ContainerState` is a message used only internally within the containerizer... there is a helper in the protobuf utils to generate one, but it takes a few arguments like the container's init process PID and sandbox directory that the handler would not easily have access to. I'm open to other ideas, but I don't think I see any other options that seem suitable.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171542
-----------------------------------------------------------
On April 7, 2017, 3:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171542
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto
Lines 57 (patched)
<https://reviews.apache.org/r/58253/#comment244485>
Good question. I wonder if there's a more generic data structure (not `ContainerInfo`, I guess) that we could use that still includes ContainerID. `ContainerState` looks promising, but I'm not sure if we have all of that when we're trying to authorize.
Can you elaborate on what (new) action will use this object type, and how, and from where?
- Adam B
On April 6, 2017, 8:33 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 6, 2017, 8:33 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171327
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto
Lines 57 (patched)
<https://reviews.apache.org/r/58253/#comment244233>
I was thinking that instead of having one field `container_id`, why not having a map of claims, then you can verify that each claim made by the subject matches the claims in the object whithout needing to know the supported claims in advance.
Limiting the fields is what lead to the whole redising of the object in the first place, from a `string value` to suport the info objects.
- Alexander Rojas
On April 7, 2017, 5:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 5:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On April 8, 2017, 1:30 a.m., Greg Mann wrote:
> > For some reason I'm having trouble replying to your previous comment, so I'll post a new one :)
> >
> > I think that it makes sense to have claims in the `authorization::Subject`, since this maps directly onto the `Principal` provided by the client. However, in the case of the `authorization::Object`, I don't think that the agent should dictate the use of particular claims there. For example, a custom authorizer might have a different way to determine which `ContainerID`s a principal should be able to launch containers within. I don't think that we should leak the specific claim keys used by the `SecretGenerator` into the `authorization::Object`, since in the future we will make the `SecretGenerator` modular and the claims within the executor token could be different for a custom generator. Does that make sense?
your anser does make sense to me.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171388
-----------------------------------------------------------
On April 7, 2017, 5:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 5:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58253: Added a ContainerID to
'ObjectApprover::Object'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58253/#review171388
-----------------------------------------------------------
For some reason I'm having trouble replying to your previous comment, so I'll post a new one :)
I think that it makes sense to have claims in the `authorization::Subject`, since this maps directly onto the `Principal` provided by the client. However, in the case of the `authorization::Object`, I don't think that the agent should dictate the use of particular claims there. For example, a custom authorizer might have a different way to determine which `ContainerID`s a principal should be able to launch containers within. I don't think that we should leak the specific claim keys used by the `SecretGenerator` into the `authorization::Object`, since in the future we will make the `SecretGenerator` modular and the claims within the executor token could be different for a custom generator. Does that make sense?
- Greg Mann
On April 7, 2017, 3:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 3:33 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.hpp 75801ccc753a60ce5e5979b6723fd2294ce7ffe5
> include/mesos/authorizer/authorizer.proto 736f76d552956f2351ffd40fc51d088dff83f8c8
> src/authorizer/local/authorizer.cpp e241edf4afa48d35dbbbb94d72e8e8690f5bedfc
>
>
> Diff: https://reviews.apache.org/r/58253/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>