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:45:48 UTC
Review Request 58255: Added implicit authorization to the agent
executor API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs
-----
src/slave/http.cpp b07ce7c73a90ef297d980806ebba9530d86f25ae
Diff: https://reviews.apache.org/r/58255/diff/1/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review172290
-----------------------------------------------------------
src/slave/http.cpp
Lines 629-650 (patched)
<https://reviews.apache.org/r/58255/#comment245395>
Can you make the response status codes consistent or add a comment on why they are inconsistent?
- Vinod Kone
On April 18, 2017, 7:58 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 7:58 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/6/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review172686
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 19, 2017, 12:34 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 19, 2017, 12:34 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/7/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 19, 2017, 12:34 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/7/
Changes: https://reviews.apache.org/r/58255/diff/6-7/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 18, 2017, 7:58 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/6/
Changes: https://reviews.apache.org/r/58255/diff/5-6/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 18, 2017, 7:29 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/5/
Changes: https://reviews.apache.org/r/58255/diff/4-5/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 18, 2017, 3:17 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/4/
Changes: https://reviews.apache.org/r/58255/diff/3-4/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 14, 2017, 9:14 p.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/3/
Changes: https://reviews.apache.org/r/58255/diff/2-3/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 12, 2017, 3:08 p.m., Alexander Rojas wrote:
> > My concern about this patch is, that unlike all the other security components of Mesos, it makes implicit authorization mandatory. Moreover, we are breaking here the process of authz between the module and mesos itself.
> >
> > I think it would be better `if`'s statements to check first that `authorizer.isSome()` as we used somewhere else. Although I would prefer to delegate the whole thing to the authorizer itself, along the lines of:
> >
> > ```c++
> > if (authorizer) {
> > authorizer->verifyClaims(principal, ???);
> > }
> > ```
Yea this is a good point. One other place in the Mesos code where we do this is in the scheduler API: https://github.com/apache/mesos/blob/3ded707cab2c1037fd1a699b075895feceb3ae4a/src/master/http.cpp#L885-L890
This enforces for the V1 API a constraint which is implicit in the V0 scheduler API: a framework can only perform actions on itself. In V0, since authentication is performed only once when the persistent TCP connection is established and all subsequent calls are issued over that connection, this constraint is inherent to the implementation. In V1, we perform a check in the handler to enforce that constraint.
The case of the executor API is slightly different, since the V0 executor API has no authentication at all. Consequently, if we were to add the implicit authorization from this patch to the local authorizer, we would be introducing authorization actions which are only used for the V1 executor API and not for the V0 executor API, which would be an unfortunate inconsistency.
I think that either approach is not ideal. On one hand, as you said we would have implicit authorization which is made mandatory in the handler, and on the other hand, we would have a set of authorization actions which are only invoked for the V0 API, even though those actions do exist on the V0 API as well.
I would propose that we punt on this for now, and leave a TODO here and in the relevant section of 'master/http.cpp' to move both the scheduler and the executor implicit authorization into the local authorizer later on. I could create a ticket for that work, reference it in the TODO, and add it to the Executor AuthN Improvements epic when we create one. Does that seem reasonable, or do you think that the executor API authorization should go into the authorizer now?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review171739
-----------------------------------------------------------
On April 14, 2017, 9:14 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 14, 2017, 9:14 p.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/3/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review171739
-----------------------------------------------------------
My concern about this patch is, that unlike all the other security components of Mesos, it makes implicit authorization mandatory. Moreover, we are breaking here the process of authz between the module and mesos itself.
I think it would be better `if`'s statements to check first that `authorizer.isSome()` as we used somewhere else. Although I would prefer to delegate the whole thing to the authorizer itself, along the lines of:
```c++
if (authorizer) {
authorizer->verifyClaims(principal, ???);
}
```
- Alexander Rojas
On April 12, 2017, 9:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 12, 2017, 9:28 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/2/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review171920
-----------------------------------------------------------
Add TODO to possibly move into authorizer, create ticket for consistent authZ story across executors and frameworks.
src/slave/http.cpp
Lines 731 (patched)
<https://reviews.apache.org/r/58255/#comment244932>
Return Forbidden?
- Greg Mann
On April 12, 2017, 7:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 12, 2017, 7:28 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/2/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/
-----------------------------------------------------------
(Updated April 12, 2017, 7:28 a.m.)
Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
Bugs: MESOS-7339
https://issues.apache.org/jira/browse/MESOS-7339
Repository: mesos
Description
-------
This patch updates the agent handler for the executor API to
verify the FrameworkID and ExecutorID contained within the
executor's `Principal`, if present. This effectively performs
implicit authorization of executor calls.
Diffs (updated)
-----
src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
Diff: https://reviews.apache.org/r/58255/diff/2/
Changes: https://reviews.apache.org/r/58255/diff/1-2/
Testing
-------
Testing details can be found at the end of this chain.
Thanks,
Greg Mann
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Greg Mann <gr...@mesosphere.io>.
> On April 11, 2017, 6:33 p.m., Vinod Kone wrote:
> > src/slave/http.cpp
> > Lines 732 (patched)
> > <https://reviews.apache.org/r/58255/diff/1/?file=1686365#file1686365line732>
> >
> > a `principalContains` helper looks weird. can you just inline the helpers?
> >
> > you can probably just do
> >
> >
> > ```
> > if (principal.isSome() &&
> > principal->claims["fid"] != call.framework_id().value()) {
> > }
> >
> > ```
The principal is `const`, and while it's more verbose I prefer the precision of using `.contains()` to avoid potentially performing the check with an empty string, so I inlined the check that way. Let me know what you think.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review171600
-----------------------------------------------------------
On April 12, 2017, 7:28 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 12, 2017, 7:28 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1
>
>
> Diff: https://reviews.apache.org/r/58255/diff/2/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 58255: Added implicit authorization to the agent
executor API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58255/#review171600
-----------------------------------------------------------
src/slave/http.cpp
Lines 699-701 (original), 727-729 (patched)
<https://reviews.apache.org/r/58255/#comment244546>
move this after implicit authz to avoid leaking more information than necessary.
src/slave/http.cpp
Lines 732 (patched)
<https://reviews.apache.org/r/58255/#comment244545>
a `principalContains` helper looks weird. can you just inline the helpers?
you can probably just do
```
if (principal.isSome() &&
principal->claims["fid"] != call.framework_id().value()) {
}
```
src/slave/http.cpp
Lines 703-707 (original), 739-757 (patched)
<https://reviews.apache.org/r/58255/#comment244547>
ditto. see above.
- Vinod Kone
On April 7, 2017, 3:45 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
>
> (Updated April 7, 2017, 3:45 a.m.)
>
>
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
>
>
> Bugs: MESOS-7339
> https://issues.apache.org/jira/browse/MESOS-7339
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
>
>
> Diffs
> -----
>
> src/slave/http.cpp b07ce7c73a90ef297d980806ebba9530d86f25ae
>
>
> Diff: https://reviews.apache.org/r/58255/diff/1/
>
>
> Testing
> -------
>
> Testing details can be found at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>