----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61171/#review181691 -----------------------------------------------------------
src/common/http.cpp Lines 1005-1009 (patched) <https://reviews.apache.org/r/61171/#comment257340> Ah, looks like we need to make sure we're doing authorization on the `Resource.reservations` field everywhere; see this case, for example: https://github.com/apache/mesos/blob/155285d3d5815aacda4d8fcc1c02c58ddb572a31/src/master/http.cpp#L514-L515 Could you make sure there aren't any other cases like that, and follow up with a patch to add this? src/master/http.cpp Lines 2524-2530 (patched) <https://reviews.apache.org/r/61171/#comment257335> Could you just do: ``` return AuthorizationAcceptor::create( principal, master->authorizer, authorization::VIEW_ROLE) .then(defer(master->self(), ``` src/tests/api_tests.cpp Lines 1638-1639 (patched) <https://reviews.apache.org/r/61171/#comment257319> s/are being filtered to unauthorized users/is returned to unauthorized users in response to the GET_AGENTS call/ src/tests/api_tests.cpp Lines 1642 (patched) <https://reviews.apache.org/r/61171/#comment257320> Can probably omit this comment. src/tests/api_tests.cpp Lines 1675-1682 (patched) <https://reviews.apache.org/r/61171/#comment257321> It would be nice to use the v1 RESERVE_RESOURCES call since we're also testing the v1 GET_AGENTS call, but I'm not going to open an issue since we mix v0/v1 in many other tests. Another option is to statically reserve the resources. Or leave as-is :) src/tests/api_tests.cpp Lines 1692-1693 (patched) <https://reviews.apache.org/r/61171/#comment257323> I think this comment is a bit misleading - the reason that this principal can view these reservations is because the authorizer's `permissive` bit is set to `true`, not because the reservations are "its own". Maybe: "Default credential principal should be allowed to see all reservations, regardless of role." src/tests/api_tests.cpp Lines 1704 (patched) <https://reviews.apache.org/r/61171/#comment257328> Might as well inspect the total/used/offered resources as well, right? Here and below. src/tests/api_tests.cpp Lines 1724-1725 (patched) <https://reviews.apache.org/r/61171/#comment257326> Maybe: "Principal 2 should not be able to see resources associated with any role." - Greg Mann On July 28, 2017, 11:57 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61171/ > ----------------------------------------------------------- > > (Updated July 28, 2017, 11:57 a.m.) > > > Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff. > > > Bugs: MESOS-7416 > https://issues.apache.org/jira/browse/MESOS-7416 > > > Repository: mesos > > > Description > ------- > > Enables filtering of the results of calls to the 'GET_AGENTS' v1 > API. It filters the contents of different resources entries based > on the 'VIEW_ROLE' permissions of the principal doing the request > based on resource roles, allocation roles and reservations. > > > Diffs > ----- > > src/common/http.hpp ba8dda18a02f51d1a28e719f06ee4b51573dfbec > src/common/http.cpp dfd5f335e8a3745d047d4f9f5e8c821b2c22da5a > src/common/protobuf_utils.hpp 80d2edd452f3ffa38c40f9a21f8489799065c401 > src/common/protobuf_utils.cpp 49d3a229925f4aa107e3e5f762936c16318aeadb > src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 > src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 > src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b > > > Diff: https://reviews.apache.org/r/61171/diff/3/ > > > Testing > ------- > > ```shell > make check > ``` > > Manual test: > > ```shell > mkdir -p /tmp/mesos/master > mkdir -p /tmp/mesos/agent > > # Create credentials > cat <<EOF > /tmp/mesos/credentials.txt > hal-9000 dave > glados potato > skynet connor > EOF > > # Create ACLs > cat <<EOF > /tmp/mesos/acls.json > { > "permissive": true, > "view_roles" : [ > { > "principals" : { "type" : "ANY" }, > "roles" : { "values" : ["*"] } > }, > { > "principals" : { "values" : ["hal-9000"] }, > "roles" : { "values" : ["space-odyssey"] } > }, > { > "principals" : { "values" : ["hal-9000"] }, > "roles" : { "type" : "NONE" } > }, > { > "principals" : { "values" : ["glados"] }, > "roles" : { "values" : ["portal"] } > }, > { > "principals" : { "values" : ["glados"] }, > "roles" : { "type" : "NONE" } > }, > { > "principals" : { "values" : ["skynet"] }, > "roles" : { "values" : ["terminator"] } > }, > { > "principals" : { "values" : ["skynet"] }, > "roles" : { "type" : "NONE" } > } > ] > } > EOF > > # Launch Master with some predefined roles. > ./bin/mesos-master.sh \ > --work_dir=/tmp/mesos/master \ > --log_dir=/tmp/mesos/master/log \ > --authenticate_http \ > --credentials=/tmp/mesos/credentials.txt \ > --authenticate_http_frameworks \ > --http_framework_authenticators=basic \ > --http_authenticators=basic \ > --authenticate_http_readonly \ > --acls=/tmp/mesos/acls.json \ > --roles="space-odyssey,portal,terminator" & > > # Launch Agent with static reservations for all roles. > sudo ./bin/mesos-agent.sh \ > --master=127.0.0.1:5050 \ > --work_dir=/tmp/mesos/agent \ > --authenticate_http_readwrite \ > --http_authenticators=basic \ > --http_credentials=/tmp/mesos/credentials.txt \ > --acls=/tmp/mesos/acls.json \ > > --resources='cpus(space-odyssey):2;cpus(portal):2;cpus(*):4;mem(space-odyssey):250;mem(portal):250;mem(*):10360;ports(space-odyssey):[31000-32000];ports(portal):[32001-33000];ports(*):[33001-35000];disk(space-odyssey):250;disk(portal):250;disk(*):1000' > & > > # Launch test framework. > ./src/mesos-execute \ > --master=127.0.0.1:5050 \ > --command='while true; do echo "Hello World"; sleep 5; done;' \ > --resources="cpus:1;mem:128;disk:32;ports:[31002-31003]" \ > --role=space-odyssey \ > --name=hello-discovery \ > --principal=hal-9000 \ > --secret=dave & > > # Create a dynamic reservation. > cat > /tmp/resources.json <<EOM > [ > { > "name": "cpus", > "type": "SCALAR", > "scalar": { "value": 2 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "mem", > "type": "SCALAR", > "scalar": { "value": 250 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "disk", > "type": "SCALAR", > "scalar": { "value": 250 }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > }, > { > "name": "ports", > "type": "RANGES", > "ranges": { > "range": [ > { > "begin": 33001, > "end": 34000 > } > ] > }, > "role": "terminator", > "reservation": { > "principal": "skynet" > } > } > ] > EOM > > http \ > -a skynet:connor \ > -f POST \ > 127.0.0.1:5050/master/reserve \ > slaveId=${SLAVE_ID} \ > resources=@/tmp/resources.json > > > # Create some quota. > cat > /tmp/quota.json <<EOM > { > "role": "portal", > "guarantee": [ > { > "name": "cpus", > "type": "SCALAR", > "scalar": { "value": 2 } > }, > { > "name": "mem", > "type": "SCALAR", > "scalar": { "value": 250 } > }, > { > "name": "disk", > "type": "SCALAR", > "scalar": { "value": 250 } > } > ] > } > EOM > > http \ > -a glados:potato \ > POST \ > 127.0.0.1:5050/master/quota \ > @/tmp/quota.json > > > # Query the master with all users and check > # that only the information of his role is > # available. > http -a glados:potato -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > > http -a skynet:connor -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > > http -a hal-9000:dave -v -j -f POST 127.0.0.1:5050/api/v1 type=GET_AGENTS > ``` > > > Thanks, > > Alexander Rojas > >
