----------------------------------------------------------- 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 > >
