mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 47795: Enabled authorization for sandboxes.
Date Wed, 25 May 2016 04:34:30 GMT

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



Great job! Just a few suggestions:
1. role -> user (to match /state filtering)
2. Add `optional FrameworkInfo` and `optionalExecutorInfo` to `authorization::Object` instead
of serializing to string
3. Remove `_WITH_INFO` and add a clear comment about what to expect in the Object.
4. Pass the frameworkId to authorizeSandboxAccess

Should I expect a separate ACL/patch for VIEW_LOGS?


include/mesos/authorizer/acls.proto (line 183)
<https://reviews.apache.org/r/47795/#comment199578>

    Just opening a discussion: Is "Access" the proper verb, vs. "Get" or "View"?
    Perhaps, since one could also "Browse" the sandbox, which is not exactly a get/view.
    I don't think it's necessary to have separate actions for Browse/Read/Download, but it
would make the verbs obvious.
    I'm ok with "Access", but would like to hear others' thoughts.



include/mesos/authorizer/acls.proto (line 184)
<https://reviews.apache.org/r/47795/#comment199568>

    Subjects: HTTP username.



include/mesos/authorizer/acls.proto (line 186)
<https://reviews.apache.org/r/47795/#comment199576>

    Please change this to "users" to match the /state filtering (WebUI should be consistent)
    https://reviews.apache.org/r/46613/diff/9#0



include/mesos/authorizer/acls.proto (lines 221 - 222)
<https://reviews.apache.org/r/47795/#comment199577>

    Not yours, but.. Please put these in numerical order. Deprecated fields already have the
deprecated tag and a comment to distinguish them.
    Misordered fields are more likely to confuse somebody into reusing an existing value.



include/mesos/authorizer/authorizer.proto (lines 58 - 59)
<https://reviews.apache.org/r/47795/#comment199570>

    Not yours, but.. Please move these down to their spot in numerical order.



include/mesos/authorizer/authorizer.proto (line 69)
<https://reviews.apache.org/r/47795/#comment199572>

    s/ACCESS_SANDBOX_WITH_INFO/ACCESS_SANDBOX/ since the INFO doesn't actually inform what
kind of info you're passing in the Object.
    Instead, we'll need a clear comment here (and in docs/ explaining what an authorizer module
writer should expect in the Object (FwkInfo+ExecInfo).



src/slave/slave.cpp (lines 5385 - 5386)
<https://reviews.apache.org/r/47795/#comment199575>

    Wouldn't this be a bit more efficient if you knew the desired frameworkId? I think `authorizeSandboxAccess()`
should also take frameworkId as a parameter.



src/slave/slave.cpp (line 5388)
<https://reviews.apache.org/r/47795/#comment199573>

    No need to use Joerg's filter/allower here. There's only 1 Framework+ExecutorInfo to copy
here, and it's probably much smaller than the file the user wants to download, so we can spare
the copy for clarity.
    You can remove this TODO.



src/slave/slave.cpp (lines 5396 - 5400)
<https://reviews.apache.org/r/47795/#comment199574>

    Rather than stringifying, let's just add `optional FrameworkInfo` and `optional ExecutorInfo`
to `authorization::Object` like in:
    https://reviews.apache.org/r/46613/diff/9#1


- Adam B


On May 24, 2016, 2:41 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47795/
> -----------------------------------------------------------
> 
> (Updated May 24, 2016, 2:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, and Vinod
Kone.
> 
> 
> Bugs: MESOS-5153
>     https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enables authorization of the sandboxes using the callback function
> parameter of `Files::attach()`.
> 
> It also adds relevant ACLs and support on the authorizer interface.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 911a2271211249a41c4467f6754e9996f640bf38

>   src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47795/diff/
> 
> 
> Testing
> -------
> 
> on OSX the script:
> 
> ```bash
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat <<EOF > /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat <<EOF > /tmp/acls.json
> {
>   "permissive": false,
>   "access_sandboxes" : [
>     {
>       "principals" : { "values" : ["foo"] },
>       "roles" : { "values" : ["test"] }
>     }
>   ]
> }
> EOF
> 
> ./mesos-master.sh --work_dir=/tmp/mesos/master &
> ./mesos-slave.sh --work_dir=/tmp/mesos/slave \
>                  --master=127.0.0.1:5050 \
>                  --authenticate_http \
>                  --http_credentials=file:///tmp/credentials.txt \
>                  --acls=file:///tmp/acls.json &
> 
> ./mesos-execute --command='while true; do echo "Hello world"; sleep 3; done' \
>                 --role=test \
>                 --master=127.0.0.1:5050 \
>                 --name=echoer &
> 
> SANDBOX_VPATH=`http GET http://127.0.0.1:5051/files/debug -a foo:bar -b  --pretty=none
\
>      | python -c 'import json,sys;obj=json.load(sys.stdin);print obj.keys()[0]'`
> 
> # This should yield a 200 OK response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a foo:bar
> 
> # HTTP/1.1 200 OK
> # Content-Disposition: attachment; filename=stdout
> # Content-Length: 3267
> # Content-Type: application/octet-stream
> # Date: Fri, 20 May 2016 13:52:31 GMT
> #
> # Received SUBSCRIBED event
> # Subscribed executor on localhost
> # Received LAUNCH event
> # Starting task echoer
> # sh -c 'while true; do echo "Hello world"; sleep 3; done'
> # Forked command at 26162
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> 
> # This shold yield a 403 Forbidden response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a baz:bar
> 
> # HTTP/1.1 403 Forbidden
> # Content-Length: 0
> # Date: Fri, 20 May 2016 13:52:37 GMT
> #
> #
> #
> 
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message