> On June 23, 2016, 2:14 p.m., Adam B wrote: > > src/slave/http.cpp, line 861 > > <https://reviews.apache.org/r/49082/diff/3/?file=1428527#file1428527line861> > > > > How many things from the current context do we really need to pull into > > this lambda? Is it safe to pull them all in?
Just `this`, because is needed by `state` a couple of lines below (which answers the next question) and the `request` parameter. Note that both lambdas are execute safely inside the agent process (this one thanks to the `defer` of the previous line, the next one because it is executed synchronously. I'll make it explicit. > On June 23, 2016, 2:14 p.m., Adam B wrote: > > src/slave/http.cpp, line 864 > > <https://reviews.apache.org/r/49082/diff/3/?file=1428527#file1428527line864> > > > > `this` here refers to the slave? I guess we're deferred back onto the > > slave, so accessing its variables is safe? (And these should all be > > read-only operations) It is safe to executed thanks to the `defer` in line 858 which executes the enclosing lambda inside the agent's process and the `state` lambda is executed sychronously by `jsonify(state)` at the end of the lambda. > On June 23, 2016, 2:14 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, line 250 > > <https://reviews.apache.org/r/49082/diff/3/?file=1428530#file1428530line250> > > > > s/this->// `this->` is required: ``` ../../src/tests/slave_authorization_tests.cpp:230:40: error: use of undeclared identifier 'StartMaster' Try<Owned<cluster::Master>> master = StartMaster(authorizer.get()); ^ ../../src/tests/slave_authorization_tests.cpp:247:7: error: use of undeclared identifier 'StartSlave' StartSlave(detector.get(), &containerizer, authorizer.get()); ^ ../../src/tests/slave_authorization_tests.cpp:230:31: error: no matching constructor for initialization of 'Try<Owned<cluster::Master> >' Try<Owned<cluster::Master>> master = StartMaster(authorizer.get()); ^ ``` - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49082/#review139234 ----------------------------------------------------------- On June 23, 2016, 2:30 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49082/ > ----------------------------------------------------------- > > (Updated June 23, 2016, 2:30 p.m.) > > > Review request for mesos, Adam B, Joerg Schad, and Till Toenshoff. > > > Bugs: mesos-5150 > https://issues.apache.org/jira/browse/mesos-5150 > > > Repository: mesos > > > Description > ------- > > While information about frameworks, executors and tasks were well > protected in the master, this information was not protected in the > agents, which enabled unauthorized users to verify the data by getting > the agent `/state` endpoint. This was particularly pressing since the > Mesos UI would work as a proxy for agents endpoints so even being > behind a firewall would not had been enough. > > > Diffs > ----- > > src/common/http.hpp 588a1eb18e2665e71cc4361835d873652c4e100d > src/common/http.cpp 4b35736ef53baeb8eeefe5aadee3964905469e6d > src/master/http.cpp 70f084b84db90fde20c05d2354be190f28e72996 > src/slave/http.cpp 22b5930a7af336a749af4b6b50254f60433ecea5 > src/tests/mesos.hpp b9de9b6dfe0e62df91d5b82006b799345b49fd3c > src/tests/mesos.cpp 15aaaa7979f227083580afc1349373f3e3364c65 > src/tests/slave_authorization_tests.cpp > 98fde4914a5fddda4401d366e75322458bc9104c > > Diff: https://reviews.apache.org/r/49082/diff/ > > > Testing > ------- > > `make check`, some manual testing in the Mesos UI and the following script: > > ```sh > #! /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_mesos_logs" : [ > { > "principals" : { "values" : ["foo"] }, > "logs" : { "type" : "ANY" } > } > ], > "access_sandboxes" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_tasks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_executors" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "view_frameworks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "register_frameworks" : [ > { > "principals" : { "values" : ["foo"] }, > "roles" : { "values" : ["test"] } > } > ], > "run_tasks" : [ > { > "principals" : { "values" : ["foo"] }, > "users" : { "values" : ["$USER"] } > } > ], > "get_endpoints" : [ > { > "principals" : { "values" : ["foo"] }, > "paths" : { "type" : "ANY" } > } > ] > } > EOF > > ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \ > --authenticate_http \ > --log_dir=/tmp/mesos/logs/master \ > --http_credentials=file:///tmp/credentials.txt \ > --acls=file:///tmp/acls.json & > ./bin/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 & > ./src/mesos-execute \ > --command='while true; do echo "Hello world"; sleep 3; done' \ > --role=test \ > --master=127.0.0.1:5050 \ > --name=echoer \ > --principal=foo & > > # This should show complete information about frameworks, > # executors and tasks. > http GET http://127.0.0.1:5051/state -a foo:bar > > # This should show partial elements of the state but none > # related to the running framework. > http GET http://127.0.0.1:5051/state -a baz:bar > > # This should yield a 401 Unauthorized response. > http GET http://127.0.0.1:5051/state -a bar:bar > ``` > > > Thanks, > > Alexander Rojas > >
