From 0dfa5a9cc1fdd5d558d179bdfa4b1e3aff3c4e6d Mon Sep 17 00:00:00 2001 From: Flynn Date: Mon, 7 Aug 2017 13:24:46 -0400 Subject: [PATCH 01/16] Build extauth filter as part of Envoy core Signed-off-by: Flynn --- source/common/http/filter/BUILD | 16 +++ source/common/http/filter/extauth.cc | 104 ++++++++++++++++++++ source/common/http/filter/extauth.h | 74 ++++++++++++++ source/exe/BUILD | 1 + source/server/config/http/BUILD | 11 +++ source/server/config/http/extauth_config.cc | 48 +++++++++ source/server/config/http/extauth_config.h | 26 +++++ 7 files changed, 280 insertions(+) create mode 100644 source/common/http/filter/extauth.cc create mode 100644 source/common/http/filter/extauth.h create mode 100644 source/server/config/http/extauth_config.cc create mode 100644 source/server/config/http/extauth_config.h diff --git a/source/common/http/filter/BUILD b/source/common/http/filter/BUILD index 6cd4fc2ae20b..74a2ca705a97 100644 --- a/source/common/http/filter/BUILD +++ b/source/common/http/filter/BUILD @@ -44,6 +44,22 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "extauth_lib", + srcs = ["extauth.cc"], + hdrs = ["extauth.h"], + deps = [ + "//include/envoy/buffer:buffer_interface", + "//include/envoy/http:filter_interface", + "//include/envoy/upstream:cluster_manager_interface", + "//source/common/common:assert_lib", + "//source/common/common:logger_lib", + "//source/common/common:enum_to_int", + "//source/common/http:message_lib", + "//source/common/http:utility_lib" + ], +) + envoy_cc_library( name = "fault_filter_lib", srcs = ["fault_filter.cc"], diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc new file mode 100644 index 000000000000..1e05fd6c58d8 --- /dev/null +++ b/source/common/http/filter/extauth.cc @@ -0,0 +1,104 @@ +#include "common/http/filter/extauth.h" + +#include "common/common/assert.h" +#include "common/common/enum_to_int.h" +#include "common/http/message_impl.h" +#include "common/http/utility.h" + +namespace Envoy { +namespace Http { + +static LowerCaseString header_to_add(std::string("x-ark3-stuff")); + +ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {} + +ExtAuth::~ExtAuth() { ASSERT(!auth_request_); } + +FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { + log().info("ExtAuth Request received; contacting auth server"); + + // Request external authentication + auth_complete_ = false; + MessagePtr request(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); + // request->headers().insertMethod().value(Http::Headers::get().MethodValues.Post); + // request->headers().insertPath().value(std::string("/ambassador/auth")); + request->headers().insertHost().value(config_->cluster_); // cluster name is Host: header value! + request->headers().insertContentLength().value(uint64_t(0)); + // request->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); + auth_request_ = + config_->cm_.httpAsyncClientForCluster(config_->cluster_) + .send(std::move(request), *this, Optional(config_->timeout_)); + // .send(...) -> onSuccess(...) or onFailure(...) + // This handle can be used to ->cancel() the request. + + // Stop until we have a result + return FilterHeadersStatus::StopIteration; +} + +FilterDataStatus ExtAuth::decodeData(Buffer::Instance&, bool) { + if (auth_complete_) { + return FilterDataStatus::Continue; + } + return FilterDataStatus::StopIterationAndBuffer; +} + +FilterTrailersStatus ExtAuth::decodeTrailers(HeaderMap&) { + if (auth_complete_) { + return FilterTrailersStatus::Continue; + } + return FilterTrailersStatus::StopIteration; +} + +ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Store& store) { + std::string final_prefix = prefix + "extauth."; + return {ALL_EXTAUTH_STATS(POOL_COUNTER_PREFIX(store, final_prefix))}; +} + +void ExtAuth::onSuccess(Http::MessagePtr&& response) { + auth_request_ = nullptr; + uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); + std::string response_body(response->bodyAsString()); + log().info("ExtAuth Auth responded with code {}", response_code); + if (!response_body.empty()) { + log().info("ExtAuth Auth said: {}", response->bodyAsString()); + } + + if (response_code != enumToInt(Http::Code::OK)) { + log().info("ExtAuth rejecting request"); + config_->stats_.rq_rejected_.inc(); + Http::HeaderMapPtr response_headers{new HeaderMapImpl(response->headers())}; + callbacks_->encodeHeaders(std::move(response_headers), response_body.empty()); + if (!response_body.empty()) { + Buffer::OwnedImpl buffer(response_body); + callbacks_->encodeData(buffer, true); + } + return; + } + + log().info("ExtAuth accepting request"); + config_->stats_.rq_passed_.inc(); + auth_complete_ = true; + callbacks_->continueDecoding(); +} + +void ExtAuth::onFailure(Http::AsyncClient::FailureReason) { + auth_request_ = nullptr; + log().warn("ExtAuth Auth request failed"); + config_->stats_.rq_failed_.inc(); + Http::Utility::sendLocalReply(*callbacks_, false, Http::Code::ServiceUnavailable, + std::string("Auth request failed.")); +} + +void ExtAuth::onDestroy() { + if (auth_request_) { + auth_request_->cancel(); + auth_request_ = nullptr; + } +} + +void ExtAuth::setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) { + callbacks_ = &callbacks; +} + +} // Http +} // Envoy diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h new file mode 100644 index 000000000000..397fbf11c914 --- /dev/null +++ b/source/common/http/filter/extauth.h @@ -0,0 +1,74 @@ +#pragma once + +#include "envoy/http/filter.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/common/logger.h" + +namespace Envoy { +namespace Http { + +/** + * All stats for the extauth filter. @see stats_macros.h + */ +// clang-format off +#define ALL_EXTAUTH_STATS(COUNTER) \ + COUNTER(rq_failed) \ + COUNTER(rq_passed) \ + COUNTER(rq_rejected) \ + COUNTER(rq_redirected) +// clang-format on + +/** + * Wrapper struct for extauth filter stats. @see stats_macros.h + */ +struct ExtAuthStats { + ALL_EXTAUTH_STATS(GENERATE_COUNTER_STRUCT) +}; + +/** + * Configuration for the extauth filter. + */ +struct ExtAuthConfig { + Upstream::ClusterManager& cm_; + ExtAuthStats stats_; + std::string cluster_; + std::chrono::milliseconds timeout_; +}; + +typedef std::shared_ptr ExtAuthConfigConstSharedPtr; + +/** + * A pass-through filter that talks to an external authn/authz service (or will soon...) + */ +class ExtAuth : Logger::Loggable, + public StreamDecoderFilter, + public Http::AsyncClient::Callbacks { +public: + ExtAuth(ExtAuthConfigConstSharedPtr config); + ~ExtAuth(); + + static ExtAuthStats generateStats(const std::string& prefix, Stats::Store& store); + + // Http::StreamFilterBase + void onDestroy() override; + + // Http::StreamDecoderFilter + FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) override; + FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; + FilterTrailersStatus decodeTrailers(HeaderMap& trailers) override; + void setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) override; + + // Http::AsyncClient::Callbacks + void onSuccess(Http::MessagePtr&& response) override; + void onFailure(Http::AsyncClient::FailureReason reason) override; + +private: + ExtAuthConfigConstSharedPtr config_; + StreamDecoderFilterCallbacks* callbacks_{}; + bool auth_complete_; + Http::AsyncClient::Request* auth_request_{}; +}; + +} // Http +} // Envoy diff --git a/source/exe/BUILD b/source/exe/BUILD index 62660f3f5cb4..bcf477045756 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -38,6 +38,7 @@ envoy_cc_library( "//source/server/config/http:buffer_lib", "//source/server/config/http:cors_lib", "//source/server/config/http:fault_lib", + "//source/server/config/http:extauth_lib", "//source/server/config/http:grpc_http1_bridge_lib", "//source/server/config/http:grpc_json_transcoder_lib", "//source/server/config/http:grpc_web_lib", diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 7f68c86f1f5e..25341b61c983 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -72,6 +72,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "extauth_lib", + srcs = ["extauth_config.cc"], + hdrs = ["extauth_config.h"], + deps = [ + "//source/common/http/filter:extauth_lib", + "//source/server:configuration_lib", + "//source/server/config/network:http_connection_manager_lib" + ], +) + envoy_cc_library( name = "fault_lib", srcs = ["fault.cc"], diff --git a/source/server/config/http/extauth_config.cc b/source/server/config/http/extauth_config.cc new file mode 100644 index 000000000000..3192fa1a01f5 --- /dev/null +++ b/source/server/config/http/extauth_config.cc @@ -0,0 +1,48 @@ +#include "server/config/http/extauth_config.h" +#include "common/http/filter/extauth.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +const std::string EXTAUTH_HTTP_FILTER_SCHEMA(R"EOF( + { + "$schema": "http://json-schema.org/schema#", + "type" : "object", + "properties" : { + "cluster" : {"type" : "string"}, + "timeout_ms": {"type" : "integer"} + }, + "required" : ["cluster", "timeout_ms"], + "additionalProperties" : false + } + )EOF"); + +HttpFilterFactoryCb ExtAuthConfig::tryCreateFilterFactory(HttpFilterType type, + const std::string& name, + const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) { + if (type != HttpFilterType::Decoder || name != "extauth") { + return nullptr; + } + + json_config.validateSchema(EXTAUTH_HTTP_FILTER_SCHEMA); + + Http::ExtAuthConfigConstSharedPtr config(new Http::ExtAuthConfig{ + server.clusterManager(), Http::ExtAuth::generateStats(stats_prefix, server.stats()), + json_config.getString("cluster"), + std::chrono::milliseconds(json_config.getInteger("timeout_ms"))}); + return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{new Http::ExtAuth(config)}); + }; +} + +/** + * Static registration for the extauth filter. @see RegisterHttpFilterConfigFactory. + */ +static RegisterHttpFilterConfigFactory register_; + +} // Configuration +} // Server +} // Envoy diff --git a/source/server/config/http/extauth_config.h b/source/server/config/http/extauth_config.h new file mode 100644 index 000000000000..48a1e6defb7f --- /dev/null +++ b/source/server/config/http/extauth_config.h @@ -0,0 +1,26 @@ +#pragma once + +#include + +#include "envoy/server/instance.h" + +#include "server/config/network/http_connection_manager.h" + +namespace Envoy { +namespace Server { +namespace Configuration { + +/** + * Config registration for the ExtAuth filter. @see HttpFilterConfigFactory. + */ +class ExtAuthConfig : public HttpFilterConfigFactory { +public: + HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, + const Json::Object& json_config, + const std::string& stats_prefix, + Server::Instance& server) override; +}; + +} // Configuration +} // Server +} // Envoy From 845ad8b62513d935f33109126a92871dee5e1175 Mon Sep 17 00:00:00 2001 From: Flynn Date: Mon, 7 Aug 2017 17:34:29 -0400 Subject: [PATCH 02/16] Support path_prefix for extauth. Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 15 ++++++++++++++- source/common/http/filter/extauth.h | 1 + source/server/config/http/extauth_config.cc | 13 ++++++++++--- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index 1e05fd6c58d8..cf8478a66e54 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -8,7 +8,8 @@ namespace Envoy { namespace Http { -static LowerCaseString header_to_add(std::string("x-ark3-stuff")); +static LowerCaseString header_to_add(std::string("x-ambassador-calltype")); +static LowerCaseString value_to_add(std::string("extauth-request")); ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {} @@ -20,10 +21,22 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { // Request external authentication auth_complete_ = false; MessagePtr request(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); + + if (!config_->path_prefix_.empty()) { + std::string path = request->headers().insertPath().value().c_str(); + + path = config_->path_prefix_ + path; + + request->headers().insertPath().value(path); + } + // request->headers().insertMethod().value(Http::Headers::get().MethodValues.Post); // request->headers().insertPath().value(std::string("/ambassador/auth")); request->headers().insertHost().value(config_->cluster_); // cluster name is Host: header value! request->headers().insertContentLength().value(uint64_t(0)); + + request->headers().addReference(header_to_add, value_to_add.get()); + // request->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index 397fbf11c914..fdb681e50e4c 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -34,6 +34,7 @@ struct ExtAuthConfig { ExtAuthStats stats_; std::string cluster_; std::chrono::milliseconds timeout_; + std::string path_prefix_; }; typedef std::shared_ptr ExtAuthConfigConstSharedPtr; diff --git a/source/server/config/http/extauth_config.cc b/source/server/config/http/extauth_config.cc index 3192fa1a01f5..51fcc71605a1 100644 --- a/source/server/config/http/extauth_config.cc +++ b/source/server/config/http/extauth_config.cc @@ -11,7 +11,8 @@ const std::string EXTAUTH_HTTP_FILTER_SCHEMA(R"EOF( "type" : "object", "properties" : { "cluster" : {"type" : "string"}, - "timeout_ms": {"type" : "integer"} + "timeout_ms": {"type" : "integer"}, + "path_prefix": {"type" : "string"} }, "required" : ["cluster", "timeout_ms"], "additionalProperties" : false @@ -29,10 +30,16 @@ HttpFilterFactoryCb ExtAuthConfig::tryCreateFilterFactory(HttpFilterType type, json_config.validateSchema(EXTAUTH_HTTP_FILTER_SCHEMA); + std::string prefix = + json_config.hasObject("path_prefix") ? json_config.getString("path_prefix") : ""; + Http::ExtAuthConfigConstSharedPtr config(new Http::ExtAuthConfig{ - server.clusterManager(), Http::ExtAuth::generateStats(stats_prefix, server.stats()), + server.clusterManager(), + Http::ExtAuth::generateStats(stats_prefix, server.stats()), json_config.getString("cluster"), - std::chrono::milliseconds(json_config.getInteger("timeout_ms"))}); + std::chrono::milliseconds(json_config.getInteger("timeout_ms")), + prefix + }); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{new Http::ExtAuth(config)}); }; From 25fe00a080c3f3787dc1d6e28eb6827baa7650f0 Mon Sep 17 00:00:00 2001 From: Flynn Date: Tue, 15 Aug 2017 14:37:25 -0400 Subject: [PATCH 03/16] Support adding headers from the auth server, with configuration specifying headers OK to be added. Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 44 +++++++++++++++++++++ source/common/http/filter/extauth.h | 2 + source/server/config/http/extauth_config.cc | 9 +++++ 3 files changed, 55 insertions(+) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index cf8478a66e54..f2d5c522f865 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -20,6 +20,8 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { // Request external authentication auth_complete_ = false; + request_headers_ = &headers; + MessagePtr request(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); if (!config_->path_prefix_.empty()) { @@ -69,28 +71,70 @@ ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Store& sto void ExtAuth::onSuccess(Http::MessagePtr&& response) { auth_request_ = nullptr; + uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); std::string response_body(response->bodyAsString()); + log().info("ExtAuth Auth responded with code {}", response_code); + if (!response_body.empty()) { log().info("ExtAuth Auth said: {}", response->bodyAsString()); } if (response_code != enumToInt(Http::Code::OK)) { log().info("ExtAuth rejecting request"); + config_->stats_.rq_rejected_.inc(); + request_headers_ = nullptr; + Http::HeaderMapPtr response_headers{new HeaderMapImpl(response->headers())}; + callbacks_->encodeHeaders(std::move(response_headers), response_body.empty()); + if (!response_body.empty()) { Buffer::OwnedImpl buffer(response_body); callbacks_->encodeData(buffer, true); } + return; } log().info("ExtAuth accepting request"); + + response->headers().iterate( + [](const HeaderEntry& header, void* vctx) -> void { + ExtAuth *self = static_cast(vctx); + // (void)vctx; + + std::string key(header.key().c_str()); + std::string value(header.value().c_str()); + + // log().info("ExtAuth response header {}: {}", key, value); + + // Should we use a map<> for this? We don't expect there to be many + // allowed headers. + + bool allowed = false; + + for (std::string allowed_header : self->config_->allowed_headers_) { + if (key == allowed_header) { + log().info("ExtAuth allowing response header {}: {}", key, value); + allowed = true; + + self->request_headers_->addCopy(LowerCaseString(key), value); + } + } + + if (!allowed) { + log().info("ExtAuth not allowing response header {}: {}", key, value); + } + }, + static_cast(this) + ); + config_->stats_.rq_passed_.inc(); auth_complete_ = true; + request_headers_ = nullptr; callbacks_->continueDecoding(); } diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index fdb681e50e4c..0244b90871a9 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -34,6 +34,7 @@ struct ExtAuthConfig { ExtAuthStats stats_; std::string cluster_; std::chrono::milliseconds timeout_; + std::vector allowed_headers_; std::string path_prefix_; }; @@ -69,6 +70,7 @@ class ExtAuth : Logger::Loggable, StreamDecoderFilterCallbacks* callbacks_{}; bool auth_complete_; Http::AsyncClient::Request* auth_request_{}; + Http::HeaderMap* request_headers_; }; } // Http diff --git a/source/server/config/http/extauth_config.cc b/source/server/config/http/extauth_config.cc index 51fcc71605a1..95adedd0ad40 100644 --- a/source/server/config/http/extauth_config.cc +++ b/source/server/config/http/extauth_config.cc @@ -12,6 +12,14 @@ const std::string EXTAUTH_HTTP_FILTER_SCHEMA(R"EOF( "properties" : { "cluster" : {"type" : "string"}, "timeout_ms": {"type" : "integer"}, + "allowed_headers": { + "type": "array", + "items": { + "type": "string" + }, + "minItems": 1, + "uniqueItems": true + }, "path_prefix": {"type" : "string"} }, "required" : ["cluster", "timeout_ms"], @@ -38,6 +46,7 @@ HttpFilterFactoryCb ExtAuthConfig::tryCreateFilterFactory(HttpFilterType type, Http::ExtAuth::generateStats(stats_prefix, server.stats()), json_config.getString("cluster"), std::chrono::milliseconds(json_config.getInteger("timeout_ms")), + json_config.getStringArray("allowed_headers", true), prefix }); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { From e57868059db3debe6e2c95759206ddf4ee76c57c Mon Sep 17 00:00:00 2001 From: Flynn Date: Wed, 23 Aug 2017 19:06:59 -0400 Subject: [PATCH 04/16] Correctly support invalidating the route cache when adding headers. Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 40 +++++++++++++++++++++------- source/common/http/filter/extauth.h | 3 +++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index f2d5c522f865..4187e1e0e5fb 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -15,12 +15,24 @@ ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {} ExtAuth::~ExtAuth() { ASSERT(!auth_request_); } +void ExtAuth::dumpHeaders(const char *what, HeaderMap* headers) { + log().info("ExtAuth headers ({}):", what); + + headers->iterate( + [](const HeaderEntry& header, void* context) -> void { + (void)context; + log().trace(" '{}':'{}'", + header.key().c_str(), header.value().c_str()); + }, + nullptr); +} + FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { - log().info("ExtAuth Request received; contacting auth server"); // Request external authentication auth_complete_ = false; request_headers_ = &headers; + dumpHeaders("decodeHeaders", request_headers_); MessagePtr request(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); @@ -39,6 +51,7 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { request->headers().addReference(header_to_add, value_to_add.get()); + log().info("ExtAuth contacting auth server"); // request->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) @@ -72,6 +85,8 @@ ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Store& sto void ExtAuth::onSuccess(Http::MessagePtr&& response) { auth_request_ = nullptr; + dumpHeaders("onSuccess", request_headers_); + uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); std::string response_body(response->bodyAsString()); @@ -101,10 +116,14 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { log().info("ExtAuth accepting request"); + // If we actually add any headers, we need to invalidate the route cache. + // If we don't, we don't want to invalidate the route cache because it's + // slow. + addedHeaders_ = false; + response->headers().iterate( [](const HeaderEntry& header, void* vctx) -> void { ExtAuth *self = static_cast(vctx); - // (void)vctx; std::string key(header.key().c_str()); std::string value(header.value().c_str()); @@ -114,24 +133,24 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { // Should we use a map<> for this? We don't expect there to be many // allowed headers. - bool allowed = false; - for (std::string allowed_header : self->config_->allowed_headers_) { if (key == allowed_header) { log().info("ExtAuth allowing response header {}: {}", key, value); - allowed = true; - + self->addedHeaders_ = true; self->request_headers_->addCopy(LowerCaseString(key), value); } } - - if (!allowed) { - log().info("ExtAuth not allowing response header {}: {}", key, value); - } }, static_cast(this) ); + if (addedHeaders_) { + dumpHeaders("ExtAuth invalidating route cache", request_headers_); + + // callbacks_->encodeHeaders(HeaderMapPtr(new HeaderMapImpl{request_headers_}), false); + callbacks_->clearRouteCache(); + } + config_->stats_.rq_passed_.inc(); auth_complete_ = true; request_headers_ = nullptr; @@ -140,6 +159,7 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { void ExtAuth::onFailure(Http::AsyncClient::FailureReason) { auth_request_ = nullptr; + request_headers_ = nullptr; log().warn("ExtAuth Auth request failed"); config_->stats_.rq_failed_.inc(); Http::Utility::sendLocalReply(*callbacks_, false, Http::Code::ServiceUnavailable, diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index 0244b90871a9..8dfdb237bf68 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -66,9 +66,12 @@ class ExtAuth : Logger::Loggable, void onFailure(Http::AsyncClient::FailureReason reason) override; private: + void dumpHeaders(const char *what, HeaderMap* headers); + ExtAuthConfigConstSharedPtr config_; StreamDecoderFilterCallbacks* callbacks_{}; bool auth_complete_; + bool addedHeaders_; Http::AsyncClient::Request* auth_request_{}; Http::HeaderMap* request_headers_; }; From 1f457723fb502956ffd433b52562a20d7d4bf705 Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 24 Aug 2017 01:54:49 -0400 Subject: [PATCH 05/16] Rename request to reqmsg just for my own sanity in keeping track of what's what. Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index 4187e1e0e5fb..b347e8826019 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -34,28 +34,28 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { request_headers_ = &headers; dumpHeaders("decodeHeaders", request_headers_); - MessagePtr request(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); + MessagePtr reqmsg(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); if (!config_->path_prefix_.empty()) { - std::string path = request->headers().insertPath().value().c_str(); + std::string path = reqmsg->headers().insertPath().value().c_str(); path = config_->path_prefix_ + path; - request->headers().insertPath().value(path); + reqmsg->headers().insertPath().value(path); } - // request->headers().insertMethod().value(Http::Headers::get().MethodValues.Post); - // request->headers().insertPath().value(std::string("/ambassador/auth")); - request->headers().insertHost().value(config_->cluster_); // cluster name is Host: header value! - request->headers().insertContentLength().value(uint64_t(0)); + // reqmsg->headers().insertMethod().value(Http::Headers::get().MethodValues.Post); + // reqmsg->headers().insertPath().value(std::string("/ambassador/auth")); + reqmsg->headers().insertHost().value(config_->cluster_); // cluster name is Host: header value! + reqmsg->headers().insertContentLength().value(uint64_t(0)); - request->headers().addReference(header_to_add, value_to_add.get()); + reqmsg->headers().addReference(header_to_add, value_to_add.get()); log().info("ExtAuth contacting auth server"); - // request->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); + // reqmsg->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) - .send(std::move(request), *this, Optional(config_->timeout_)); + .send(std::move(reqmsg), *this, Optional(config_->timeout_)); // .send(...) -> onSuccess(...) or onFailure(...) // This handle can be used to ->cancel() the request. From 8d95f25ac27dac1001dda4209ebcf6161e55992c Mon Sep 17 00:00:00 2001 From: Flynn Date: Mon, 28 Aug 2017 22:37:43 -0400 Subject: [PATCH 06/16] Invert the allowed-header check so that we're iterating the vector and not the map. Sigh. Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 49 +++++++++++++++------------- source/common/http/filter/extauth.h | 1 - 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index b347e8826019..bbc3effb3c30 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -115,39 +115,44 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { } log().info("ExtAuth accepting request"); + bool addedHeaders = false; - // If we actually add any headers, we need to invalidate the route cache. - // If we don't, we don't want to invalidate the route cache because it's - // slow. - addedHeaders_ = false; + // Do we have any headers configured to copy? - response->headers().iterate( - [](const HeaderEntry& header, void* vctx) -> void { - ExtAuth *self = static_cast(vctx); + if (!config_->allowed_headers_.empty()) { + // Yup. Let's see if any of them are present. - std::string key(header.key().c_str()); - std::string value(header.value().c_str()); + for (std::string allowed_header : config_->allowed_headers_) { + LowerCaseString key(allowed_header); - // log().info("ExtAuth response header {}: {}", key, value); + // OK, do we have this header? - // Should we use a map<> for this? We don't expect there to be many - // allowed headers. + const HeaderEntry* hdr = response->headers().get(key); - for (std::string allowed_header : self->config_->allowed_headers_) { - if (key == allowed_header) { - log().info("ExtAuth allowing response header {}: {}", key, value); - self->addedHeaders_ = true; - self->request_headers_->addCopy(LowerCaseString(key), value); + if (hdr) { + // Well, the header exists at all. Does it have a value? + + const HeaderString& value = hdr->value(); + + if (!value.empty()) { + // Not empty! Copy it into our request_headers_. + + std::string valstr(value.c_str()); + + ENVOY_STREAM_LOG(trace, "ExtAuth allowing response header {}: {}", *callbacks_, allowed_header, valstr); + addedHeaders = true; + request_headers_->addCopy(key, valstr); } } - }, - static_cast(this) - ); + } + } + + if (addedHeaders) { + // Yup, we added headers. Invalidate the route cache in case any of + // the headers will affect routing decisions. - if (addedHeaders_) { dumpHeaders("ExtAuth invalidating route cache", request_headers_); - // callbacks_->encodeHeaders(HeaderMapPtr(new HeaderMapImpl{request_headers_}), false); callbacks_->clearRouteCache(); } diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index 8dfdb237bf68..a4b005f70b36 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -71,7 +71,6 @@ class ExtAuth : Logger::Loggable, ExtAuthConfigConstSharedPtr config_; StreamDecoderFilterCallbacks* callbacks_{}; bool auth_complete_; - bool addedHeaders_; Http::AsyncClient::Request* auth_request_{}; Http::HeaderMap* request_headers_; }; From 891006a03d0e277976e1778e69cf583f4882e861 Mon Sep 17 00:00:00 2001 From: Flynn Date: Mon, 28 Aug 2017 22:40:12 -0400 Subject: [PATCH 07/16] Add comments, remove dead code, clean up logging. Signed-off-by: Flynn --- source/common/http/conn_manager_impl.cc | 12 ++ source/common/http/filter/BUILD | 4 +- source/common/http/filter/extauth.cc | 174 ++++++++++++++++---- source/common/http/filter/extauth.h | 29 +++- source/server/config/http/BUILD | 2 +- source/server/config/http/extauth_config.cc | 36 ++-- source/server/config/http/extauth_config.h | 22 +-- 7 files changed, 213 insertions(+), 66 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c1f95fe251b9..c88b9f110be6 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -642,6 +642,16 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); +#ifndef NVLOG + headers.iterate( + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + ENVOY_STREAM_LOG(trace, " H'{}':'{}'", *static_cast(context), + header.key().c_str(), header.value().c_str()); + return HeaderMap::Iterate::Continue; + }, + this); +#endif + if (!(*entry)->commonHandleAfterHeadersCallback(status) && std::next(entry) != decoder_filters_.end()) { // Stop iteration IFF this is not the last filter. If it is the last filter, continue with @@ -1254,6 +1264,8 @@ Router::RouteConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::route } void ConnectionManagerImpl::ActiveStreamFilterBase::clearRouteCache() { + ENVOY_STREAM_LOG(trace, "clearing route cache: filter={}", parent_, + static_cast(this)); parent_.cached_route_ = Optional(); } diff --git a/source/common/http/filter/BUILD b/source/common/http/filter/BUILD index 74a2ca705a97..7dc9ba29c7eb 100644 --- a/source/common/http/filter/BUILD +++ b/source/common/http/filter/BUILD @@ -53,10 +53,10 @@ envoy_cc_library( "//include/envoy/http:filter_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:assert_lib", - "//source/common/common:logger_lib", "//source/common/common:enum_to_int", + "//source/common/common:logger_lib", "//source/common/http:message_lib", - "//source/common/http:utility_lib" + "//source/common/http:utility_lib", ], ) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index bbc3effb3c30..9fef00926a9c 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -8,6 +8,27 @@ namespace Envoy { namespace Http { +/** + * A pass-through filter that talks to an external authn/authz service. + * + * When Envoy receives a request for which this filter is enabled, an + * asynchronous request with the same HTTP method and headers, but an empty + * body, is made to the configured external auth service. The original + * request is stalled until the auth request completes. + * + * If the auth request returns HTTP 200, the original request is allowed + * to continue. If any headers are listed in the extauth filter's "headers" + * array, those headers will be copied from the auth response into the + * original request (overwriting any duplicate headers). + * + * If the auth request returns anything other than HTTP 200, the original + * request is rejected. The full response from the auth service is returned + * as the response to the rejected request. + * + * Note that at present, a call to the external service is made for _every + * request_ being routed. + */ + static LowerCaseString header_to_add(std::string("x-ambassador-calltype")); static LowerCaseString value_to_add(std::string("extauth-request")); @@ -15,28 +36,58 @@ ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {} ExtAuth::~ExtAuth() { ASSERT(!auth_request_); } -void ExtAuth::dumpHeaders(const char *what, HeaderMap* headers) { - log().info("ExtAuth headers ({}):", what); +/* dump headers for debugging */ +void ExtAuth::dumpHeaders(const char* what, HeaderMap* headers) { +#ifndef NVLOG + ENVOY_STREAM_LOG(trace, "ExtAuth headers ({}):", *callbacks_, what); headers->iterate( - [](const HeaderEntry& header, void* context) -> void { - (void)context; - log().trace(" '{}':'{}'", - header.key().c_str(), header.value().c_str()); - }, - nullptr); + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + ENVOY_STREAM_LOG(trace, " '{}':'{}'", *static_cast(context), + header.key().c_str(), header.value().c_str()); + return HeaderMap::Iterate::Continue; + }, + static_cast(callbacks_)); +#endif } +/* + * decodeHeaders is called at the point that the HTTP machinery handling + * the request has parsed the HTTP headers for this request. + * + * Our primary job here is to construct the request to the auth service + * and start it executing, but we also have to be sure to save a pointer + * to the incoming request headers in case we need to modify them in + * flight. + */ + FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { + // Remember that we have _not_ finished talking to the auth service... - // Request external authentication auth_complete_ = false; + + // ...and hang onto a pointer to the original request headers. + // + // Note that using a reference here won't work. Move semantics are the + // root of this issue, I think. + request_headers_ = &headers; + + // Debugging. dumpHeaders("decodeHeaders", request_headers_); + // OK, time to get the auth-service request set up. Create a + // RequestMessageImpl to hold all the details, and start it off as a + // copy of the incoming request's headers. + MessagePtr reqmsg(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); + // We do need to tweak a couple of things. To start with, has a change + // to the path we hand to the auth service been configured? + if (!config_->path_prefix_.empty()) { + // Yes, it has. Go ahead and prepend it to the reqmsg path. + std::string path = reqmsg->headers().insertPath().value().c_str(); path = config_->path_prefix_ + path; @@ -44,25 +95,41 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { reqmsg->headers().insertPath().value(path); } - // reqmsg->headers().insertMethod().value(Http::Headers::get().MethodValues.Post); - // reqmsg->headers().insertPath().value(std::string("/ambassador/auth")); - reqmsg->headers().insertHost().value(config_->cluster_); // cluster name is Host: header value! + // Next up, reset the Host: header to match the cluster name we're about + // to send the auth request to... + + reqmsg->headers().insertHost().value(config_->cluster_); + + // ...and make sure the body is correctly marked as empty. + reqmsg->headers().insertContentLength().value(uint64_t(0)); + // Finally, we mark the request as being an Ambassador auth request. + reqmsg->headers().addReference(header_to_add, value_to_add.get()); - log().info("ExtAuth contacting auth server"); - // reqmsg->body() = Buffer::InstancePtr(new Buffer::OwnedImpl(request_body)); + // Fire the request up. When it's finished, we'll get a call to + // either onSuccess() or onFailure(). + + ENVOY_STREAM_LOG(trace, "ExtAuth contacting auth server", *callbacks_); + auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) .send(std::move(reqmsg), *this, Optional(config_->timeout_)); - // .send(...) -> onSuccess(...) or onFailure(...) - // This handle can be used to ->cancel() the request. - // Stop until we have a result + // It'll take some time for our auth call to complete. Stop + // filtering while we wait for it. + return FilterHeadersStatus::StopIteration; } +/* + * decodeHeaders is called at the point that the HTTP machinery handling + * the request has parsed the HTTP body for this request. We don't need + * to do anything special here; we just need to make sure that we don't + * let things proceed until our auth call is done. + */ + FilterDataStatus ExtAuth::decodeData(Buffer::Instance&, bool) { if (auth_complete_) { return FilterDataStatus::Continue; @@ -70,6 +137,13 @@ FilterDataStatus ExtAuth::decodeData(Buffer::Instance&, bool) { return FilterDataStatus::StopIterationAndBuffer; } +/* + * decodeTrailers is called at the point that the HTTP machinery handling + * the request has parsed the HTTP trailers for this request. We don't need + * to do anything special here; we just need to make sure that we don't + * let things proceed until our auth call is done. + */ + FilterTrailersStatus ExtAuth::decodeTrailers(HeaderMap&) { if (auth_complete_) { return FilterTrailersStatus::Continue; @@ -77,50 +151,83 @@ FilterTrailersStatus ExtAuth::decodeTrailers(HeaderMap&) { return FilterTrailersStatus::StopIteration; } -ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Store& store) { +ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Scope& scope) { std::string final_prefix = prefix + "extauth."; - return {ALL_EXTAUTH_STATS(POOL_COUNTER_PREFIX(store, final_prefix))}; + return {ALL_EXTAUTH_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } +/* + * onSuccess is called when our asynch auth request succeeds, meaning + * "the HTTP protocol was successfully followed to completion" -- it + * could still be the case that the auth server gave us a failure + * response. + */ + void ExtAuth::onSuccess(Http::MessagePtr&& response) { + // We're done with our auth request, so make sure it gets shredded. auth_request_ = nullptr; dumpHeaders("onSuccess", request_headers_); + // What did we get back from the auth server? + uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); std::string response_body(response->bodyAsString()); - log().info("ExtAuth Auth responded with code {}", response_code); + ENVOY_STREAM_LOG(trace, "ExtAuth Auth responded with code {}", *callbacks_, response_code); if (!response_body.empty()) { - log().info("ExtAuth Auth said: {}", response->bodyAsString()); + ENVOY_STREAM_LOG(trace, "ExtAuth Auth said: {}", *callbacks_, response->bodyAsString()); } + // By definition, any response code other than 200, "OK", means we deny + // this request. + if (response_code != enumToInt(Http::Code::OK)) { - log().info("ExtAuth rejecting request"); + ENVOY_STREAM_LOG(debug, "ExtAuth rejecting request", *callbacks_); + + // Bump the rejection count... config_->stats_.rq_rejected_.inc(); + + // ...and ditch our pointer to the request headers. + request_headers_ = nullptr; + // Whatever the auth server replied, we're going to hand that back to the + // original requestor. That means both the header and the body; start by + // copying the headers... + Http::HeaderMapPtr response_headers{new HeaderMapImpl(response->headers())}; callbacks_->encodeHeaders(std::move(response_headers), response_body.empty()); + // ...and then copy the body, as well, if there is one. + if (!response_body.empty()) { Buffer::OwnedImpl buffer(response_body); callbacks_->encodeData(buffer, true); } + // ...ahhhhnd we're done. + return; } - log().info("ExtAuth accepting request"); + ENVOY_STREAM_LOG(debug, "ExtAuth accepting request", *callbacks_); + + // OK, we're going to approve this request, great! Next up: the filter can + // be configured to copy headers from the auth server to the requester. + // If that's configured, we need to take care of that now -- and if we actually + // copy any headers, we'll need to be sure to invalidate the route cache. + // (If we don't copy any headers, we should leave the route cache alone.) + bool addedHeaders = false; // Do we have any headers configured to copy? if (!config_->allowed_headers_.empty()) { - // Yup. Let's see if any of them are present. + // Yup. Let's see if any of them are present. for (std::string allowed_header : config_->allowed_headers_) { LowerCaseString key(allowed_header); @@ -139,7 +246,8 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { std::string valstr(value.c_str()); - ENVOY_STREAM_LOG(trace, "ExtAuth allowing response header {}: {}", *callbacks_, allowed_header, valstr); + ENVOY_STREAM_LOG(trace, "ExtAuth allowing response header {}: {}", *callbacks_, + allowed_header, valstr); addedHeaders = true; request_headers_->addCopy(key, valstr); } @@ -148,7 +256,7 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { } if (addedHeaders) { - // Yup, we added headers. Invalidate the route cache in case any of + // Yup, we added headers. Invalidate the route cache in case any of // the headers will affect routing decisions. dumpHeaders("ExtAuth invalidating route cache", request_headers_); @@ -156,16 +264,24 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { callbacks_->clearRouteCache(); } + // Finally done. Bump the "passed" stat... config_->stats_.rq_passed_.inc(); + + // ...remember that auth is done... auth_complete_ = true; + + // ...clear our request-header pointer now that we're finished with + // this request... request_headers_ = nullptr; + + // ...and allow everything to continue. callbacks_->continueDecoding(); } void ExtAuth::onFailure(Http::AsyncClient::FailureReason) { auth_request_ = nullptr; request_headers_ = nullptr; - log().warn("ExtAuth Auth request failed"); + ENVOY_STREAM_LOG(warn, "ExtAuth Auth request failed", *callbacks_); config_->stats_.rq_failed_.inc(); Http::Utility::sendLocalReply(*callbacks_, false, Http::Code::ServiceUnavailable, std::string("Auth request failed.")); @@ -182,5 +298,5 @@ void ExtAuth::setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) callbacks_ = &callbacks; } -} // Http -} // Envoy +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index a4b005f70b36..ce057117a2e8 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -8,6 +8,27 @@ namespace Envoy { namespace Http { +/** + * A pass-through filter that talks to an external authn/authz service. + * + * When Envoy receives a request for which this filter is enabled, an + * asynchronous request with the same HTTP method and headers, but an empty + * body, is made to the configured external auth service. The original + * request is stalled until the auth request completes. + * + * If the auth request returns HTTP 200, the original request is allowed + * to continue. If any headers are listed in the extauth filter's "headers" + * array, those headers will be copied from the auth response into the + * original request (overwriting any duplicate headers). + * + * If the auth request returns anything other than HTTP 200, the original + * request is rejected. The full response from the auth service is returned + * as the response to the rejected request. + * + * Note that at present, a call to the external service is made for _every + * request_ being routed. + */ + /** * All stats for the extauth filter. @see stats_macros.h */ @@ -41,7 +62,7 @@ struct ExtAuthConfig { typedef std::shared_ptr ExtAuthConfigConstSharedPtr; /** - * A pass-through filter that talks to an external authn/authz service (or will soon...) + * ExtAuth filter itself. */ class ExtAuth : Logger::Loggable, public StreamDecoderFilter, @@ -50,7 +71,7 @@ class ExtAuth : Logger::Loggable, ExtAuth(ExtAuthConfigConstSharedPtr config); ~ExtAuth(); - static ExtAuthStats generateStats(const std::string& prefix, Stats::Store& store); + static ExtAuthStats generateStats(const std::string& prefix, Stats::Scope& scope); // Http::StreamFilterBase void onDestroy() override; @@ -66,7 +87,7 @@ class ExtAuth : Logger::Loggable, void onFailure(Http::AsyncClient::FailureReason reason) override; private: - void dumpHeaders(const char *what, HeaderMap* headers); + void dumpHeaders(const char* what, HeaderMap* headers); ExtAuthConfigConstSharedPtr config_; StreamDecoderFilterCallbacks* callbacks_{}; @@ -76,4 +97,4 @@ class ExtAuth : Logger::Loggable, }; } // Http -} // Envoy +} // namespace Envoy diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 25341b61c983..2055240639be 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -79,7 +79,7 @@ envoy_cc_library( deps = [ "//source/common/http/filter:extauth_lib", "//source/server:configuration_lib", - "//source/server/config/network:http_connection_manager_lib" + "//source/server/config/network:http_connection_manager_lib", ], ) diff --git a/source/server/config/http/extauth_config.cc b/source/server/config/http/extauth_config.cc index 95adedd0ad40..71b16c4904d8 100644 --- a/source/server/config/http/extauth_config.cc +++ b/source/server/config/http/extauth_config.cc @@ -1,5 +1,9 @@ #include "server/config/http/extauth_config.h" + +#include "envoy/registry/registry.h" + #include "common/http/filter/extauth.h" +#include "common/json/config_schemas.h" namespace Envoy { namespace Server { @@ -27,28 +31,20 @@ const std::string EXTAUTH_HTTP_FILTER_SCHEMA(R"EOF( } )EOF"); -HttpFilterFactoryCb ExtAuthConfig::tryCreateFilterFactory(HttpFilterType type, - const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) { - if (type != HttpFilterType::Decoder || name != "extauth") { - return nullptr; - } - +HttpFilterFactoryCb ExtAuthConfig::createFilterFactory(const Json::Object& json_config, + const std::string& stats_prefix, + FactoryContext& context) { json_config.validateSchema(EXTAUTH_HTTP_FILTER_SCHEMA); - std::string prefix = - json_config.hasObject("path_prefix") ? json_config.getString("path_prefix") : ""; + std::string prefix = + json_config.hasObject("path_prefix") ? json_config.getString("path_prefix") : ""; Http::ExtAuthConfigConstSharedPtr config(new Http::ExtAuthConfig{ - server.clusterManager(), - Http::ExtAuth::generateStats(stats_prefix, server.stats()), + context.clusterManager(), Http::ExtAuth::generateStats(stats_prefix, context.scope()), json_config.getString("cluster"), std::chrono::milliseconds(json_config.getInteger("timeout_ms")), - json_config.getStringArray("allowed_headers", true), - prefix - }); + json_config.getStringArray("allowed_headers", true), prefix}); + return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{new Http::ExtAuth(config)}); }; @@ -57,8 +53,8 @@ HttpFilterFactoryCb ExtAuthConfig::tryCreateFilterFactory(HttpFilterType type, /** * Static registration for the extauth filter. @see RegisterHttpFilterConfigFactory. */ -static RegisterHttpFilterConfigFactory register_; +static Registry::RegisterFactory register_; -} // Configuration -} // Server -} // Envoy +} // namespace Configuration +} // namespace Server +} // namespace Envoy diff --git a/source/server/config/http/extauth_config.h b/source/server/config/http/extauth_config.h index 48a1e6defb7f..5eeaaf50beba 100644 --- a/source/server/config/http/extauth_config.h +++ b/source/server/config/http/extauth_config.h @@ -2,9 +2,11 @@ #include -#include "envoy/server/instance.h" +#include "envoy/server/filter_config.h" -#include "server/config/network/http_connection_manager.h" +// #include "envoy/server/instance.h" + +// #include "server/config/network/http_connection_manager.h" namespace Envoy { namespace Server { @@ -13,14 +15,14 @@ namespace Configuration { /** * Config registration for the ExtAuth filter. @see HttpFilterConfigFactory. */ -class ExtAuthConfig : public HttpFilterConfigFactory { +class ExtAuthConfig : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb tryCreateFilterFactory(HttpFilterType type, const std::string& name, - const Json::Object& json_config, - const std::string& stats_prefix, - Server::Instance& server) override; + std::string name() override { return "extauth"; } + + HttpFilterFactoryCb createFilterFactory(const Json::Object& json_config, + const std::string& stats_prefix, FactoryContext& context); }; -} // Configuration -} // Server -} // Envoy +} // namespace Configuration +} // namespace Server +} // namespace Envoy From b7ce76851dfb2c15a335c0ee7f799cefdb15a8b2 Mon Sep 17 00:00:00 2001 From: Flynn Date: Mon, 20 Nov 2017 12:35:47 -0500 Subject: [PATCH 08/16] Do not overwrite the Host header when talking to the extauth service (fixes https://github.com/datawire/ambassador/issues/154) Signed-off-by: Flynn --- source/common/http/filter/extauth.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index 9fef00926a9c..c85a154c5710 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -95,12 +95,18 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { reqmsg->headers().insertPath().value(path); } - // Next up, reset the Host: header to match the cluster name we're about - // to send the auth request to... - - reqmsg->headers().insertHost().value(config_->cluster_); + // https://github.com/datawire/ambassador/issues/154 + // We used to reset the Host: header to match the cluster name we're about + // to send the auth request to. That, however, causes trouble for anyone + // who wants to make auth decisions based on the host to which the client + // started out trying to talk to. + // + // We may need to make this configurable later, so I'm leaving this line + // in for reference. + // reqmsg->headers().insertHost().value(config_->cluster_); - // ...and make sure the body is correctly marked as empty. + // After setting up whatever headers we need to, make sure the body is + // correctly marked as empty. reqmsg->headers().insertContentLength().value(uint64_t(0)); From 70e29a50002402eece9c06cc1e6dfae6897968a5 Mon Sep 17 00:00:00 2001 From: Flynn Date: Thu, 15 Feb 2018 11:31:49 -0500 Subject: [PATCH 09/16] Re-sort for check_format. Sigh. SIgned-off-by: Flynn --- source/exe/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/exe/BUILD b/source/exe/BUILD index bcf477045756..179c8e23b073 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -37,8 +37,8 @@ envoy_cc_library( "//source/server/config/access_log:grpc_access_log_lib", "//source/server/config/http:buffer_lib", "//source/server/config/http:cors_lib", - "//source/server/config/http:fault_lib", "//source/server/config/http:extauth_lib", + "//source/server/config/http:fault_lib", "//source/server/config/http:grpc_http1_bridge_lib", "//source/server/config/http:grpc_json_transcoder_lib", "//source/server/config/http:grpc_web_lib", From 0f18d0b8190cb55325b24a13f4f74e3f0f304656 Mon Sep 17 00:00:00 2001 From: Flynn Date: Fri, 16 Feb 2018 01:04:42 -0500 Subject: [PATCH 10/16] Let's see about fixing the clang errors from CI. Signed-off-by: Flynn --- source/server/config/http/extauth_config.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/server/config/http/extauth_config.h b/source/server/config/http/extauth_config.h index 5eeaaf50beba..87a0923e8b58 100644 --- a/source/server/config/http/extauth_config.h +++ b/source/server/config/http/extauth_config.h @@ -20,7 +20,8 @@ class ExtAuthConfig : public NamedHttpFilterConfigFactory { std::string name() override { return "extauth"; } HttpFilterFactoryCb createFilterFactory(const Json::Object& json_config, - const std::string& stats_prefix, FactoryContext& context); + const std::string& stats_prefix, + FactoryContext& context) override; }; } // namespace Configuration From 7a0d470a89b3c7fa3ba6b61c3145e533bfd51511 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Mon, 19 Feb 2018 23:25:02 -0800 Subject: [PATCH 11/16] filter_auth: initial code review changes Signed-off-by: Gabriel --- source/common/http/filter/extauth.cc | 133 +++++++++++---------------- source/common/http/filter/extauth.h | 4 +- 2 files changed, 58 insertions(+), 79 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index c85a154c5710..967e6e3bd29f 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -5,63 +5,52 @@ #include "common/http/message_impl.h" #include "common/http/utility.h" +#include "absl/strings/str_cat.h" + namespace Envoy { namespace Http { -/** - * A pass-through filter that talks to an external authn/authz service. - * - * When Envoy receives a request for which this filter is enabled, an - * asynchronous request with the same HTTP method and headers, but an empty - * body, is made to the configured external auth service. The original - * request is stalled until the auth request completes. - * - * If the auth request returns HTTP 200, the original request is allowed - * to continue. If any headers are listed in the extauth filter's "headers" - * array, those headers will be copied from the auth response into the - * original request (overwriting any duplicate headers). - * - * If the auth request returns anything other than HTTP 200, the original - * request is rejected. The full response from the auth service is returned - * as the response to the rejected request. - * - * Note that at present, a call to the external service is made for _every - * request_ being routed. - */ - -static LowerCaseString header_to_add(std::string("x-ambassador-calltype")); -static LowerCaseString value_to_add(std::string("extauth-request")); - -ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(config) {} +namespace { + +const LowerCaseString& header_to_add() { + CONSTRUCT_ON_FIRST_USE(LowerCaseString, "x-ambassador-calltype"); +} + +const std::string& value_to_add() { CONSTRUCT_ON_FIRST_USE(std::string, "extauth-request"); } + +} // namespace + +ExtAuth::ExtAuth(const ExtAuthConfigConstSharedPtr& config) : config_(config) {} ExtAuth::~ExtAuth() { ASSERT(!auth_request_); } -/* dump headers for debugging */ void ExtAuth::dumpHeaders(const char* what, HeaderMap* headers) { #ifndef NVLOG ENVOY_STREAM_LOG(trace, "ExtAuth headers ({}):", *callbacks_, what); - - headers->iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - ENVOY_STREAM_LOG(trace, " '{}':'{}'", *static_cast(context), - header.key().c_str(), header.value().c_str()); - return HeaderMap::Iterate::Continue; - }, - static_cast(callbacks_)); + if (headers) { + headers->iterate( + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + ENVOY_STREAM_LOG(trace, " '{}':'{}'", + *static_cast(context), + header.key().c_str(), header.value().c_str()); + return HeaderMap::Iterate::Continue; + }, + static_cast(callbacks_)); + } #endif } -/* - * decodeHeaders is called at the point that the HTTP machinery handling - * the request has parsed the HTTP headers for this request. - * - * Our primary job here is to construct the request to the auth service - * and start it executing, but we also have to be sure to save a pointer - * to the incoming request headers in case we need to modify them in - * flight. - */ - +// TODO(gsagula): at the end of this PR, most of the comments inside member functions should be +// removed. FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { + + // decodeHeaders is called at the point that the HTTP machinery handling + // the request has parsed the HTTP headers for this request. + // Our primary job here is to construct the request to the auth service + // and start it executing, but we also have to be sure to save a pointer + // to the incoming request headers in case we need to modify them in + // flight. + // Remember that we have _not_ finished talking to the auth service... auth_complete_ = false; @@ -80,19 +69,16 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { // RequestMessageImpl to hold all the details, and start it off as a // copy of the incoming request's headers. - MessagePtr reqmsg(new RequestMessageImpl(HeaderMapPtr{new HeaderMapImpl(headers)})); + MessagePtr request_message{new RequestMessageImpl{HeaderMapPtr{new HeaderMapImpl{headers}}}}; // We do need to tweak a couple of things. To start with, has a change // to the path we hand to the auth service been configured? if (!config_->path_prefix_.empty()) { - // Yes, it has. Go ahead and prepend it to the reqmsg path. - - std::string path = reqmsg->headers().insertPath().value().c_str(); - - path = config_->path_prefix_ + path; - - reqmsg->headers().insertPath().value(path); + // Yes, it has. Go ahead and prepend it to the request_message path. + std::string path; + absl::StrAppend(&path, config_->path_prefix_, request_message->headers().insertPath().value().c_str()); + request_message->headers().insertPath().value(path); } // https://github.com/datawire/ambassador/issues/154 @@ -103,16 +89,16 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { // // We may need to make this configurable later, so I'm leaving this line // in for reference. - // reqmsg->headers().insertHost().value(config_->cluster_); + // request_message->headers().insertHost().value(config_->cluster_); // After setting up whatever headers we need to, make sure the body is // correctly marked as empty. - reqmsg->headers().insertContentLength().value(uint64_t(0)); + request_message->headers().insertContentLength().value(uint64_t(0)); // Finally, we mark the request as being an Ambassador auth request. - reqmsg->headers().addReference(header_to_add, value_to_add.get()); + request_message->headers().addReference(header_to_add(), value_to_add()); // Fire the request up. When it's finished, we'll get a call to // either onSuccess() or onFailure(). @@ -121,7 +107,7 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) - .send(std::move(reqmsg), *this, Optional(config_->timeout_)); + .send(std::move(request_message), *this, Optional(config_->timeout_)); // It'll take some time for our auth call to complete. Stop // filtering while we wait for it. @@ -129,28 +115,22 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { return FilterHeadersStatus::StopIteration; } -/* - * decodeHeaders is called at the point that the HTTP machinery handling - * the request has parsed the HTTP body for this request. We don't need - * to do anything special here; we just need to make sure that we don't - * let things proceed until our auth call is done. - */ - FilterDataStatus ExtAuth::decodeData(Buffer::Instance&, bool) { + // decodeHeaders is called at the point that the HTTP machinery handling + // the request has parsed the HTTP body for this request. We don't need + // to do anything special here; we just need to make sure that we don't + // let things proceed until our auth call is done. if (auth_complete_) { return FilterDataStatus::Continue; } return FilterDataStatus::StopIterationAndBuffer; } -/* - * decodeTrailers is called at the point that the HTTP machinery handling - * the request has parsed the HTTP trailers for this request. We don't need - * to do anything special here; we just need to make sure that we don't - * let things proceed until our auth call is done. - */ - FilterTrailersStatus ExtAuth::decodeTrailers(HeaderMap&) { + // decodeTrailers is called at the point that the HTTP machinery handling + // the request has parsed the HTTP trailers for this request. We don't need + // to do anything special here; we just need to make sure that we don't + // let things proceed until our auth call is done. if (auth_complete_) { return FilterTrailersStatus::Continue; } @@ -162,14 +142,13 @@ ExtAuthStats ExtAuth::generateStats(const std::string& prefix, Stats::Scope& sco return {ALL_EXTAUTH_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } -/* - * onSuccess is called when our asynch auth request succeeds, meaning - * "the HTTP protocol was successfully followed to completion" -- it - * could still be the case that the auth server gave us a failure - * response. - */ - void ExtAuth::onSuccess(Http::MessagePtr&& response) { + + // onSuccess is called when our asynch auth request succeeds, meaning + // "the HTTP protocol was successfully followed to completion" -- it + // could still be the case that the auth server gave us a failure + // response. + // We're done with our auth request, so make sure it gets shredded. auth_request_ = nullptr; diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index ce057117a2e8..b3fad92f9a7e 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -68,7 +68,7 @@ class ExtAuth : Logger::Loggable, public StreamDecoderFilter, public Http::AsyncClient::Callbacks { public: - ExtAuth(ExtAuthConfigConstSharedPtr config); + ExtAuth(const ExtAuthConfigConstSharedPtr& config); ~ExtAuth(); static ExtAuthStats generateStats(const std::string& prefix, Stats::Scope& scope); @@ -87,7 +87,7 @@ class ExtAuth : Logger::Loggable, void onFailure(Http::AsyncClient::FailureReason reason) override; private: - void dumpHeaders(const char* what, HeaderMap* headers); + void dumpHeaders(const char* what, HeaderMap* headers); // dump headers for debugging ExtAuthConfigConstSharedPtr config_; StreamDecoderFilterCallbacks* callbacks_{}; From 3e020b666031edf2df8178b64a8ba38d80644d6f Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 21 Feb 2018 20:44:35 -0800 Subject: [PATCH 12/16] filter_auth: code review changes Signed-off-by: Gabriel --- source/common/http/filter/extauth.cc | 23 ++++++++++++----------- source/common/http/filter/extauth.h | 4 ++-- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index 967e6e3bd29f..a7e1d6b4e147 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -12,15 +12,15 @@ namespace Http { namespace { -const LowerCaseString& header_to_add() { +const LowerCaseString header_to_add() { CONSTRUCT_ON_FIRST_USE(LowerCaseString, "x-ambassador-calltype"); } -const std::string& value_to_add() { CONSTRUCT_ON_FIRST_USE(std::string, "extauth-request"); } +const std::string value_to_add() { CONSTRUCT_ON_FIRST_USE(std::string, "extauth-request"); } } // namespace -ExtAuth::ExtAuth(const ExtAuthConfigConstSharedPtr& config) : config_(config) {} +ExtAuth::ExtAuth(ExtAuthConfigConstSharedPtr config) : config_(std::move(config)) {} ExtAuth::~ExtAuth() { ASSERT(!auth_request_); } @@ -77,7 +77,8 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { if (!config_->path_prefix_.empty()) { // Yes, it has. Go ahead and prepend it to the request_message path. std::string path; - absl::StrAppend(&path, config_->path_prefix_, request_message->headers().insertPath().value().c_str()); + absl::StrAppend(&path, config_->path_prefix_, + request_message->headers().insertPath().value().getString()); request_message->headers().insertPath().value(path); } @@ -105,9 +106,9 @@ FilterHeadersStatus ExtAuth::decodeHeaders(HeaderMap& headers, bool) { ENVOY_STREAM_LOG(trace, "ExtAuth contacting auth server", *callbacks_); - auth_request_ = - config_->cm_.httpAsyncClientForCluster(config_->cluster_) - .send(std::move(request_message), *this, Optional(config_->timeout_)); + auth_request_ = config_->cm_.httpAsyncClientForCluster(config_->cluster_) + .send(std::move(request_message), *this, + Optional(config_->timeout_)); // It'll take some time for our auth call to complete. Stop // filtering while we wait for it. @@ -157,11 +158,11 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { // What did we get back from the auth server? uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); - std::string response_body(response->bodyAsString()); + std::string response_body(std::move(response->bodyAsString())); ENVOY_STREAM_LOG(trace, "ExtAuth Auth responded with code {}", *callbacks_, response_code); - if (!response_body.empty()) { + if (!response->body()->length()) { ENVOY_STREAM_LOG(trace, "ExtAuth Auth said: {}", *callbacks_, response->bodyAsString()); } @@ -214,7 +215,7 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { if (!config_->allowed_headers_.empty()) { // Yup. Let's see if any of them are present. - for (std::string allowed_header : config_->allowed_headers_) { + for (const std::string& allowed_header : config_->allowed_headers_) { LowerCaseString key(allowed_header); // OK, do we have this header? @@ -229,7 +230,7 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { if (!value.empty()) { // Not empty! Copy it into our request_headers_. - std::string valstr(value.c_str()); + std::string valstr{value.c_str()}; ENVOY_STREAM_LOG(trace, "ExtAuth allowing response header {}: {}", *callbacks_, allowed_header, valstr); diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index b3fad92f9a7e..6638d48ec828 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -64,11 +64,11 @@ typedef std::shared_ptr ExtAuthConfigConstSharedPtr; /** * ExtAuth filter itself. */ -class ExtAuth : Logger::Loggable, +class ExtAuth : public Logger::Loggable, public StreamDecoderFilter, public Http::AsyncClient::Callbacks { public: - ExtAuth(const ExtAuthConfigConstSharedPtr& config); + ExtAuth(ExtAuthConfigConstSharedPtr config); ~ExtAuth(); static ExtAuthStats generateStats(const std::string& prefix, Stats::Scope& scope); From 44ddcb9bf032dc0b44f155e0ccd4f614fd31bf96 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 21 Feb 2018 22:02:56 -0800 Subject: [PATCH 13/16] filter_auth: changed server config to use std make_shared Signed-off-by: Gabriel --- source/server/config/http/extauth_config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/config/http/extauth_config.cc b/source/server/config/http/extauth_config.cc index 71b16c4904d8..a713aaa2b742 100644 --- a/source/server/config/http/extauth_config.cc +++ b/source/server/config/http/extauth_config.cc @@ -46,7 +46,7 @@ HttpFilterFactoryCb ExtAuthConfig::createFilterFactory(const Json::Object& json_ json_config.getStringArray("allowed_headers", true), prefix}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr{new Http::ExtAuth(config)}); + callbacks.addStreamDecoderFilter(std::make_shared(config)); }; } From e332b8aa3d73f09f696c2057a22880633bce9835 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 21 Feb 2018 22:45:50 -0800 Subject: [PATCH 14/16] filter_auth: fixed sanitizer errors Signed-off-by: Gabriel --- source/common/http/filter/extauth.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/filter/extauth.cc b/source/common/http/filter/extauth.cc index a7e1d6b4e147..ac2eaadfae9d 100644 --- a/source/common/http/filter/extauth.cc +++ b/source/common/http/filter/extauth.cc @@ -158,7 +158,7 @@ void ExtAuth::onSuccess(Http::MessagePtr&& response) { // What did we get back from the auth server? uint64_t response_code = Http::Utility::getResponseStatus(response->headers()); - std::string response_body(std::move(response->bodyAsString())); + std::string response_body = response->bodyAsString(); ENVOY_STREAM_LOG(trace, "ExtAuth Auth responded with code {}", *callbacks_, response_code); From 858548934f4849c324985fb428e94f731b244da2 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 21 Feb 2018 23:15:47 -0800 Subject: [PATCH 15/16] filter_auth: request_headers_ initialization Signed-off-by: Gabriel --- source/common/http/filter/extauth.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/filter/extauth.h b/source/common/http/filter/extauth.h index 6638d48ec828..b4c6e82e34f2 100644 --- a/source/common/http/filter/extauth.h +++ b/source/common/http/filter/extauth.h @@ -93,7 +93,7 @@ class ExtAuth : public Logger::Loggable, StreamDecoderFilterCallbacks* callbacks_{}; bool auth_complete_; Http::AsyncClient::Request* auth_request_{}; - Http::HeaderMap* request_headers_; + Http::HeaderMap* request_headers_{}; }; } // Http From f36184e36f76bf7efbf448e716528f0c814c56b4 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 22 Feb 2018 07:29:09 -0800 Subject: [PATCH 16/16] forcing ci. Signed-off-by: Gabriel