Repository: mesos Updated Branches: refs/heads/master 0183a73f3 -> 105755a9e
Refactored endpoint extraction from URL path. The short path of HTTP endpoint is now extracted from `URL.path` in separate functions in both `Slave::Http` and `Master::Http`, making `extractEndpoint()` consistent in both classes. In the future, `extractEndpoint() should be replaced in favor of a more general solution, see MESOS-5369 for details. Review: https://reviews.apache.org/r/47339/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/105755a9 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/105755a9 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/105755a9 Branch: refs/heads/master Commit: 105755a9e20a917b2a3fe1492487f9a64b482eec Parents: 0183a73 Author: Jan Schlicht <j...@mesosphere.io> Authored: Fri May 13 18:40:55 2016 +0200 Committer: Alexander Rukletsov <al...@apache.org> Committed: Fri May 13 19:07:07 2016 +0200 ---------------------------------------------------------------------- src/master/http.cpp | 50 ++++++++++++++++------------- src/master/master.hpp | 3 ++ src/slave/http.cpp | 78 ++++++++++++++++++++++++++++++---------------- src/slave/slave.hpp | 24 ++++++++------ 4 files changed, 99 insertions(+), 56 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/105755a9/src/master/http.cpp ---------------------------------------------------------------------- diff --git a/src/master/http.cpp b/src/master/http.cpp index 6cefcc8..0245b86 100644 --- a/src/master/http.cpp +++ b/src/master/http.cpp @@ -870,31 +870,21 @@ Future<Response> Master::Http::flags( return MethodNotAllowed({"GET"}, request.method); } - // Paths are of the form "/master/endpoint". We're only interested - // in the part after "/master" and tokenize the path accordingly. - // - // TODO(nfnt): Refactor this into a helper function in `Master::Http` so - // that other endpoints can reuse it. In the long run, absolute paths for - // endpoins should be supported, see MESOS-5369. - const vector<string> pathComponents = - strings::tokenize(request.url.path, "/", 2); - - if (pathComponents.size() != 2u || - pathComponents[0] != master->self().id) { - return Failure("Unexpected path '" + request.url.path + "'"); + Try<string> endpoint = extractEndpoint(request.url); + if (endpoint.isError()) { + return Failure("Failed to extract endpoint: " + endpoint.error()); } - const string endpoint = "/" + pathComponents[1]; - return authorizeEndpoint(principal, endpoint, request.method) + return authorizeEndpoint(principal, endpoint.get(), request.method) .then(defer( - master->self(), - [this, request](bool authorized) -> Future<Response> { - if (!authorized) { - return Forbidden(); - } + master->self(), + [this, request](bool authorized) -> Future<Response> { + if (!authorized) { + return Forbidden(); + } - return _flags(request); - })); + return _flags(request); + })); } @@ -2853,6 +2843,24 @@ Future<Response> Master::Http::_operation( } +Try<string> Master::Http::extractEndpoint(const process::http::URL& url) const +{ + // Paths are of the form "/master/endpoint". We're only interested + // in the part after "/master" and tokenize the path accordingly. + // + // TODO(nfnt): In the long run, absolute paths for + // endpoins should be supported, see MESOS-5369. + const vector<string> pathComponents = strings::tokenize(url.path, "/", 2); + + if (pathComponents.size() < 2u || + pathComponents[0] != master->self().id) { + return Error("Unexpected path '" + url.path + "'"); + } + + return "/" + pathComponents[1]; +} + + Future<bool> Master::Http::authorizeEndpoint( const Option<string>& principal, const string& endpoint, http://git-wip-us.apache.org/repos/asf/mesos/blob/105755a9/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index bd0fa98..380be95 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -1232,6 +1232,9 @@ private: Resources required, const Offer::Operation& operation) const; + // Helper routines for endpoint authorization. + Try<std::string> extractEndpoint(const process::http::URL& url) const; + // Authorizes access to an HTTP endpoint. The `method` parameter // determines which ACL action will be used in the authorization. // It is expected that the caller has validated that `method` is http://git-wip-us.apache.org/repos/asf/mesos/blob/105755a9/src/slave/http.cpp ---------------------------------------------------------------------- diff --git a/src/slave/http.cpp b/src/slave/http.cpp index 0fb9b81..c5d6a7a 100644 --- a/src/slave/http.cpp +++ b/src/slave/http.cpp @@ -360,9 +360,20 @@ Future<Response> Slave::Http::flags( const Request& request, const Option<string>& principal) const { + // TODO(nfnt): Remove check for enabled + // authorization as part of MESOS-5346. + if (request.method != "GET" && slave->authorizer.isSome()) { + return MethodNotAllowed({"GET"}, request.method); + } + const Flags slaveFlags = slave->flags; - return authorizeEndpoint(request, principal) + Try<string> endpoint = extractEndpoint(request.url); + if (endpoint.isError()) { + return Failure("Failed to extract endpoint: " + endpoint.error()); + } + + return authorizeEndpoint(principal, endpoint.get(), request.method) .then(defer( slave->self(), [request, slaveFlags](bool authorized) -> Future<Response> { @@ -631,10 +642,21 @@ Future<Response> Slave::Http::statistics( const Request& request, const Option<string>& principal) const { + // TODO(nfnt): Remove check for enabled + // authorization as part of MESOS-5346. + if (request.method != "GET" && slave->authorizer.isSome()) { + return MethodNotAllowed({"GET"}, request.method); + } + const PID<Slave> pid = slave->self(); Shared<RateLimiter> limiter = statisticsLimiter; - return authorizeEndpoint(request, principal) + Try<string> endpoint = extractEndpoint(request.url); + if (endpoint.isError()) { + return Failure("Failed to extract endpoint: " + endpoint.error()); + } + + return authorizeEndpoint(principal, endpoint.get(), request.method) .then(defer( pid, [pid, limiter, request](bool authorized) -> Future<Response> { @@ -812,51 +834,55 @@ Future<Response> Slave::Http::containers(const Request& request) const } -Future<bool> Slave::Http::authorizeEndpoint( - const Request& httpRequest, - const Option<string>& principal) const +Try<string> Slave::Http::extractEndpoint(const process::http::URL& url) const { - if (slave->authorizer.isNone()) { - return true; - } - // Paths are of the form "/slave(n)/endpoint". We're only interested // in the part after "/slave(n)" and tokenize the path accordingly. // - // TODO(alexr): Refactor this into a helper function in `Slave::Http` so - // that other endpoints can reuse it. In the long run, absolute paths for + // TODO(alexr): In the long run, absolute paths for // endpoins should be supported, see MESOS-5369. - const vector<string> pathComponents = - strings::tokenize(httpRequest.url.path, "/", 2); + const vector<string> pathComponents = strings::tokenize(url.path, "/", 2); - if (pathComponents.size() != 2u || + if (pathComponents.size() < 2u || pathComponents[0] != slave->self().id) { - return Failure("Unexpected path '" + httpRequest.url.path + "'"); + return Error("Unexpected path '" + url.path + "'"); + } + + return "/" + pathComponents[1]; +} + + +Future<bool> Slave::Http::authorizeEndpoint( + const Option<string>& principal, + const string& endpoint, + const string& method) const +{ + if (slave->authorizer.isNone()) { + return true; } - const string endpoint = "/" + pathComponents[1]; - authorization::Request authorizationRequest; + authorization::Request request; - // TODO(nfnt): Add an additional case when POST requests need to be - // authorized separately from GET requests. - if (httpRequest.method == "GET") { - authorizationRequest.set_action(authorization::GET_ENDPOINT_WITH_PATH); + // TODO(nfnt): Add an additional case when POST requests + // need to be authorized separately from GET requests. + if (method == "GET") { + request.set_action(authorization::GET_ENDPOINT_WITH_PATH); } else { - return Failure("Unexpected request method '" + httpRequest.method + "'"); + return Failure("Unexpected request method '" + method + "'"); } if (principal.isSome()) { - authorizationRequest.mutable_subject()->set_value(principal.get()); + request.mutable_subject()->set_value(principal.get()); } - authorizationRequest.mutable_object()->set_value(endpoint); + request.mutable_object()->set_value(endpoint); LOG(INFO) << "Authorizing principal '" << (principal.isSome() ? principal.get() : "ANY") - << "' to " << httpRequest.method + << "' to " << method << " the '" << endpoint << "' endpoint"; - return slave->authorizer.get()->authorized(authorizationRequest); + return slave->authorizer.get()->authorized(request); } } // namespace slave { http://git-wip-us.apache.org/repos/asf/mesos/blob/105755a9/src/slave/slave.hpp ---------------------------------------------------------------------- diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp index be622d3..89e3348 100644 --- a/src/slave/slave.hpp +++ b/src/slave/slave.hpp @@ -472,21 +472,27 @@ private: const process::http::Request& request, const Flags& flags); - // Authorizes access to an HTTP endpoint. It extracts the endpoint - // from the URL of the request by removing the "/slave(n)" part of - // the URL's path. The request's `method` determines which ACL action - // will be used in the authorization. - process::Future<bool> authorizeEndpoint( - const process::http::Request& request, - const Option<std::string>& principal) const; - - // Make continuation for `statistics` `static` as it might // execute when the invoking `Http` is already destructed. static process::http::Response _statistics( const ResourceUsage& usage, const process::http::Request& request); + // Helper routines for endpoint authorization. + Try<std::string> extractEndpoint(const process::http::URL& url) const; + + // Authorizes access to an HTTP endpoint. The `method` parameter + // determines which ACL action will be used in the authorization. + // It is expected that the caller has validated that `method` is + // supported by this function. Currently "GET" is supported. + // + // TODO(nfnt): Prefer types instead of strings + // for `endpoint` and `method`, see MESOS-5300. + process::Future<bool> authorizeEndpoint( + const Option<std::string>& principal, + const std::string& endpoint, + const std::string& method) const; + Slave* slave; // Used to rate limit the statistics endpoint.
