Repository: mesos Updated Branches: refs/heads/master d86341b21 -> b6187a3b2
Fixed some nits in libprocess firewall. Review: https://reviews.apache.org/r/35353 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/70dfe873 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/70dfe873 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/70dfe873 Branch: refs/heads/master Commit: 70dfe873777520df17ae196e5dd2874c47988308 Parents: d86341b Author: Alexander Rojas <[email protected]> Authored: Wed Jun 24 17:35:49 2015 +0200 Committer: Till Toenshoff <[email protected]> Committed: Wed Jun 24 17:35:50 2015 +0200 ---------------------------------------------------------------------- .../libprocess/include/process/firewall.hpp | 37 +++++++++++--------- 3rdparty/libprocess/include/process/process.hpp | 20 ++++++----- 3rdparty/libprocess/src/process.cpp | 27 ++++++++------ 3rdparty/libprocess/src/tests/process_tests.cpp | 36 ++++++++++--------- 4 files changed, 69 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/70dfe873/3rdparty/libprocess/include/process/firewall.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/firewall.hpp b/3rdparty/libprocess/include/process/firewall.hpp index 16ed852..f71d654 100644 --- a/3rdparty/libprocess/include/process/firewall.hpp +++ b/3rdparty/libprocess/include/process/firewall.hpp @@ -20,20 +20,23 @@ #include <process/http.hpp> #include <process/socket.hpp> +#include <stout/error.hpp> #include <stout/hashset.hpp> +#include <stout/option.hpp> namespace process { namespace firewall { /** - * A 'FirewallRule' is an interface which allows control over which - * incoming HTTP connections are allowed. Concrete classes based on - * this interface must implement the 'apply' method; this method - * receives as parameters the socket where the connection is being - * initiated, as well as the request itself. + * A 'FirewallRule' describes an interface which provides control + * over incoming HTTP requests while also taking the underlying + * connection into account. + * + * Concrete classes based on this interface must implement the + * 'apply' method. * * Rules can be installed using the free function - * 'process::firewall::install()' defined in 'process.hpp' + * 'process::firewall::install()' defined in 'process.hpp'. */ class FirewallRule { @@ -42,24 +45,24 @@ public: virtual ~FirewallRule() {} /** - * Method used to do inspection of incoming HTTP requests. It allows - * implementations to verify conditions on the requests and notify - * the caller if the rule has been broken. + * Verify rule by applying it to an HTTP request and its underlying + * socket connection. * - * @param socket Socket used to attempt the HTTP connection. + * @param socket Socket used to deliver the HTTP request. * @param request HTTP request made by the client to libprocess. - * @return If the condition verification fails, i.e. the condition - * has been broken, the returned Try contains an error. + * @return If the rule verification fails, i.e. the rule didn't + * match, the returned error is set with an explanation for the + * failure. Otherwise None is returned. */ - virtual Try<Nothing> apply( + virtual Option<Error> apply( const network::Socket& socket, const http::Request& request) = 0; }; /** - * Simple firewall rule to reject any connection requesting a path - * in the provided list of disabled endpoints. + * Simple firewall rule to forbid any HTTP request to a path + * in the provided list of endpoints. * * Matches are required to be exact, no substrings nor wildcards are * considered for a match. @@ -72,7 +75,7 @@ public: virtual ~DisabledEndpointsFirewallRule() {} - virtual Try<Nothing> apply( + virtual Option<Error> apply( const network::Socket&, const http::Request& request) { @@ -80,7 +83,7 @@ public: return Error("'" + request.path + "' is disabled"); } - return Nothing(); + return None(); } private: http://git-wip-us.apache.org/repos/asf/mesos/blob/70dfe873/3rdparty/libprocess/include/process/process.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/process.hpp b/3rdparty/libprocess/include/process/process.hpp index 6a0b21d..59b50af 100644 --- a/3rdparty/libprocess/include/process/process.hpp +++ b/3rdparty/libprocess/include/process/process.hpp @@ -5,6 +5,7 @@ #include <map> #include <queue> +#include <vector> #include <process/address.hpp> #include <process/clock.hpp> @@ -14,6 +15,7 @@ #include <process/http.hpp> #include <process/message.hpp> #include <process/mime.hpp> +#include <process/owned.hpp> #include <process/pid.hpp> #include <stout/duration.hpp> @@ -25,16 +27,18 @@ namespace process { namespace firewall { + /** - * Installs the list of firewall rules to be used to allow or reject - * incoming connections. Each incoming connection will be tested - * against each rule in the order in which they appear in the vector - * 'rules'. As soon as any of the tests (calling the apply method) - * fails, the connection is rejected. If no test fails, the connection - * is allowed. + * Install a list of firewall rules which are used to forbid incoming + * HTTP requests. The rules will be applied in the provided order to + * each incoming request. If any rule forbids the request, no more + * rules are applied and a "403 Forbidden" response will be returned + * containing the reason from the rule. Note that if a request is + * forbidden, the request's handler is not notified. + * @see process#firewall#FirewallRule * - * @param rules List of rules which will be applied to incoming - * connections. + * @param rules List of rules which will be applied to all incoming + * HTTP requests. */ void install(std::vector<Owned<FirewallRule>>&& rules); http://git-wip-us.apache.org/repos/asf/mesos/blob/70dfe873/3rdparty/libprocess/src/process.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/process.cpp b/3rdparty/libprocess/src/process.cpp index a67a3af..4f0b281 100644 --- a/3rdparty/libprocess/src/process.cpp +++ b/3rdparty/libprocess/src/process.cpp @@ -58,6 +58,7 @@ #include <process/io.hpp> #include <process/logging.hpp> #include <process/mime.hpp> +#include <process/owned.hpp> #include <process/process.hpp> #include <process/profiler.hpp> #include <process/socket.hpp> @@ -360,7 +361,7 @@ public: void terminate(const UPID& pid, bool inject, ProcessBase* sender = NULL); bool wait(const UPID& pid); - void installFirewall(std::vector<Owned<FirewallRule>>&& rules); + void installFirewall(vector<Owned<FirewallRule>>&& rules); void enqueue(ProcessBase* process); ProcessBase* dequeue(); @@ -388,8 +389,8 @@ private: // Number of running processes, to support Clock::settle operation. int running; - // List of rules applied to all incoming HTTP connections. - std::vector<Owned<FirewallRule>> firewallRules; + // List of rules applied to all incoming HTTP requests. + vector<Owned<FirewallRule>> firewallRules; std::recursive_mutex firewall_mutex; }; @@ -696,7 +697,7 @@ void on_accept(const Future<Socket>& socket) namespace firewall { -void install(std::vector<Owned<FirewallRule>>&& rules) +void install(vector<Owned<FirewallRule>>&& rules) { process::initialize(); @@ -2008,12 +2009,15 @@ bool ProcessManager::handle( } synchronized (firewall_mutex) { + // Don't use a const reference, since it cannot be guaranteed + // that the rules don't keep an internal state. foreach (Owned<FirewallRule>& rule, firewallRules) { - Try<Nothing> applied = rule->apply(socket, *request); - if (applied.isError()) { + Option<Error> rejection = rule->apply(socket, *request); + if (rejection.isSome()) { VLOG(1) << "Returning '403 Forbidden' for '" << request->path - << "' (firewall rule forbids connection): " - << applied.error(); + << "' (firewall rule forbids request): " + << rejection.get().message; + // TODO(arojas): Get rid of the duplicated code to return an // error. @@ -2023,7 +2027,10 @@ bool ProcessManager::handle( // Enqueue the response with the HttpProxy so that it respects // the order of requests to account for HTTP/1.1 pipelining. dispatch( - proxy, &HttpProxy::enqueue, Forbidden(applied.error()), *request); + proxy, + &HttpProxy::enqueue, + Forbidden(rejection.get().message), + *request); // Cleanup request. delete request; @@ -2478,7 +2485,7 @@ bool ProcessManager::wait(const UPID& pid) } -void ProcessManager::installFirewall(std::vector<Owned<FirewallRule>>&& rules) +void ProcessManager::installFirewall(vector<Owned<FirewallRule>>&& rules) { synchronized (firewall_mutex) { firewallRules = std::move(rules); http://git-wip-us.apache.org/repos/asf/mesos/blob/70dfe873/3rdparty/libprocess/src/tests/process_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/process_tests.cpp b/3rdparty/libprocess/src/tests/process_tests.cpp index f4633aa..99065b3 100644 --- a/3rdparty/libprocess/src/tests/process_tests.cpp +++ b/3rdparty/libprocess/src/tests/process_tests.cpp @@ -8,6 +8,7 @@ #include <string> #include <sstream> #include <tuple> +#include <vector> #include <process/async.hpp> #include <process/collect.hpp> @@ -22,6 +23,7 @@ #include <process/gmock.hpp> #include <process/gtest.hpp> #include <process/network.hpp> +#include <process/owned.hpp> #include <process/process.hpp> #include <process/run.hpp> #include <process/socket.hpp> @@ -48,7 +50,9 @@ using process::firewall::FirewallRule; using process::network::Address; using process::network::Socket; +using std::move; using std::string; +using std::vector; using testing::_; using testing::Assign; @@ -1917,19 +1921,19 @@ public: // attempts to connect to those endpoints. TEST(Process, FirewallDisablePaths) { - const string processId = "testprocess"; + const string id = "testprocess"; + // TODO(arojas): Add initilization list construction when available. hashset<string> endpoints; - endpoints.insert(path::join("", processId, "handler1")); - endpoints.insert(path::join("", processId, "handler2/nested")); + endpoints.insert(path::join("", id, "handler1")); + endpoints.insert(path::join("", id, "handler2/nested")); // Patterns are not supported, so this should do nothing. - endpoints.insert(path::join("", processId, "handler3/*")); + endpoints.insert(path::join("", id, "handler3/*")); - std::vector<Owned<FirewallRule>> rules; - rules.emplace_back(new DisabledEndpointsFirewallRule(endpoints)); - process::firewall::install(std::move(rules)); + process::firewall::install( + {Owned<FirewallRule>(new DisabledEndpointsFirewallRule(endpoints))}); - HTTPEndpointProcess process(processId); + HTTPEndpointProcess process(id); PID<HTTPEndpointProcess> pid = spawn(process); @@ -1990,17 +1994,17 @@ TEST(Process, FirewallDisablePaths) // An empty vector should allow all paths. TEST(Process, FirewallUninstall) { - const string processId = "testprocess"; + const string id = "testprocess"; + // TODO(arojas): Add initilization list construction when available. hashset<string> endpoints; - endpoints.insert(path::join("", processId, "handler1")); - endpoints.insert(path::join("", processId, "handler2")); + endpoints.insert(path::join("", id, "handler1")); + endpoints.insert(path::join("", id, "handler2")); - std::vector<Owned<FirewallRule>> rules; - rules.emplace_back(new DisabledEndpointsFirewallRule(endpoints)); - process::firewall::install(std::move(rules)); + process::firewall::install( + {Owned<FirewallRule>(new DisabledEndpointsFirewallRule(endpoints))}); - HTTPEndpointProcess process(processId); + HTTPEndpointProcess process(id); PID<HTTPEndpointProcess> pid = spawn(process); @@ -2014,7 +2018,7 @@ TEST(Process, FirewallUninstall) AWAIT_READY(response); EXPECT_EQ(http::statuses[403], response.get().status); - process::firewall::install(std::vector<Owned<FirewallRule>>()); + process::firewall::install({}); EXPECT_CALL(process, handler1(_)) .WillOnce(Return(http::OK()));
