From 3d4f7aadfc5df51204faa2f9c398dc1705b8a7b6 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 24 Jul 2019 11:56:42 -0700 Subject: [PATCH 01/32] Initial ABAC filter Signed-off-by: Kuat Yessenov --- CODEOWNERS | 2 + .../config/filter/http/abac/v2alpha/BUILD | 15 ++ .../filter/http/abac/v2alpha/abac.proto | 18 ++ bazel/repositories.bzl | 4 + bazel/repository_locations.bzl | 31 +++- source/common/common/logger.h | 1 + source/extensions/extensions_build_config.bzl | 1 + source/extensions/filters/http/abac/BUILD | 37 ++++ .../filters/http/abac/abac_filter.cc | 171 ++++++++++++++++++ .../filters/http/abac/abac_filter.h | 54 ++++++ source/extensions/filters/http/abac/config.cc | 53 ++++++ source/extensions/filters/http/abac/config.h | 31 ++++ .../filters/http/well_known_names.h | 2 + 13 files changed, 416 insertions(+), 4 deletions(-) create mode 100644 api/envoy/config/filter/http/abac/v2alpha/BUILD create mode 100644 api/envoy/config/filter/http/abac/v2alpha/abac.proto create mode 100644 source/extensions/filters/http/abac/BUILD create mode 100644 source/extensions/filters/http/abac/abac_filter.cc create mode 100644 source/extensions/filters/http/abac/abac_filter.h create mode 100644 source/extensions/filters/http/abac/config.cc create mode 100644 source/extensions/filters/http/abac/config.h diff --git a/CODEOWNERS b/CODEOWNERS index a252439569d6c..2529287a3208b 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -20,6 +20,8 @@ extensions/filters/common/original_src @snowp @klarose /*/extensions/filters/network/thrift_proxy @zuercher @brian-pane # jwt_authn http filter extension /*/extensions/filters/http/jwt_authn @qiwzhang @lizan +# abac http filter extension +/*/extensions/filters/http/abac @kyessenov @PiotrSikora # grpc_http1_reverse_bridge http filter extension /*/extensions/filters/http/grpc_http1_reverse_bridge @snowp @zuercher # header_to_metadata extension diff --git a/api/envoy/config/filter/http/abac/v2alpha/BUILD b/api/envoy/config/filter/http/abac/v2alpha/BUILD new file mode 100644 index 0000000000000..02b407e3ea54c --- /dev/null +++ b/api/envoy/config/filter/http/abac/v2alpha/BUILD @@ -0,0 +1,15 @@ +load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal") + +licenses(["notice"]) # Apache 2 + +api_proto_library_internal( + name = "abac", + srcs = ["abac.proto"], + external_cc_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", + ], + external_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", + ], + require_py = False, +) diff --git a/api/envoy/config/filter/http/abac/v2alpha/abac.proto b/api/envoy/config/filter/http/abac/v2alpha/abac.proto new file mode 100644 index 0000000000000..0e767a496c0e9 --- /dev/null +++ b/api/envoy/config/filter/http/abac/v2alpha/abac.proto @@ -0,0 +1,18 @@ +syntax = "proto3"; + +package envoy.config.filter.http.abac.v2alpha; + +option java_outer_classname = "AbacProto"; +option java_multiple_files = true; +option java_package = "io.envoyproxy.envoy.config.filter.http.abac.v2alpha"; +option go_package = "v2alpha"; + +import "google/api/expr/v1alpha1/syntax.proto"; + +// [#protodoc-title: ABAC] +// Attribute-Based Access Control. + +// ABAC filter config. +message ABAC { + google.api.expr.v1alpha1.Expr expr = 1; +} diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index fe86b72263d81..d72c85fc25a07 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -156,6 +156,10 @@ def envoy_dependencies(skip_targets = []): _com_lightstep_tracer_cpp() _io_opentracing_cpp() _net_zlib() + _repository_impl("com_github_gflags_gflags") + _repository_impl("com_google_glog") + _repository_impl("com_google_re2") + _repository_impl("com_google_cel_cpp") _repository_impl("bazel_toolchains") _python_deps() diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index a9db5ba2a0aca..d6efa9a0f1f66 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -24,10 +24,10 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://commondatastorage.googleapis.com/chromium-boringssl-docs/fips/boringssl-66005f41fbc3529ffe8d007708756720529da20d.tar.xz"], ), com_google_absl = dict( - sha256 = "7ddf863ddced6fa5bf7304103f9c7aa619c20a2fcf84475512c8d3834b9d14fa", - strip_prefix = "abseil-cpp-61c9bf3e3e1c28a4aa6d7f1be4b37fd473bb5529", - # 2019-06-05 - urls = ["https://github.com/abseil/abseil-cpp/archive/61c9bf3e3e1c28a4aa6d7f1be4b37fd473bb5529.tar.gz"], + sha256 = "05a97ad5bb123ee3f6d65c7b06f6d7de5ce62e4fb971cbe4b5e391dd69704bb7", + strip_prefix = "abseil-cpp-ad1485c8986246b2ae9105e512738d0e97aec887", + # 2019-07-24 + urls = ["https://github.com/abseil/abseil-cpp/archive/ad1485c8986246b2ae9105e512738d0e97aec887.tar.gz"], ), com_github_apache_thrift = dict( sha256 = "7d59ac4fdcb2c58037ebd4a9da5f9a49e3e034bf75b3f26d9fe48ba3d8806e6b", @@ -247,4 +247,27 @@ REPOSITORY_LOCATIONS = dict( sha256 = "fcdebf54c89d839ffa7eefae166c8e4b551c765559db13ff15bff98047f344fb", urls = ["https://storage.googleapis.com/quiche-envoy-integration/2a930469533c3b541443488a629fe25cd8ff53d0.tar.gz"], ), + com_github_gflags_gflags = dict( + sha256 = "6e16c8bc91b1310a44f3965e616383dbda48f83e8c1eaa2370a215057b00cabe", + strip_prefix = "gflags-77592648e3f3be87d6c7123eb81cbad75f9aef5a", + urls = [ + "https://mirror.bazel.build/github.com/gflags/gflags/archive/77592648e3f3be87d6c7123eb81cbad75f9aef5a.tar.gz", + "https://github.com/gflags/gflags/archive/77592648e3f3be87d6c7123eb81cbad75f9aef5a.tar.gz", + ], + ), + com_google_glog = dict( + sha256 = "819cb075d6b02b8e9c9c77c2be1d55cef7fec47e7c94359d2626f46268ae67bf", + strip_prefix = "glog-ba8a9f6952d04d1403b97df24e6836227751454e", + urls = ["https://github.com/google/glog/archive/ba8a9f6952d04d1403b97df24e6836227751454e.tar.gz"], + ), + com_google_cel_cpp = dict( + sha256 = "37df0e66c84ddffe7bbac6e659856f48b34dbd9e1e869f57275493a20b271d11", + strip_prefix = "cel-cpp-592129c73f70de6d5ff8977aa94e030a1a9c4e8a", + urls = ["https://github.com/google/cel-cpp/archive/592129c73f70de6d5ff8977aa94e030a1a9c4e8a.tar.gz"], + ), + com_google_re2 = dict( + sha256 = "f31db9cd224d018a7e4fe88ef84aaa874b0b3ed91d4d98ee5a1531101d3fdc64", + strip_prefix = "re2-87e2ad45e7b18738e1551474f7ee5886ff572059", + urls = ["https://github.com/google/re2/archive/87e2ad45e7b18738e1551474f7ee5886ff572059.tar.gz"], + ), ) diff --git a/source/common/common/logger.h b/source/common/common/logger.h index dcc83627ac07d..acaa2d9d401ec 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -49,6 +49,7 @@ namespace Logger { FUNCTION(quic) \ FUNCTION(pool) \ FUNCTION(rbac) \ + FUNCTION(abac) \ FUNCTION(redis) \ FUNCTION(router) \ FUNCTION(runtime) \ diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index c18d6d6fa47ef..17dac24ba9cc5 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -49,6 +49,7 @@ EXTENSIONS = { "envoy.filters.http.original_src": "//source/extensions/filters/http/original_src:config", "envoy.filters.http.ratelimit": "//source/extensions/filters/http/ratelimit:config", "envoy.filters.http.rbac": "//source/extensions/filters/http/rbac:config", + "envoy.filters.http.abac": "//source/extensions/filters/http/abac:config", "envoy.filters.http.router": "//source/extensions/filters/http/router:config", "envoy.filters.http.squash": "//source/extensions/filters/http/squash:config", "envoy.filters.http.tap": "//source/extensions/filters/http/tap:config", diff --git a/source/extensions/filters/http/abac/BUILD b/source/extensions/filters/http/abac/BUILD new file mode 100644 index 0000000000000..108f62b86a973 --- /dev/null +++ b/source/extensions/filters/http/abac/BUILD @@ -0,0 +1,37 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], + deps = [ + ":abac_filter_lib", + "//include/envoy/registry", + "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/common:factory_base_lib", + "@com_google_cel_cpp//eval/public:builtin_func_registrar", + "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", + ], +) + +envoy_cc_library( + name = "abac_filter_lib", + srcs = ["abac_filter.cc"], + hdrs = ["abac_filter.h"], + deps = [ + "//include/envoy/http:filter_interface", + #"//include/envoy/stats:stats_macros", + "//source/common/http:utility_lib", + "//source/common/protobuf", + "@com_google_cel_cpp//eval/public:cel_expression", + "@envoy_api//envoy/config/filter/http/abac/v2alpha:abac_cc", + ], +) diff --git a/source/extensions/filters/http/abac/abac_filter.cc b/source/extensions/filters/http/abac/abac_filter.cc new file mode 100644 index 0000000000000..baf030067a4e6 --- /dev/null +++ b/source/extensions/filters/http/abac/abac_filter.cc @@ -0,0 +1,171 @@ +#include "extensions/filters/http/abac/abac_filter.h" + +#include "common/protobuf/protobuf.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace ABACFilter { + +using CelValue = google::api::expr::runtime::CelValue; +using CelMap = google::api::expr::runtime::CelMap; +using CelList = google::api::expr::runtime::CelList; + +namespace { + +constexpr absl::string_view kMetadata = "metadata"; +constexpr absl::string_view kConnection = "connection"; +constexpr absl::string_view kRequestedServerName = "requested_server_name"; +constexpr absl::string_view kLocalAddress = "local_address"; +constexpr absl::string_view kRemoteAddress = "remote_address"; +constexpr absl::string_view kCertificatePresented = "certificate_presented"; +constexpr absl::string_view kSubjectLocalCertificate = "subject_local_certificate"; +constexpr absl::string_view kIssuerPeerCertificate = "issuer_peer_certificate"; +constexpr absl::string_view kSubjectPeerCertificate = "subject_peer_certificate"; +constexpr absl::string_view kTlsVersion = "tls_version"; +constexpr absl::string_view kRequest = "request"; +constexpr absl::string_view kPath = "path"; +constexpr absl::string_view kHost = "host"; +constexpr absl::string_view kScheme = "scheme"; +constexpr absl::string_view kMethod = "method"; +constexpr absl::string_view kReferer = "referer"; +constexpr absl::string_view kHeaders = "headers"; +constexpr absl::string_view kTime = "time"; + +class ConnectionWrapper : public CelMap { +public: + ConnectionWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} + + absl::optional operator[](CelValue key) const override { + if (!key.IsString()) { + return {}; + } + auto value = key.StringOrDie().value(); + if (value == kRequestedServerName) { + return CelValue::CreateString(info_.requestedServerName()); + } else if (value == kLocalAddress) { + return CelValue::CreateString(info_.downstreamLocalAddress()->asString()); + } else if (value == kRemoteAddress) { + return CelValue::CreateString(info_.downstreamRemoteAddress()->asString()); + } + + auto ssl_info = info_.downstreamSslConnection(); + if (ssl_info != nullptr) { + if (value == kCertificatePresented) { + return CelValue::CreateBool(ssl_info->peerCertificatePresented()); + } else if (value == kSubjectLocalCertificate) { + return CelValue::CreateString(ssl_info->subjectLocalCertificate()); + } else if (value == kIssuerPeerCertificate) { + return CelValue::CreateString(ssl_info->issuerPeerCertificate()); + } else if (value == kSubjectPeerCertificate) { + return CelValue::CreateString(ssl_info->subjectPeerCertificate()); + } else if (value == kTlsVersion) { + return CelValue::CreateString(ssl_info->tlsVersion()); + } + } + return {}; + } + int size() const override { return 0; } + bool empty() const override { return false; } + const CelList* ListKeys() const override { return nullptr; } + +private: + const StreamInfo::StreamInfo& info_; +}; + +class HeadersWrapper : public CelMap { +public: + HeadersWrapper(const Http::HeaderMap& headers) : headers_(headers) {} + + absl::optional operator[](CelValue key) const override { + if (!key.IsString()) { + return {}; + } + auto out = headers_.get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); + if (out == nullptr) { + return {}; + } + return CelValue::CreateString(out->value().getStringView()); + } + int size() const override { return headers_.size(); } + bool empty() const override { return headers_.empty(); } + const CelList* ListKeys() const override { return nullptr; } + const Http::HeaderMap& headers_; +}; + +class RequestWrapper : public CelMap { +public: + RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) + : wrapper_(headers), info_(info) {} + + absl::optional operator[](CelValue key) const override { + if (!key.IsString()) { + return {}; + } + auto value = key.StringOrDie().value(); + if (value == kPath) { + return CelValue::CreateString(wrapper_.headers_.Path()->value().getStringView()); + } else if (value == kHost) { + return CelValue::CreateString(wrapper_.headers_.Host()->value().getStringView()); + } else if (value == kScheme) { + return CelValue::CreateString(wrapper_.headers_.Scheme()->value().getStringView()); + } else if (value == kMethod) { + return CelValue::CreateString(wrapper_.headers_.Method()->value().getStringView()); + } else if (value == kReferer) { + return CelValue::CreateString(wrapper_.headers_.Referer()->value().getStringView()); + } else if (value == kHeaders) { + return CelValue::CreateMap(&wrapper_); + } else if (value == kTime) { + return CelValue::CreateTimestamp(absl::FromChrono(info_.startTime())); + } + return {}; + } + + int size() const override { return 0; } + bool empty() const override { return false; } + const CelList* ListKeys() const override { return nullptr; } + +private: + const HeadersWrapper wrapper_; + const StreamInfo::StreamInfo& info_; +}; + +} // namespace + +Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http::HeaderMap& headers, + bool) { + Protobuf::Arena arena; + google::api::expr::runtime::Activation activation; + activation.InsertValue( + kMetadata, CelValue::CreateMessage(&callbacks_->streamInfo().dynamicMetadata(), &arena)); + const ConnectionWrapper connection(callbacks_->streamInfo()); + activation.InsertValue(kConnection, CelValue::CreateMap(&connection)); + const RequestWrapper request(headers, callbacks_->streamInfo()); + activation.InsertValue(kRequest, CelValue::CreateMap(&request)); + + auto eval_status = expr_->Evaluate(activation, &arena); + if (!eval_status.ok()) { + ENVOY_LOG(debug, "evaluation failed: {}", eval_status.message()); + return Http::FilterHeadersStatus::StopIteration; + } + + auto result = eval_status.ValueOrDie(); + if (result.IsBool() && result.BoolOrDie()) { + return Http::FilterHeadersStatus::Continue; + } + + if (result.IsError()) { + ENVOY_LOG(debug, "evaluation error: {}", result.ErrorOrDie()->message()); + } else if (result.IsString()) { + ENVOY_LOG(debug, "evaluation resulted in a string: {}", result.StringOrDie().value()); + } else { + ENVOY_LOG(debug, "expression did not evaluate to 'true': type {}", + CelValue::TypeName(result.type())); + } + return Http::FilterHeadersStatus::StopIteration; +} + +} // namespace ABACFilter +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/abac/abac_filter.h b/source/extensions/filters/http/abac/abac_filter.h new file mode 100644 index 0000000000000..7abcacae75af9 --- /dev/null +++ b/source/extensions/filters/http/abac/abac_filter.h @@ -0,0 +1,54 @@ +#pragma once + +#include + +#include "envoy/config/filter/http/abac/v2alpha/abac.pb.h" +#include "envoy/http/filter.h" + +//#include "envoy/stats/scope.h" +//#include "envoy/stats/stats_macros.h" + +#include "common/common/logger.h" + +#include "eval/public/cel_expression.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace ABACFilter { + +/** + * A filter that provides attribute-based access control authorization for HTTP requests. + */ +class AttributeBasedAccessControlFilter : public Http::StreamDecoderFilter, + public Logger::Loggable { +public: + AttributeBasedAccessControlFilter(std::shared_ptr expr) + : expr_(expr) {} + + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool) override; + + Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { + return Http::FilterDataStatus::Continue; + } + + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { + return Http::FilterTrailersStatus::Continue; + } + + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { + callbacks_ = &callbacks; + } + + // Http::StreamFilterBase + void onDestroy() override {} + +private: + Http::StreamDecoderFilterCallbacks* callbacks_{}; + std::shared_ptr expr_; +}; + +} // namespace ABACFilter +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/abac/config.cc b/source/extensions/filters/http/abac/config.cc new file mode 100644 index 0000000000000..3bedda9f25ea8 --- /dev/null +++ b/source/extensions/filters/http/abac/config.cc @@ -0,0 +1,53 @@ +#include "extensions/filters/http/abac/config.h" + +#include "envoy/registry/registry.h" + +#include "extensions/filters/http/abac/abac_filter.h" + +#include "eval/public/builtin_func_registrar.h" +#include "eval/public/cel_expr_builder_factory.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace ABACFilter { + +Http::FilterFactoryCb +AttributeBasedAccessControlFilterConfigFactory::createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::abac::v2alpha::ABAC& config, const std::string&, + Server::Configuration::FactoryContext&) { + google::api::expr::runtime::InterpreterOptions options; + + // Conformance with java/go runimes requires this setting + options.partial_string_match = true; + + auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); + auto register_status = + google::api::expr::runtime::RegisterBuiltinFunctions(builder->GetRegistry()); + if (!register_status.ok()) { + throw EnvoyException( + absl::StrCat("failed to register built-in functions: ", register_status.message())); + } + google::api::expr::v1alpha1::SourceInfo source_info; + auto cel_expression_status = builder->CreateExpression(&config.expr(), &source_info); + if (!cel_expression_status.ok()) { + throw EnvoyException( + absl::StrCat("failed to create an expression: ", cel_expression_status.status().message())); + } + auto cel_expression = absl::ShareUniquePtr(std::move(cel_expression_status.ValueOrDie())); + return [cel_expression](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter( + std::make_shared(cel_expression)); + }; +} + +/** + * Static registration for the ABAC filter. @see RegisterFactory + */ +REGISTER_FACTORY(AttributeBasedAccessControlFilterConfigFactory, + Server::Configuration::NamedHttpFilterConfigFactory); + +} // namespace ABACFilter +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/abac/config.h b/source/extensions/filters/http/abac/config.h new file mode 100644 index 0000000000000..c1b59214d5aea --- /dev/null +++ b/source/extensions/filters/http/abac/config.h @@ -0,0 +1,31 @@ +#pragma once + +#include "envoy/config/filter/http/abac/v2alpha/abac.pb.h" +#include "envoy/config/filter/http/abac/v2alpha/abac.pb.validate.h" + +#include "extensions/filters/http/common/factory_base.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace ABACFilter { + +/* + * Config registration for the ABAC filter. @see NamedHttpFilterConfigFactory. + */ +class AttributeBasedAccessControlFilterConfigFactory + : public Common::FactoryBase { +public: + AttributeBasedAccessControlFilterConfigFactory() : FactoryBase(HttpFilterNames::get().Abac) {} + +private: + Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::config::filter::http::abac::v2alpha::ABAC& proto_config, const std::string&, + Server::Configuration::FactoryContext&) override; +}; + +} // namespace ABACFilter +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 3ce6643fa8fc2..e477730ba91e0 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -48,6 +48,8 @@ class HttpFilterNameValues { const std::string ExtAuthorization = "envoy.ext_authz"; // RBAC HTTP Authorization filter const std::string Rbac = "envoy.filters.http.rbac"; + // ABAC HTTP Authorization filter + const std::string Abac = "envoy.filters.http.abac"; // JWT authentication filter const std::string JwtAuthn = "envoy.filters.http.jwt_authn"; // Header to metadata filter From beb197caa4b624332065a7b2b98564977f71d1a3 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 24 Jul 2019 22:17:29 -0700 Subject: [PATCH 02/32] typos Signed-off-by: Kuat Yessenov --- source/extensions/filters/http/abac/abac_filter.h | 3 --- source/extensions/filters/http/abac/config.cc | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/source/extensions/filters/http/abac/abac_filter.h b/source/extensions/filters/http/abac/abac_filter.h index 7abcacae75af9..b7e319a05ecaa 100644 --- a/source/extensions/filters/http/abac/abac_filter.h +++ b/source/extensions/filters/http/abac/abac_filter.h @@ -5,9 +5,6 @@ #include "envoy/config/filter/http/abac/v2alpha/abac.pb.h" #include "envoy/http/filter.h" -//#include "envoy/stats/scope.h" -//#include "envoy/stats/stats_macros.h" - #include "common/common/logger.h" #include "eval/public/cel_expression.h" diff --git a/source/extensions/filters/http/abac/config.cc b/source/extensions/filters/http/abac/config.cc index 3bedda9f25ea8..8cefb842ace7e 100644 --- a/source/extensions/filters/http/abac/config.cc +++ b/source/extensions/filters/http/abac/config.cc @@ -18,7 +18,7 @@ AttributeBasedAccessControlFilterConfigFactory::createFilterFactoryFromProtoType Server::Configuration::FactoryContext&) { google::api::expr::runtime::InterpreterOptions options; - // Conformance with java/go runimes requires this setting + // Conformance with java/go runtimes requires this setting options.partial_string_match = true; auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); From ec74571c73706439f3b74876d39105d5c6a73d45 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 25 Jul 2019 19:43:34 -0700 Subject: [PATCH 03/32] review Signed-off-by: Kuat Yessenov --- source/common/common/logger.h | 2 +- source/extensions/extensions_build_config.bzl | 2 +- source/extensions/filters/http/abac/abac_filter.cc | 6 +++--- source/extensions/filters/http/well_known_names.h | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/common/logger.h b/source/common/common/logger.h index acaa2d9d401ec..33b0d67b74922 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -22,6 +22,7 @@ namespace Logger { // clang-format off // TODO: find out a way for extensions to register new logger IDs #define ALL_LOGGER_IDS(FUNCTION) \ + FUNCTION(abac) \ FUNCTION(admin) \ FUNCTION(aws) \ FUNCTION(assert) \ @@ -49,7 +50,6 @@ namespace Logger { FUNCTION(quic) \ FUNCTION(pool) \ FUNCTION(rbac) \ - FUNCTION(abac) \ FUNCTION(redis) \ FUNCTION(router) \ FUNCTION(runtime) \ diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 17dac24ba9cc5..2d71998a95a37 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -29,6 +29,7 @@ EXTENSIONS = { # HTTP filters # + "envoy.filters.http.abac": "//source/extensions/filters/http/abac:config", "envoy.filters.http.buffer": "//source/extensions/filters/http/buffer:config", "envoy.filters.http.cors": "//source/extensions/filters/http/cors:config", "envoy.filters.http.csrf": "//source/extensions/filters/http/csrf:config", @@ -49,7 +50,6 @@ EXTENSIONS = { "envoy.filters.http.original_src": "//source/extensions/filters/http/original_src:config", "envoy.filters.http.ratelimit": "//source/extensions/filters/http/ratelimit:config", "envoy.filters.http.rbac": "//source/extensions/filters/http/rbac:config", - "envoy.filters.http.abac": "//source/extensions/filters/http/abac:config", "envoy.filters.http.router": "//source/extensions/filters/http/router:config", "envoy.filters.http.squash": "//source/extensions/filters/http/squash:config", "envoy.filters.http.tap": "//source/extensions/filters/http/tap:config", diff --git a/source/extensions/filters/http/abac/abac_filter.cc b/source/extensions/filters/http/abac/abac_filter.cc index baf030067a4e6..49bbcbc8029a3 100644 --- a/source/extensions/filters/http/abac/abac_filter.cc +++ b/source/extensions/filters/http/abac/abac_filter.cc @@ -145,8 +145,8 @@ Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http: auto eval_status = expr_->Evaluate(activation, &arena); if (!eval_status.ok()) { - ENVOY_LOG(debug, "evaluation failed: {}", eval_status.message()); - return Http::FilterHeadersStatus::StopIteration; + ENVOY_LOG(debug, "evaluation failed: {}", eval_status.status().message()); + return Http::FilterHeadersStatus::StopIterationAndWatermark; } auto result = eval_status.ValueOrDie(); @@ -162,7 +162,7 @@ Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http: ENVOY_LOG(debug, "expression did not evaluate to 'true': type {}", CelValue::TypeName(result.type())); } - return Http::FilterHeadersStatus::StopIteration; + return Http::FilterHeadersStatus::StopIterationAndWatermark; } } // namespace ABACFilter diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index e477730ba91e0..b867d6813d4c3 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -12,6 +12,8 @@ namespace HttpFilters { */ class HttpFilterNameValues { public: + // ABAC HTTP Authorization filter + const std::string Abac = "envoy.filters.http.abac"; // Buffer filter const std::string Buffer = "envoy.buffer"; // CORS filter @@ -48,8 +50,6 @@ class HttpFilterNameValues { const std::string ExtAuthorization = "envoy.ext_authz"; // RBAC HTTP Authorization filter const std::string Rbac = "envoy.filters.http.rbac"; - // ABAC HTTP Authorization filter - const std::string Abac = "envoy.filters.http.abac"; // JWT authentication filter const std::string JwtAuthn = "envoy.filters.http.jwt_authn"; // Header to metadata filter From 97efd6dcf5a3e3f1c62cda36e6af790d09d6b90a Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 25 Jul 2019 19:45:55 -0700 Subject: [PATCH 04/32] spelling Signed-off-by: Kuat Yessenov --- tools/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 051ce386f1819..70ef648cdf1b9 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -2,6 +2,7 @@ # lower case and title case (e.g. HTTP will accept http and Http). Entries in all lower case # will accept title case (e.g. lyft matches Lyft). AAAA +ABAC ACK AES AFAICT From f3668b21af423925459598c0073f25e8940a1ce3 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 25 Jul 2019 20:07:37 -0700 Subject: [PATCH 05/32] undo watermark Signed-off-by: Kuat Yessenov --- source/extensions/filters/http/abac/abac_filter.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/abac/abac_filter.cc b/source/extensions/filters/http/abac/abac_filter.cc index 49bbcbc8029a3..ab128253ac98a 100644 --- a/source/extensions/filters/http/abac/abac_filter.cc +++ b/source/extensions/filters/http/abac/abac_filter.cc @@ -14,6 +14,7 @@ using CelList = google::api::expr::runtime::CelList; namespace { constexpr absl::string_view kMetadata = "metadata"; + constexpr absl::string_view kConnection = "connection"; constexpr absl::string_view kRequestedServerName = "requested_server_name"; constexpr absl::string_view kLocalAddress = "local_address"; @@ -23,6 +24,7 @@ constexpr absl::string_view kSubjectLocalCertificate = "subject_local_certificat constexpr absl::string_view kIssuerPeerCertificate = "issuer_peer_certificate"; constexpr absl::string_view kSubjectPeerCertificate = "subject_peer_certificate"; constexpr absl::string_view kTlsVersion = "tls_version"; + constexpr absl::string_view kRequest = "request"; constexpr absl::string_view kPath = "path"; constexpr absl::string_view kHost = "host"; @@ -146,7 +148,7 @@ Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http: auto eval_status = expr_->Evaluate(activation, &arena); if (!eval_status.ok()) { ENVOY_LOG(debug, "evaluation failed: {}", eval_status.status().message()); - return Http::FilterHeadersStatus::StopIterationAndWatermark; + return Http::FilterHeadersStatus::StopIteration; } auto result = eval_status.ValueOrDie(); @@ -162,7 +164,7 @@ Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http: ENVOY_LOG(debug, "expression did not evaluate to 'true': type {}", CelValue::TypeName(result.type())); } - return Http::FilterHeadersStatus::StopIterationAndWatermark; + return Http::FilterHeadersStatus::StopIteration; } } // namespace ABACFilter From 8e80999b6a7d80cecb948316762a22b827cff0e4 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 29 Jul 2019 22:06:42 -0700 Subject: [PATCH 06/32] review feedback Signed-off-by: Kuat Yessenov --- .../config/filter/http/abac/v2alpha/BUILD | 15 -- .../filter/http/abac/v2alpha/abac.proto | 18 -- source/common/common/logger.h | 1 - source/extensions/extensions_build_config.bzl | 1 - source/extensions/filters/common/expr/BUILD | 34 ++++ .../extensions/filters/common/expr/context.cc | 62 +++++++ .../extensions/filters/common/expr/context.h | 64 +++++++ .../filters/common/expr/evaluator.cc | 56 ++++++ .../filters/common/expr/evaluator.h | 31 ++++ source/extensions/filters/http/abac/BUILD | 37 ---- .../filters/http/abac/abac_filter.cc | 173 ------------------ .../filters/http/abac/abac_filter.h | 51 ------ source/extensions/filters/http/abac/config.cc | 53 ------ source/extensions/filters/http/abac/config.h | 31 ---- source/extensions/filters/http/rbac/BUILD | 1 + .../filters/http/rbac/rbac_filter.cc | 22 ++- .../filters/http/rbac/rbac_filter.h | 2 + .../filters/http/well_known_names.h | 2 - 18 files changed, 269 insertions(+), 385 deletions(-) delete mode 100644 api/envoy/config/filter/http/abac/v2alpha/BUILD delete mode 100644 api/envoy/config/filter/http/abac/v2alpha/abac.proto create mode 100644 source/extensions/filters/common/expr/BUILD create mode 100644 source/extensions/filters/common/expr/context.cc create mode 100644 source/extensions/filters/common/expr/context.h create mode 100644 source/extensions/filters/common/expr/evaluator.cc create mode 100644 source/extensions/filters/common/expr/evaluator.h delete mode 100644 source/extensions/filters/http/abac/BUILD delete mode 100644 source/extensions/filters/http/abac/abac_filter.cc delete mode 100644 source/extensions/filters/http/abac/abac_filter.h delete mode 100644 source/extensions/filters/http/abac/config.cc delete mode 100644 source/extensions/filters/http/abac/config.h diff --git a/api/envoy/config/filter/http/abac/v2alpha/BUILD b/api/envoy/config/filter/http/abac/v2alpha/BUILD deleted file mode 100644 index 02b407e3ea54c..0000000000000 --- a/api/envoy/config/filter/http/abac/v2alpha/BUILD +++ /dev/null @@ -1,15 +0,0 @@ -load("@envoy_api//bazel:api_build_system.bzl", "api_proto_library_internal") - -licenses(["notice"]) # Apache 2 - -api_proto_library_internal( - name = "abac", - srcs = ["abac.proto"], - external_cc_proto_deps = [ - "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", - ], - external_proto_deps = [ - "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", - ], - require_py = False, -) diff --git a/api/envoy/config/filter/http/abac/v2alpha/abac.proto b/api/envoy/config/filter/http/abac/v2alpha/abac.proto deleted file mode 100644 index 0e767a496c0e9..0000000000000 --- a/api/envoy/config/filter/http/abac/v2alpha/abac.proto +++ /dev/null @@ -1,18 +0,0 @@ -syntax = "proto3"; - -package envoy.config.filter.http.abac.v2alpha; - -option java_outer_classname = "AbacProto"; -option java_multiple_files = true; -option java_package = "io.envoyproxy.envoy.config.filter.http.abac.v2alpha"; -option go_package = "v2alpha"; - -import "google/api/expr/v1alpha1/syntax.proto"; - -// [#protodoc-title: ABAC] -// Attribute-Based Access Control. - -// ABAC filter config. -message ABAC { - google.api.expr.v1alpha1.Expr expr = 1; -} diff --git a/source/common/common/logger.h b/source/common/common/logger.h index d0101213a2439..42360c572b39a 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -22,7 +22,6 @@ namespace Logger { // clang-format off // TODO: find out a way for extensions to register new logger IDs #define ALL_LOGGER_IDS(FUNCTION) \ - FUNCTION(abac) \ FUNCTION(admin) \ FUNCTION(aws) \ FUNCTION(assert) \ diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 2d71998a95a37..c18d6d6fa47ef 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -29,7 +29,6 @@ EXTENSIONS = { # HTTP filters # - "envoy.filters.http.abac": "//source/extensions/filters/http/abac:config", "envoy.filters.http.buffer": "//source/extensions/filters/http/buffer:config", "envoy.filters.http.cors": "//source/extensions/filters/http/cors:config", "envoy.filters.http.csrf": "//source/extensions/filters/http/csrf:config", diff --git a/source/extensions/filters/common/expr/BUILD b/source/extensions/filters/common/expr/BUILD new file mode 100644 index 0000000000000..eabf7672c0878 --- /dev/null +++ b/source/extensions/filters/common/expr/BUILD @@ -0,0 +1,34 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "evaluator_lib", + srcs = ["evaluator.cc"], + hdrs = ["evaluator.h"], + deps = [ + ":context_lib", + "//source/common/protobuf", + "//source/common/http:utility_lib", + "@com_google_cel_cpp//eval/public:builtin_func_registrar", + "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", + "@com_google_cel_cpp//eval/public:cel_expression", + "@com_google_cel_cpp//eval/public:cel_value", + ], +) + +envoy_cc_library( + name = "context_lib", + srcs = ["context.cc"], + hdrs = ["context.h"], + deps = [ + "//source/common/http:utility_lib", + "@com_google_cel_cpp//eval/public:cel_value", + ], +) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc new file mode 100644 index 0000000000000..1cc473c886301 --- /dev/null +++ b/source/extensions/filters/common/expr/context.cc @@ -0,0 +1,62 @@ +#include "extensions/filters/common/expr/context.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace Expr { + +namespace { + +absl::optional convertHeaderEntry(const Http::HeaderEntry* header) { + if (header == nullptr) { + return {}; + } + return CelValue::CreateString(header->value().getStringView()); +} + +} // namespace + +absl::optional HeadersWrapper::operator[](CelValue key) const { + if (!key.IsString()) { + return {}; + } + auto out = headers_.get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); + return convertHeaderEntry(out); +} + +absl::optional RequestWrapper::operator[](CelValue key) const { + if (!key.IsString()) { + return {}; + } + auto value = key.StringOrDie().value(); + if (value == Path) { + return convertHeaderEntry(wrapper_.headers_.Path()); + } else if (value == Host) { + return convertHeaderEntry(wrapper_.headers_.Host()); + } else if (value == Scheme) { + return convertHeaderEntry(wrapper_.headers_.Scheme()); + } else if (value == Method) { + return convertHeaderEntry(wrapper_.headers_.Method()); + } else if (value == Referer) { + return convertHeaderEntry(wrapper_.headers_.Referer()); + } else if (value == Headers) { + return CelValue::CreateMap(&wrapper_); + // time of the request? + } else if (value == Time) { + return CelValue::CreateTimestamp(absl::FromChrono(info_.startTime())); + } else if (value == ID) { + return convertHeaderEntry(wrapper_.headers_.RequestId()); + } else if (value == UserAgent) { + return convertHeaderEntry(wrapper_.headers_.UserAgent()); + } + // size = content length + // useragent + return {}; +} + +} // namespace Expr +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h new file mode 100644 index 0000000000000..28f297f6bb50e --- /dev/null +++ b/source/extensions/filters/common/expr/context.h @@ -0,0 +1,64 @@ +#pragma once + +#include "common/http/headers.h" +#include "envoy/stream_info/stream_info.h" + +#include "eval/public/cel_value.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace Expr { + +using CelValue = google::api::expr::runtime::CelValue; + +// Symbols for traversing the attribute context in the expressions. +constexpr absl::string_view Request = "request"; +constexpr absl::string_view Path = "path"; +constexpr absl::string_view Host = "host"; +constexpr absl::string_view Scheme = "scheme"; +constexpr absl::string_view Method = "method"; +constexpr absl::string_view Referer = "referer"; +constexpr absl::string_view Headers = "headers"; +constexpr absl::string_view Time = "time"; +constexpr absl::string_view ID = "id"; +constexpr absl::string_view UserAgent = "useragent"; +constexpr absl::string_view Metadata = "metadata"; + +class RequestWrapper; + +class HeadersWrapper : public google::api::expr::runtime::CelMap { +public: + HeadersWrapper(const Http::HeaderMap& headers) : headers_(headers) {} + absl::optional operator[](CelValue key) const override; + int size() const override { return headers_.size(); } + bool empty() const override { return headers_.empty(); } + const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } +private: + friend class RequestWrapper; + const Http::HeaderMap& headers_; +}; + +class BaseWrapper : public google::api::expr::runtime::CelMap { +public: + int size() const override { return 0; } + bool empty() const override { return false; } + const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } +}; + +class RequestWrapper : public BaseWrapper { +public: + RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) : + wrapper_(headers), info_(info) {} + absl::optional operator[](CelValue key) const override; +private: + const HeadersWrapper wrapper_; + const StreamInfo::StreamInfo& info_; +}; + +} // namespace Expr +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc new file mode 100644 index 0000000000000..1acfda0459941 --- /dev/null +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -0,0 +1,56 @@ +#include "extensions/filters/common/expr/evaluator.h" + +#include "envoy/common/exception.h" +#include "common/protobuf/protobuf.h" + +#include "eval/public/builtin_func_registrar.h" +#include "eval/public/cel_expr_builder_factory.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace Expr { + +ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr) { + google::api::expr::runtime::InterpreterOptions options; + + // Conformance with spec/go runtimes requires this setting + options.partial_string_match = true; + + auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); + auto register_status = + google::api::expr::runtime::RegisterBuiltinFunctions(builder->GetRegistry()); + if (!register_status.ok()) { + throw EnvoyException( + absl::StrCat("failed to register built-in functions: ", register_status.message())); + } + google::api::expr::v1alpha1::SourceInfo source_info; + auto cel_expression_status = builder->CreateExpression(&expr, &source_info); + if (!cel_expression_status.ok()) { + throw EnvoyException( + absl::StrCat("failed to create an expression: ", cel_expression_status.status().message())); + } + return std::move(cel_expression_status.ValueOrDie()); +} + +absl::optional evaluate(const google::api::expr::runtime::CelExpression& expr, + const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers) { + Protobuf::Arena arena; + google::api::expr::runtime::Activation activation; + const RequestWrapper request(headers, info); + activation.InsertValue(Request, CelValue::CreateMap(&request)); + + auto eval_status = expr.Evaluate(activation, &arena); + if (!eval_status.ok()) { + return {}; + } + return eval_status.ValueOrDie(); +} + +} // namespace Expr +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h new file mode 100644 index 0000000000000..13347b415f81c --- /dev/null +++ b/source/extensions/filters/common/expr/evaluator.h @@ -0,0 +1,31 @@ +#pragma once + +#include "extensions/filters/common/expr/context.h" + +#include "eval/public/cel_expression.h" +#include "eval/public/cel_value.h" +#include "common/http/headers.h" +#include "envoy/stream_info/stream_info.h" + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace Expr { + +using ExpressionPtr = std::unique_ptr; + +// Creates an interpretable expression from a protobuf representation. +// Throws an exception if fails to construct a runtime expression. +ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr); + +// Evaluates an expression for a request. +absl::optional evaluate(const google::api::expr::runtime::CelExpression& expr, + const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers); + +} // namespace Expr +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/abac/BUILD b/source/extensions/filters/http/abac/BUILD deleted file mode 100644 index 108f62b86a973..0000000000000 --- a/source/extensions/filters/http/abac/BUILD +++ /dev/null @@ -1,37 +0,0 @@ -licenses(["notice"]) # Apache 2 - -load( - "//bazel:envoy_build_system.bzl", - "envoy_cc_library", - "envoy_package", -) - -envoy_package() - -envoy_cc_library( - name = "config", - srcs = ["config.cc"], - hdrs = ["config.h"], - deps = [ - ":abac_filter_lib", - "//include/envoy/registry", - "//source/extensions/filters/http:well_known_names", - "//source/extensions/filters/http/common:factory_base_lib", - "@com_google_cel_cpp//eval/public:builtin_func_registrar", - "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", - ], -) - -envoy_cc_library( - name = "abac_filter_lib", - srcs = ["abac_filter.cc"], - hdrs = ["abac_filter.h"], - deps = [ - "//include/envoy/http:filter_interface", - #"//include/envoy/stats:stats_macros", - "//source/common/http:utility_lib", - "//source/common/protobuf", - "@com_google_cel_cpp//eval/public:cel_expression", - "@envoy_api//envoy/config/filter/http/abac/v2alpha:abac_cc", - ], -) diff --git a/source/extensions/filters/http/abac/abac_filter.cc b/source/extensions/filters/http/abac/abac_filter.cc deleted file mode 100644 index ab128253ac98a..0000000000000 --- a/source/extensions/filters/http/abac/abac_filter.cc +++ /dev/null @@ -1,173 +0,0 @@ -#include "extensions/filters/http/abac/abac_filter.h" - -#include "common/protobuf/protobuf.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace ABACFilter { - -using CelValue = google::api::expr::runtime::CelValue; -using CelMap = google::api::expr::runtime::CelMap; -using CelList = google::api::expr::runtime::CelList; - -namespace { - -constexpr absl::string_view kMetadata = "metadata"; - -constexpr absl::string_view kConnection = "connection"; -constexpr absl::string_view kRequestedServerName = "requested_server_name"; -constexpr absl::string_view kLocalAddress = "local_address"; -constexpr absl::string_view kRemoteAddress = "remote_address"; -constexpr absl::string_view kCertificatePresented = "certificate_presented"; -constexpr absl::string_view kSubjectLocalCertificate = "subject_local_certificate"; -constexpr absl::string_view kIssuerPeerCertificate = "issuer_peer_certificate"; -constexpr absl::string_view kSubjectPeerCertificate = "subject_peer_certificate"; -constexpr absl::string_view kTlsVersion = "tls_version"; - -constexpr absl::string_view kRequest = "request"; -constexpr absl::string_view kPath = "path"; -constexpr absl::string_view kHost = "host"; -constexpr absl::string_view kScheme = "scheme"; -constexpr absl::string_view kMethod = "method"; -constexpr absl::string_view kReferer = "referer"; -constexpr absl::string_view kHeaders = "headers"; -constexpr absl::string_view kTime = "time"; - -class ConnectionWrapper : public CelMap { -public: - ConnectionWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} - - absl::optional operator[](CelValue key) const override { - if (!key.IsString()) { - return {}; - } - auto value = key.StringOrDie().value(); - if (value == kRequestedServerName) { - return CelValue::CreateString(info_.requestedServerName()); - } else if (value == kLocalAddress) { - return CelValue::CreateString(info_.downstreamLocalAddress()->asString()); - } else if (value == kRemoteAddress) { - return CelValue::CreateString(info_.downstreamRemoteAddress()->asString()); - } - - auto ssl_info = info_.downstreamSslConnection(); - if (ssl_info != nullptr) { - if (value == kCertificatePresented) { - return CelValue::CreateBool(ssl_info->peerCertificatePresented()); - } else if (value == kSubjectLocalCertificate) { - return CelValue::CreateString(ssl_info->subjectLocalCertificate()); - } else if (value == kIssuerPeerCertificate) { - return CelValue::CreateString(ssl_info->issuerPeerCertificate()); - } else if (value == kSubjectPeerCertificate) { - return CelValue::CreateString(ssl_info->subjectPeerCertificate()); - } else if (value == kTlsVersion) { - return CelValue::CreateString(ssl_info->tlsVersion()); - } - } - return {}; - } - int size() const override { return 0; } - bool empty() const override { return false; } - const CelList* ListKeys() const override { return nullptr; } - -private: - const StreamInfo::StreamInfo& info_; -}; - -class HeadersWrapper : public CelMap { -public: - HeadersWrapper(const Http::HeaderMap& headers) : headers_(headers) {} - - absl::optional operator[](CelValue key) const override { - if (!key.IsString()) { - return {}; - } - auto out = headers_.get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); - if (out == nullptr) { - return {}; - } - return CelValue::CreateString(out->value().getStringView()); - } - int size() const override { return headers_.size(); } - bool empty() const override { return headers_.empty(); } - const CelList* ListKeys() const override { return nullptr; } - const Http::HeaderMap& headers_; -}; - -class RequestWrapper : public CelMap { -public: - RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) - : wrapper_(headers), info_(info) {} - - absl::optional operator[](CelValue key) const override { - if (!key.IsString()) { - return {}; - } - auto value = key.StringOrDie().value(); - if (value == kPath) { - return CelValue::CreateString(wrapper_.headers_.Path()->value().getStringView()); - } else if (value == kHost) { - return CelValue::CreateString(wrapper_.headers_.Host()->value().getStringView()); - } else if (value == kScheme) { - return CelValue::CreateString(wrapper_.headers_.Scheme()->value().getStringView()); - } else if (value == kMethod) { - return CelValue::CreateString(wrapper_.headers_.Method()->value().getStringView()); - } else if (value == kReferer) { - return CelValue::CreateString(wrapper_.headers_.Referer()->value().getStringView()); - } else if (value == kHeaders) { - return CelValue::CreateMap(&wrapper_); - } else if (value == kTime) { - return CelValue::CreateTimestamp(absl::FromChrono(info_.startTime())); - } - return {}; - } - - int size() const override { return 0; } - bool empty() const override { return false; } - const CelList* ListKeys() const override { return nullptr; } - -private: - const HeadersWrapper wrapper_; - const StreamInfo::StreamInfo& info_; -}; - -} // namespace - -Http::FilterHeadersStatus AttributeBasedAccessControlFilter::decodeHeaders(Http::HeaderMap& headers, - bool) { - Protobuf::Arena arena; - google::api::expr::runtime::Activation activation; - activation.InsertValue( - kMetadata, CelValue::CreateMessage(&callbacks_->streamInfo().dynamicMetadata(), &arena)); - const ConnectionWrapper connection(callbacks_->streamInfo()); - activation.InsertValue(kConnection, CelValue::CreateMap(&connection)); - const RequestWrapper request(headers, callbacks_->streamInfo()); - activation.InsertValue(kRequest, CelValue::CreateMap(&request)); - - auto eval_status = expr_->Evaluate(activation, &arena); - if (!eval_status.ok()) { - ENVOY_LOG(debug, "evaluation failed: {}", eval_status.status().message()); - return Http::FilterHeadersStatus::StopIteration; - } - - auto result = eval_status.ValueOrDie(); - if (result.IsBool() && result.BoolOrDie()) { - return Http::FilterHeadersStatus::Continue; - } - - if (result.IsError()) { - ENVOY_LOG(debug, "evaluation error: {}", result.ErrorOrDie()->message()); - } else if (result.IsString()) { - ENVOY_LOG(debug, "evaluation resulted in a string: {}", result.StringOrDie().value()); - } else { - ENVOY_LOG(debug, "expression did not evaluate to 'true': type {}", - CelValue::TypeName(result.type())); - } - return Http::FilterHeadersStatus::StopIteration; -} - -} // namespace ABACFilter -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/abac/abac_filter.h b/source/extensions/filters/http/abac/abac_filter.h deleted file mode 100644 index b7e319a05ecaa..0000000000000 --- a/source/extensions/filters/http/abac/abac_filter.h +++ /dev/null @@ -1,51 +0,0 @@ -#pragma once - -#include - -#include "envoy/config/filter/http/abac/v2alpha/abac.pb.h" -#include "envoy/http/filter.h" - -#include "common/common/logger.h" - -#include "eval/public/cel_expression.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace ABACFilter { - -/** - * A filter that provides attribute-based access control authorization for HTTP requests. - */ -class AttributeBasedAccessControlFilter : public Http::StreamDecoderFilter, - public Logger::Loggable { -public: - AttributeBasedAccessControlFilter(std::shared_ptr expr) - : expr_(expr) {} - - Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool) override; - - Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { - return Http::FilterDataStatus::Continue; - } - - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap&) override { - return Http::FilterTrailersStatus::Continue; - } - - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { - callbacks_ = &callbacks; - } - - // Http::StreamFilterBase - void onDestroy() override {} - -private: - Http::StreamDecoderFilterCallbacks* callbacks_{}; - std::shared_ptr expr_; -}; - -} // namespace ABACFilter -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/abac/config.cc b/source/extensions/filters/http/abac/config.cc deleted file mode 100644 index 8cefb842ace7e..0000000000000 --- a/source/extensions/filters/http/abac/config.cc +++ /dev/null @@ -1,53 +0,0 @@ -#include "extensions/filters/http/abac/config.h" - -#include "envoy/registry/registry.h" - -#include "extensions/filters/http/abac/abac_filter.h" - -#include "eval/public/builtin_func_registrar.h" -#include "eval/public/cel_expr_builder_factory.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace ABACFilter { - -Http::FilterFactoryCb -AttributeBasedAccessControlFilterConfigFactory::createFilterFactoryFromProtoTyped( - const envoy::config::filter::http::abac::v2alpha::ABAC& config, const std::string&, - Server::Configuration::FactoryContext&) { - google::api::expr::runtime::InterpreterOptions options; - - // Conformance with java/go runtimes requires this setting - options.partial_string_match = true; - - auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); - auto register_status = - google::api::expr::runtime::RegisterBuiltinFunctions(builder->GetRegistry()); - if (!register_status.ok()) { - throw EnvoyException( - absl::StrCat("failed to register built-in functions: ", register_status.message())); - } - google::api::expr::v1alpha1::SourceInfo source_info; - auto cel_expression_status = builder->CreateExpression(&config.expr(), &source_info); - if (!cel_expression_status.ok()) { - throw EnvoyException( - absl::StrCat("failed to create an expression: ", cel_expression_status.status().message())); - } - auto cel_expression = absl::ShareUniquePtr(std::move(cel_expression_status.ValueOrDie())); - return [cel_expression](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter( - std::make_shared(cel_expression)); - }; -} - -/** - * Static registration for the ABAC filter. @see RegisterFactory - */ -REGISTER_FACTORY(AttributeBasedAccessControlFilterConfigFactory, - Server::Configuration::NamedHttpFilterConfigFactory); - -} // namespace ABACFilter -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/abac/config.h b/source/extensions/filters/http/abac/config.h deleted file mode 100644 index c1b59214d5aea..0000000000000 --- a/source/extensions/filters/http/abac/config.h +++ /dev/null @@ -1,31 +0,0 @@ -#pragma once - -#include "envoy/config/filter/http/abac/v2alpha/abac.pb.h" -#include "envoy/config/filter/http/abac/v2alpha/abac.pb.validate.h" - -#include "extensions/filters/http/common/factory_base.h" -#include "extensions/filters/http/well_known_names.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace ABACFilter { - -/* - * Config registration for the ABAC filter. @see NamedHttpFilterConfigFactory. - */ -class AttributeBasedAccessControlFilterConfigFactory - : public Common::FactoryBase { -public: - AttributeBasedAccessControlFilterConfigFactory() : FactoryBase(HttpFilterNames::get().Abac) {} - -private: - Http::FilterFactoryCb createFilterFactoryFromProtoTyped( - const envoy::config::filter::http::abac::v2alpha::ABAC& proto_config, const std::string&, - Server::Configuration::FactoryContext&) override; -}; - -} // namespace ABACFilter -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/rbac/BUILD b/source/extensions/filters/http/rbac/BUILD index f7adc41aa8507..d1f54a2b8c0d3 100644 --- a/source/extensions/filters/http/rbac/BUILD +++ b/source/extensions/filters/http/rbac/BUILD @@ -31,6 +31,7 @@ envoy_cc_library( "//source/extensions/filters/common/rbac:engine_lib", "//source/extensions/filters/common/rbac:utility_lib", "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/common/expr:evaluator_lib", "@envoy_api//envoy/config/filter/http/rbac/v2:rbac_cc", ], ) diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 965888fd3105f..50c4f0f19c541 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -24,7 +24,8 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const std::string& stats_prefix, Stats::Scope& scope) : stats_(Filters::Common::RBAC::generateStats(stats_prefix, scope)), engine_(Filters::Common::RBAC::createEngine(proto_config)), - shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)) {} + shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)), + expr_(proto_config.has_condition() ? Filters::Common::Expr::create(proto_config.condition()) : nullptr) {} const absl::optional& RoleBasedAccessControlFilterConfig::engine(const Router::RouteConstSharedPtr route, @@ -109,7 +110,6 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head callbacks_->streamInfo().dynamicMetadata(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); - return Http::FilterHeadersStatus::Continue; } else { ENVOY_LOG(debug, "enforced denied"); callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, @@ -117,9 +117,25 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head config_->stats().denied_.inc(); return Http::FilterHeadersStatus::StopIteration; } + } else { + ENVOY_LOG(debug, "no engine, allowed by default"); + } + + if (expr_ != nullptr) { + auto eval_status = Filter::Common::Expr::Evaluate(expr_, headers, callbacks_->streamInfo()); + if (!eval_status.has_value()) { + ENVOY_LOG(debug, "evaluation failed"); + return Http::FilterHeadersStatus::StopIteration; + } + auto result = eval_status.value(); + ENVOY_LOG(trace, "value: {}", result.IsError() ? result.ErrorOrDie()->message() : + (result.IsString() ? result.StringOrDie().value() : "")); + if (! result.IsBool() || (result.IsBool() && !result.BoolOrDie())) { + ENVOY_LOG(debug, "evaluated to non-bool or false"); + return Http::FilterHeadersStatus::StopIteration; + } } - ENVOY_LOG(debug, "no engine, allowed by default"); return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index 5397a3c31c586..4345eaeab17e1 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -11,6 +11,7 @@ #include "extensions/filters/common/rbac/engine_impl.h" #include "extensions/filters/common/rbac/utility.h" +#include "extensions/filters/common/expr/evaluator.h" namespace Envoy { namespace Extensions { @@ -57,6 +58,7 @@ class RoleBasedAccessControlFilterConfig { const absl::optional engine_; const absl::optional shadow_engine_; + const std::unique_ptr expr_; }; using RoleBasedAccessControlFilterConfigSharedPtr = diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index b867d6813d4c3..3ce6643fa8fc2 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -12,8 +12,6 @@ namespace HttpFilters { */ class HttpFilterNameValues { public: - // ABAC HTTP Authorization filter - const std::string Abac = "envoy.filters.http.abac"; // Buffer filter const std::string Buffer = "envoy.buffer"; // CORS filter From 970b3615842f2ab8c7a93bddad7289edf061b4d1 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 29 Jul 2019 22:06:57 -0700 Subject: [PATCH 07/32] review feedback Signed-off-by: Kuat Yessenov --- CODEOWNERS | 2 -- api/envoy/config/filter/http/rbac/v2/BUILD | 7 ++++ .../config/filter/http/rbac/v2/rbac.proto | 4 +++ source/extensions/filters/common/expr/BUILD | 2 +- .../extensions/filters/common/expr/context.cc | 32 +++++++++++++++++-- .../extensions/filters/common/expr/context.h | 29 ++++++++++++++--- .../filters/common/expr/evaluator.cc | 10 ++++-- .../filters/common/expr/evaluator.h | 14 ++++---- source/extensions/filters/http/rbac/BUILD | 2 +- .../filters/http/rbac/rbac_filter.cc | 20 ++++++++---- .../filters/http/rbac/rbac_filter.h | 6 ++-- tools/spelling_dictionary.txt | 1 - 12 files changed, 99 insertions(+), 30 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index 2529287a3208b..a252439569d6c 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -20,8 +20,6 @@ extensions/filters/common/original_src @snowp @klarose /*/extensions/filters/network/thrift_proxy @zuercher @brian-pane # jwt_authn http filter extension /*/extensions/filters/http/jwt_authn @qiwzhang @lizan -# abac http filter extension -/*/extensions/filters/http/abac @kyessenov @PiotrSikora # grpc_http1_reverse_bridge http filter extension /*/extensions/filters/http/grpc_http1_reverse_bridge @snowp @zuercher # header_to_metadata extension diff --git a/api/envoy/config/filter/http/rbac/v2/BUILD b/api/envoy/config/filter/http/rbac/v2/BUILD index 6182fe26748a0..d828071efa68f 100644 --- a/api/envoy/config/filter/http/rbac/v2/BUILD +++ b/api/envoy/config/filter/http/rbac/v2/BUILD @@ -5,5 +5,12 @@ licenses(["notice"]) # Apache 2 api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], + external_cc_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", + ], + external_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", + ], + require_py = False, deps = ["//envoy/config/rbac/v2:rbac"], ) diff --git a/api/envoy/config/filter/http/rbac/v2/rbac.proto b/api/envoy/config/filter/http/rbac/v2/rbac.proto index 0a75d9590fa5a..7281740f87236 100644 --- a/api/envoy/config/filter/http/rbac/v2/rbac.proto +++ b/api/envoy/config/filter/http/rbac/v2/rbac.proto @@ -8,6 +8,7 @@ option java_package = "io.envoyproxy.envoy.config.filter.http.rbac.v2"; option go_package = "v2"; import "envoy/config/rbac/v2/rbac.proto"; +import "google/api/expr/v1alpha1/syntax.proto"; import "validate/validate.proto"; import "gogoproto/gogo.proto"; @@ -25,6 +26,9 @@ message RBAC { // but will emit stats and logs and can be used for rule testing. // If absent, no shadow RBAC policy will be applied. config.rbac.v2.RBAC shadow_rules = 2; + + // A symbolic expression over request attributes specifying an access control condition. + google.api.expr.v1alpha1.Expr condition = 3; } message RBACPerRoute { diff --git a/source/extensions/filters/common/expr/BUILD b/source/extensions/filters/common/expr/BUILD index eabf7672c0878..7b2a6f140792b 100644 --- a/source/extensions/filters/common/expr/BUILD +++ b/source/extensions/filters/common/expr/BUILD @@ -14,8 +14,8 @@ envoy_cc_library( hdrs = ["evaluator.h"], deps = [ ":context_lib", - "//source/common/protobuf", "//source/common/http:utility_lib", + "//source/common/protobuf", "@com_google_cel_cpp//eval/public:builtin_func_registrar", "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", "@com_google_cel_cpp//eval/public:cel_expression", diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 1cc473c886301..5267dbac10a7f 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -1,5 +1,8 @@ #include "extensions/filters/common/expr/context.h" +#include "absl/strings/numbers.h" +#include "absl/time/time.h" + namespace Envoy { namespace Extensions { namespace Filters { @@ -42,16 +45,39 @@ absl::optional RequestWrapper::operator[](CelValue key) const { return convertHeaderEntry(wrapper_.headers_.Referer()); } else if (value == Headers) { return CelValue::CreateMap(&wrapper_); - // time of the request? } else if (value == Time) { return CelValue::CreateTimestamp(absl::FromChrono(info_.startTime())); } else if (value == ID) { return convertHeaderEntry(wrapper_.headers_.RequestId()); } else if (value == UserAgent) { return convertHeaderEntry(wrapper_.headers_.UserAgent()); + } else if (value == Size) { + // it is important to make a choice whether to rely on content-length vs stream info + // (which is not available at the time of the request headers) + auto length_header = wrapper_.headers_.ContentLength(); + if (length_header != nullptr) { + int64_t length; + if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { + return CelValue::CreateInt64(length); + } + } + } else if (value == TotalSize) { + return CelValue::CreateInt64(info_.bytesReceived() + wrapper_.headers_.byteSize()); + } + return {}; +} + +absl::optional ConnectionWrapper::operator[](CelValue key) const { + if (!key.IsString()) { + return {}; } - // size = content length - // useragent + auto value = key.StringOrDie().value(); + if (value == LocalAddress) { + return CelValue::CreateString(info_.downstreamLocalAddress()->asStringView()); + } else if (value == RemoteAddress) { + return CelValue::CreateString(info_.downstreamRemoteAddress()->asStringView()); + } + return {}; } diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 28f297f6bb50e..aeeabc64477ac 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -1,8 +1,9 @@ #pragma once -#include "common/http/headers.h" #include "envoy/stream_info/stream_info.h" +#include "common/http/headers.h" + #include "eval/public/cel_value.h" namespace Envoy { @@ -13,7 +14,7 @@ namespace Expr { using CelValue = google::api::expr::runtime::CelValue; -// Symbols for traversing the attribute context in the expressions. +// Symbols for traversing the request properties in the expressions constexpr absl::string_view Request = "request"; constexpr absl::string_view Path = "path"; constexpr absl::string_view Host = "host"; @@ -24,8 +25,17 @@ constexpr absl::string_view Headers = "headers"; constexpr absl::string_view Time = "time"; constexpr absl::string_view ID = "id"; constexpr absl::string_view UserAgent = "useragent"; +constexpr absl::string_view Size = "size"; +constexpr absl::string_view TotalSize = "total_size"; + +// Per-request metadata constexpr absl::string_view Metadata = "metadata"; +// Downstream connection properties +constexpr absl::string_view Connection = "connection"; +constexpr absl::string_view LocalAddress = "local_address"; +constexpr absl::string_view RemoteAddress = "remote_address"; + class RequestWrapper; class HeadersWrapper : public google::api::expr::runtime::CelMap { @@ -35,6 +45,7 @@ class HeadersWrapper : public google::api::expr::runtime::CelMap { int size() const override { return headers_.size(); } bool empty() const override { return headers_.empty(); } const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } + private: friend class RequestWrapper; const Http::HeaderMap& headers_; @@ -49,14 +60,24 @@ class BaseWrapper : public google::api::expr::runtime::CelMap { class RequestWrapper : public BaseWrapper { public: - RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) : - wrapper_(headers), info_(info) {} + RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) + : wrapper_(headers), info_(info) {} absl::optional operator[](CelValue key) const override; + private: const HeadersWrapper wrapper_; const StreamInfo::StreamInfo& info_; }; +class ConnectionWrapper : public BaseWrapper { +public: + ConnectionWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} + absl::optional operator[](CelValue key) const override; + +private: + const StreamInfo::StreamInfo& info_; +} + } // namespace Expr } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index 1acfda0459941..dc22872fc60d6 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -1,6 +1,7 @@ #include "extensions/filters/common/expr/evaluator.h" #include "envoy/common/exception.h" + #include "common/protobuf/protobuf.h" #include "eval/public/builtin_func_registrar.h" @@ -34,18 +35,21 @@ ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr) { return std::move(cel_expression_status.ValueOrDie()); } -absl::optional evaluate(const google::api::expr::runtime::CelExpression& expr, - const StreamInfo::StreamInfo& info, - const Http::HeaderMap& headers) { +absl::optional evaluate(const Expression& expr, const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers) { Protobuf::Arena arena; google::api::expr::runtime::Activation activation; const RequestWrapper request(headers, info); + const ConnectionWrapper connection(info); activation.InsertValue(Request, CelValue::CreateMap(&request)); + activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), &arena)); + activation.InsertValue(Connection, CelValue::CreateMap(&connection)); auto eval_status = expr.Evaluate(activation, &arena); if (!eval_status.ok()) { return {}; } + return eval_status.ValueOrDie(); } diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 13347b415f81c..3034f207cff81 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -1,11 +1,13 @@ #pragma once +#include "envoy/stream_info/stream_info.h" + +#include "common/http/headers.h" + #include "extensions/filters/common/expr/context.h" #include "eval/public/cel_expression.h" #include "eval/public/cel_value.h" -#include "common/http/headers.h" -#include "envoy/stream_info/stream_info.h" namespace Envoy { namespace Extensions { @@ -13,16 +15,16 @@ namespace Filters { namespace Common { namespace Expr { -using ExpressionPtr = std::unique_ptr; +using Expression = google::api::expr::runtime::CelExpression; +using ExpressionPtr = std::unique_ptr; // Creates an interpretable expression from a protobuf representation. // Throws an exception if fails to construct a runtime expression. ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr); // Evaluates an expression for a request. -absl::optional evaluate(const google::api::expr::runtime::CelExpression& expr, - const StreamInfo::StreamInfo& info, - const Http::HeaderMap& headers); +absl::optional evaluate(const Expression& expr, const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers); } // namespace Expr } // namespace Common diff --git a/source/extensions/filters/http/rbac/BUILD b/source/extensions/filters/http/rbac/BUILD index d1f54a2b8c0d3..37970040848d0 100644 --- a/source/extensions/filters/http/rbac/BUILD +++ b/source/extensions/filters/http/rbac/BUILD @@ -28,10 +28,10 @@ envoy_cc_library( "//include/envoy/http:filter_interface", "//include/envoy/stats:stats_macros", "//source/common/http:utility_lib", + "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/common/rbac:engine_lib", "//source/extensions/filters/common/rbac:utility_lib", "//source/extensions/filters/http:well_known_names", - "//source/extensions/filters/common/expr:evaluator_lib", "@envoy_api//envoy/config/filter/http/rbac/v2:rbac_cc", ], ) diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 50c4f0f19c541..6631da36d1048 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -25,7 +25,8 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( : stats_(Filters::Common::RBAC::generateStats(stats_prefix, scope)), engine_(Filters::Common::RBAC::createEngine(proto_config)), shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)), - expr_(proto_config.has_condition() ? Filters::Common::Expr::create(proto_config.condition()) : nullptr) {} + expr_(proto_config.has_condition() ? Filters::Common::Expr::create(proto_config.condition()) + : nullptr) {} const absl::optional& RoleBasedAccessControlFilterConfig::engine(const Router::RouteConstSharedPtr route, @@ -121,17 +122,22 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head ENVOY_LOG(debug, "no engine, allowed by default"); } - if (expr_ != nullptr) { - auto eval_status = Filter::Common::Expr::Evaluate(expr_, headers, callbacks_->streamInfo()); + if (config_->expr() != nullptr) { + auto eval_status = + Filters::Common::Expr::evaluate(*config_->expr(), callbacks_->streamInfo(), headers); if (!eval_status.has_value()) { ENVOY_LOG(debug, "evaluation failed"); return Http::FilterHeadersStatus::StopIteration; } auto result = eval_status.value(); - ENVOY_LOG(trace, "value: {}", result.IsError() ? result.ErrorOrDie()->message() : - (result.IsString() ? result.StringOrDie().value() : "")); - if (! result.IsBool() || (result.IsBool() && !result.BoolOrDie())) { - ENVOY_LOG(debug, "evaluated to non-bool or false"); + ENVOY_LOG(trace, "value: {}", + result.IsError() + ? result.ErrorOrDie()->message() + : (result.IsString() + ? result.StringOrDie().value() + : (result.IsInt64() ? absl::StrCat(result.Int64OrDie()) : ""))); + if (!result.IsBool() || (result.IsBool() && !result.BoolOrDie())) { + ENVOY_LOG(debug, "enforced: denied by condition"); return Http::FilterHeadersStatus::StopIteration; } } diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index 4345eaeab17e1..66f73d8b2365a 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -9,9 +9,9 @@ #include "common/common/logger.h" +#include "extensions/filters/common/expr/evaluator.h" #include "extensions/filters/common/rbac/engine_impl.h" #include "extensions/filters/common/rbac/utility.h" -#include "extensions/filters/common/expr/evaluator.h" namespace Envoy { namespace Extensions { @@ -48,6 +48,8 @@ class RoleBasedAccessControlFilterConfig { engine(const Router::RouteConstSharedPtr route, Filters::Common::RBAC::EnforcementMode mode) const; + Filters::Common::Expr::Expression* expr() { return expr_.get(); } + private: const absl::optional& engine(Filters::Common::RBAC::EnforcementMode mode) const { @@ -58,7 +60,7 @@ class RoleBasedAccessControlFilterConfig { const absl::optional engine_; const absl::optional shadow_engine_; - const std::unique_ptr expr_; + const Filters::Common::Expr::ExpressionPtr expr_; }; using RoleBasedAccessControlFilterConfigSharedPtr = diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index ce801c292e2cd..50abfff89e48c 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -2,7 +2,6 @@ # lower case and title case (e.g. HTTP will accept http and Http). Entries in all lower case # will accept title case (e.g. lyft matches Lyft). AAAA -ABAC ACK AES AFAICT From ebf4c4a8e4f68d16c04b9ee84f39778d84012db1 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 29 Jul 2019 22:14:38 -0700 Subject: [PATCH 08/32] review feedback Signed-off-by: Kuat Yessenov --- source/extensions/filters/common/expr/context.h | 2 +- source/extensions/filters/http/rbac/rbac_filter.cc | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index aeeabc64477ac..78102bbca4160 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -76,7 +76,7 @@ class ConnectionWrapper : public BaseWrapper { private: const StreamInfo::StreamInfo& info_; -} +}; } // namespace Expr } // namespace Common diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 6631da36d1048..f696c48b14b0d 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -127,6 +127,9 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head Filters::Common::Expr::evaluate(*config_->expr(), callbacks_->streamInfo(), headers); if (!eval_status.has_value()) { ENVOY_LOG(debug, "evaluation failed"); + callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, + absl::nullopt, RcDetails::get().RbacAccessDenied); + config_->stats().denied_.inc(); return Http::FilterHeadersStatus::StopIteration; } auto result = eval_status.value(); @@ -138,6 +141,9 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head : (result.IsInt64() ? absl::StrCat(result.Int64OrDie()) : ""))); if (!result.IsBool() || (result.IsBool() && !result.BoolOrDie())) { ENVOY_LOG(debug, "enforced: denied by condition"); + callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, + absl::nullopt, RcDetails::get().RbacAccessDenied); + config_->stats().denied_.inc(); return Http::FilterHeadersStatus::StopIteration; } } From d1fd462dfa30ad46318202f0428466ef074442af Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 29 Jul 2019 22:19:11 -0700 Subject: [PATCH 09/32] add code owners Signed-off-by: Kuat Yessenov --- CODEOWNERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CODEOWNERS b/CODEOWNERS index a252439569d6c..f06f96f89afb1 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -46,3 +46,5 @@ extensions/filters/common/original_src @snowp @klarose /*/extensions/retry/host/omit_canary_hosts @sriduth @snowp # http inspector /*/extensions/filters/listener/http_inspector @crazyxy @PiotrSikora @lizan +# attribute context +/*/extensions/filters/common/expr @kyessenov @yangminzhu From 41220dad5f92862d0bd2ce911adfdb3221d1007f Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 30 Jul 2019 13:30:26 -0700 Subject: [PATCH 10/32] update cel-cpp Signed-off-by: Kuat Yessenov --- bazel/repositories.bzl | 2 -- bazel/repository_locations.bzl | 19 +++---------------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index ce402cd785503..0b55cfb12dcf3 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -153,8 +153,6 @@ def envoy_dependencies(skip_targets = []): _com_lightstep_tracer_cpp() _io_opentracing_cpp() _net_zlib() - _repository_impl("com_github_gflags_gflags") - _repository_impl("com_google_glog") _repository_impl("com_google_re2") _repository_impl("com_google_cel_cpp") _repository_impl("bazel_toolchains") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index f498dee1cd12c..977f80a27e1c3 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -247,23 +247,10 @@ REPOSITORY_LOCATIONS = dict( sha256 = "fcdebf54c89d839ffa7eefae166c8e4b551c765559db13ff15bff98047f344fb", urls = ["https://storage.googleapis.com/quiche-envoy-integration/2a930469533c3b541443488a629fe25cd8ff53d0.tar.gz"], ), - com_github_gflags_gflags = dict( - sha256 = "6e16c8bc91b1310a44f3965e616383dbda48f83e8c1eaa2370a215057b00cabe", - strip_prefix = "gflags-77592648e3f3be87d6c7123eb81cbad75f9aef5a", - urls = [ - "https://mirror.bazel.build/github.com/gflags/gflags/archive/77592648e3f3be87d6c7123eb81cbad75f9aef5a.tar.gz", - "https://github.com/gflags/gflags/archive/77592648e3f3be87d6c7123eb81cbad75f9aef5a.tar.gz", - ], - ), - com_google_glog = dict( - sha256 = "819cb075d6b02b8e9c9c77c2be1d55cef7fec47e7c94359d2626f46268ae67bf", - strip_prefix = "glog-ba8a9f6952d04d1403b97df24e6836227751454e", - urls = ["https://github.com/google/glog/archive/ba8a9f6952d04d1403b97df24e6836227751454e.tar.gz"], - ), com_google_cel_cpp = dict( - sha256 = "37df0e66c84ddffe7bbac6e659856f48b34dbd9e1e869f57275493a20b271d11", - strip_prefix = "cel-cpp-592129c73f70de6d5ff8977aa94e030a1a9c4e8a", - urls = ["https://github.com/google/cel-cpp/archive/592129c73f70de6d5ff8977aa94e030a1a9c4e8a.tar.gz"], + sha256 = "a0e6a6ccf25c1e57ba3c7a997edd9279f8e0b0112e0a3ac705a5568fa32792fc", + strip_prefix = "cel-cpp-d56f26adb53d0f41508a909d15e74d9ffb0e8a6c", + urls = ["https://github.com/google/cel-cpp/archive/d56f26adb53d0f41508a909d15e74d9ffb0e8a6c.tar.gz"], ), com_google_re2 = dict( sha256 = "f31db9cd224d018a7e4fe88ef84aaa874b0b3ed91d4d98ee5a1531101d3fdc64", From 982e3fdd2a30bd681c939970fcea5b20b9a6285d Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 30 Jul 2019 14:37:05 -0700 Subject: [PATCH 11/32] combine engines Signed-off-by: Kuat Yessenov --- api/envoy/config/filter/http/rbac/v2/BUILD | 6 --- .../config/filter/http/rbac/v2/rbac.proto | 4 -- api/envoy/config/filter/network/rbac/v2/BUILD | 1 + api/envoy/config/rbac/v2/BUILD | 27 +++++++----- api/envoy/config/rbac/v2/rbac.proto | 5 +++ source/extensions/filters/common/rbac/BUILD | 1 + .../extensions/filters/common/rbac/engine.h | 12 +++--- .../filters/common/rbac/engine_impl.cc | 25 ++++++++--- .../filters/common/rbac/engine_impl.h | 9 ++-- source/extensions/filters/http/rbac/BUILD | 1 - .../filters/http/rbac/rbac_filter.cc | 41 +++---------------- .../filters/http/rbac/rbac_filter.h | 4 -- .../filters/network/rbac/rbac_filter.cc | 3 +- 13 files changed, 63 insertions(+), 76 deletions(-) diff --git a/api/envoy/config/filter/http/rbac/v2/BUILD b/api/envoy/config/filter/http/rbac/v2/BUILD index d828071efa68f..5edeb6fcdb996 100644 --- a/api/envoy/config/filter/http/rbac/v2/BUILD +++ b/api/envoy/config/filter/http/rbac/v2/BUILD @@ -5,12 +5,6 @@ licenses(["notice"]) # Apache 2 api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], - external_cc_proto_deps = [ - "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", - ], - external_proto_deps = [ - "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", - ], require_py = False, deps = ["//envoy/config/rbac/v2:rbac"], ) diff --git a/api/envoy/config/filter/http/rbac/v2/rbac.proto b/api/envoy/config/filter/http/rbac/v2/rbac.proto index 7281740f87236..0a75d9590fa5a 100644 --- a/api/envoy/config/filter/http/rbac/v2/rbac.proto +++ b/api/envoy/config/filter/http/rbac/v2/rbac.proto @@ -8,7 +8,6 @@ option java_package = "io.envoyproxy.envoy.config.filter.http.rbac.v2"; option go_package = "v2"; import "envoy/config/rbac/v2/rbac.proto"; -import "google/api/expr/v1alpha1/syntax.proto"; import "validate/validate.proto"; import "gogoproto/gogo.proto"; @@ -26,9 +25,6 @@ message RBAC { // but will emit stats and logs and can be used for rule testing. // If absent, no shadow RBAC policy will be applied. config.rbac.v2.RBAC shadow_rules = 2; - - // A symbolic expression over request attributes specifying an access control condition. - google.api.expr.v1alpha1.Expr condition = 3; } message RBACPerRoute { diff --git a/api/envoy/config/filter/network/rbac/v2/BUILD b/api/envoy/config/filter/network/rbac/v2/BUILD index 6182fe26748a0..5edeb6fcdb996 100644 --- a/api/envoy/config/filter/network/rbac/v2/BUILD +++ b/api/envoy/config/filter/network/rbac/v2/BUILD @@ -5,5 +5,6 @@ licenses(["notice"]) # Apache 2 api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], + require_py = False, deps = ["//envoy/config/rbac/v2:rbac"], ) diff --git a/api/envoy/config/rbac/v2/BUILD b/api/envoy/config/rbac/v2/BUILD index c2059893912c8..c05ea950612f9 100644 --- a/api/envoy/config/rbac/v2/BUILD +++ b/api/envoy/config/rbac/v2/BUILD @@ -5,6 +5,13 @@ load("@envoy_api//bazel:api_build_system.bzl", "api_go_proto_library", "api_prot api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], + external_cc_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto", + ], + external_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", + ], + require_py = False, visibility = ["//visibility:public"], deps = [ "//envoy/api/v2/core:address", @@ -14,13 +21,13 @@ api_proto_library_internal( ], ) -api_go_proto_library( - name = "rbac", - proto = ":rbac", - deps = [ - "//envoy/api/v2/core:address_go_proto", - "//envoy/api/v2/route:route_go_proto", - "//envoy/type/matcher:metadata_go_proto", - "//envoy/type/matcher:string_go_proto", - ], -) +#api_go_proto_library( +# name = "rbac", +# proto = ":rbac", +# deps = [ +# "//envoy/api/v2/core:address_go_proto", +# "//envoy/api/v2/route:route_go_proto", +# "//envoy/type/matcher:metadata_go_proto", +# "//envoy/type/matcher:string_go_proto", +# ], +#) diff --git a/api/envoy/config/rbac/v2/rbac.proto b/api/envoy/config/rbac/v2/rbac.proto index 3cfe43a828fef..6cb871c4bb002 100644 --- a/api/envoy/config/rbac/v2/rbac.proto +++ b/api/envoy/config/rbac/v2/rbac.proto @@ -7,6 +7,8 @@ import "envoy/api/v2/route/route.proto"; import "envoy/type/matcher/metadata.proto"; import "envoy/type/matcher/string.proto"; +import "google/api/expr/v1alpha1/syntax.proto"; + package envoy.config.rbac.v2; option java_outer_classname = "RbacProto"; @@ -77,6 +79,9 @@ message RBAC { // Maps from policy name to policy. A match occurs when at least one policy matches the request. map policies = 2; + + // A symbolic expression specifying an access control condition. + google.api.expr.v1alpha1.Expr condition = 3; } // Policy specifies a role and the principals that are assigned/denied the role. A policy matches if diff --git a/source/extensions/filters/common/rbac/BUILD b/source/extensions/filters/common/rbac/BUILD index 6aa4fc4088bde..363fe29f2fea9 100644 --- a/source/extensions/filters/common/rbac/BUILD +++ b/source/extensions/filters/common/rbac/BUILD @@ -53,6 +53,7 @@ envoy_cc_library( srcs = ["engine_impl.cc"], hdrs = ["engine_impl.h"], deps = [ + "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/common/rbac:engine_interface", "//source/extensions/filters/common/rbac:matchers_lib", "@envoy_api//envoy/api/v2/core:base_cc", diff --git a/source/extensions/filters/common/rbac/engine.h b/source/extensions/filters/common/rbac/engine.h index 4a07c03d2a312..093eb72fb7786 100644 --- a/source/extensions/filters/common/rbac/engine.h +++ b/source/extensions/filters/common/rbac/engine.h @@ -4,6 +4,7 @@ #include "envoy/http/filter.h" #include "envoy/http/header_map.h" #include "envoy/network/connection.h" +#include "envoy/stream_info/stream_info.h" namespace Envoy { namespace Extensions { @@ -24,24 +25,25 @@ class RoleBasedAccessControlEngine { * @param connection the downstream connection used to identify the action/principal. * @param headers the headers of the incoming request used to identify the action/principal. An * empty map should be used if there are no headers available. - * @param metadata the metadata with additional information about the action/principal. + * @param info the per-request or per-connection stream info with additional information + * about the action/principal. * @param effective_policy_id it will be filled by the matching policy's ID, * which is used to identity the source of the allow/deny. */ virtual bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata, + const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const PURE; /** * Returns whether or not the current action is permitted. * * @param connection the downstream connection used to identify the action/principal. - * @param metadata the metadata with additional information about the action/principal. + * @param info the per-request or per-connection stream info with additional information + * about the action/principal. * @param effective_policy_id it will be filled by the matching policy's ID, * which is used to identity the source of the allow/deny. */ - virtual bool allowed(const Network::Connection& connection, - const envoy::api::v2::core::Metadata& metadata, + virtual bool allowed(const Network::Connection& connection, const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const PURE; }; diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index c2b1e05cd2a1a..cc33b489d3858 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -11,7 +11,8 @@ namespace RBAC { RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( const envoy::config::rbac::v2::RBAC& rules) : allowed_if_matched_(rules.action() == - envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW) { + envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW), + expr_(rules.has_condition() ? Expr::create(rules.condition()) : nullptr) { for (const auto& policy : rules.policies()) { policies_.insert(std::make_pair(policy.first, policy.second)); } @@ -19,12 +20,12 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata, + const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const { bool matched = false; for (auto it = policies_.begin(); it != policies_.end(); it++) { - if (it->second.matches(connection, headers, metadata)) { + if (it->second.matches(connection, headers, info.dynamicMetadata())) { matched = true; if (effective_policy_id != nullptr) { *effective_policy_id = it->first; @@ -33,6 +34,20 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec } } + if (!matched && expr_ != nullptr) { + auto eval_status = Expr::evaluate(*expr_, info, headers); + + // evaluation error is effectively a denial + if (!eval_status.has_value()) { + return false; + } + auto result = eval_status.value(); + + // condition is effectively OR-ed with policy matchers + // non-bool is effectivey "false" + matched = result.IsBool() ? result.BoolOrDie() : false; + } + // only allowed if: // - matched and ALLOW action // - not matched and DENY action @@ -40,10 +55,10 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec } bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connection, - const envoy::api::v2::core::Metadata& metadata, + const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const { static const Http::HeaderMapImpl* empty_header = new Http::HeaderMapImpl(); - return allowed(connection, *empty_header, metadata, effective_policy_id); + return allowed(connection, *empty_header, info, effective_policy_id); } } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index cabbeb31e30fa..85e9dd8a15ea7 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -2,6 +2,7 @@ #include "envoy/config/filter/http/rbac/v2/rbac.pb.h" +#include "extensions/filters/common/expr/evaluator.h" #include "extensions/filters/common/rbac/engine.h" #include "extensions/filters/common/rbac/matchers.h" @@ -16,17 +17,17 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { RoleBasedAccessControlEngineImpl(const envoy::config::rbac::v2::RBAC& rules); bool allowed(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata, - std::string* effective_policy_id) const override; + const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const override; - bool allowed(const Network::Connection& connection, - const envoy::api::v2::core::Metadata& metadata, + bool allowed(const Network::Connection& connection, const StreamInfo::StreamInfo& info, std::string* effective_policy_id) const override; private: const bool allowed_if_matched_; std::map policies_; + + Expr::ExpressionPtr expr_; }; } // namespace RBAC diff --git a/source/extensions/filters/http/rbac/BUILD b/source/extensions/filters/http/rbac/BUILD index 37970040848d0..f7adc41aa8507 100644 --- a/source/extensions/filters/http/rbac/BUILD +++ b/source/extensions/filters/http/rbac/BUILD @@ -28,7 +28,6 @@ envoy_cc_library( "//include/envoy/http:filter_interface", "//include/envoy/stats:stats_macros", "//source/common/http:utility_lib", - "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/common/rbac:engine_lib", "//source/extensions/filters/common/rbac:utility_lib", "//source/extensions/filters/http:well_known_names", diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index f696c48b14b0d..ec3dff972d4d5 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -24,9 +24,7 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const std::string& stats_prefix, Stats::Scope& scope) : stats_(Filters::Common::RBAC::generateStats(stats_prefix, scope)), engine_(Filters::Common::RBAC::createEngine(proto_config)), - shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)), - expr_(proto_config.has_condition() ? Filters::Common::Expr::create(proto_config.condition()) - : nullptr) {} + shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)) {} const absl::optional& RoleBasedAccessControlFilterConfig::engine(const Router::RouteConstSharedPtr route, @@ -78,8 +76,8 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (shadow_engine.has_value()) { std::string shadow_resp_code = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; - if (shadow_engine->allowed(*callbacks_->connection(), headers, - callbacks_->streamInfo().dynamicMetadata(), &effective_policy_id)) { + if (shadow_engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), + &effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); config_->stats().shadow_allowed_.inc(); } else { @@ -107,10 +105,10 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head const auto& engine = config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Enforced); if (engine.has_value()) { - if (engine->allowed(*callbacks_->connection(), headers, - callbacks_->streamInfo().dynamicMetadata(), nullptr)) { + if (engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); + return Http::FilterHeadersStatus::Continue; } else { ENVOY_LOG(debug, "enforced denied"); callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, @@ -118,36 +116,9 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head config_->stats().denied_.inc(); return Http::FilterHeadersStatus::StopIteration; } - } else { - ENVOY_LOG(debug, "no engine, allowed by default"); - } - - if (config_->expr() != nullptr) { - auto eval_status = - Filters::Common::Expr::evaluate(*config_->expr(), callbacks_->streamInfo(), headers); - if (!eval_status.has_value()) { - ENVOY_LOG(debug, "evaluation failed"); - callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, - absl::nullopt, RcDetails::get().RbacAccessDenied); - config_->stats().denied_.inc(); - return Http::FilterHeadersStatus::StopIteration; - } - auto result = eval_status.value(); - ENVOY_LOG(trace, "value: {}", - result.IsError() - ? result.ErrorOrDie()->message() - : (result.IsString() - ? result.StringOrDie().value() - : (result.IsInt64() ? absl::StrCat(result.Int64OrDie()) : ""))); - if (!result.IsBool() || (result.IsBool() && !result.BoolOrDie())) { - ENVOY_LOG(debug, "enforced: denied by condition"); - callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, - absl::nullopt, RcDetails::get().RbacAccessDenied); - config_->stats().denied_.inc(); - return Http::FilterHeadersStatus::StopIteration; - } } + ENVOY_LOG(debug, "no engine, allowed by default"); return Http::FilterHeadersStatus::Continue; } diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index 66f73d8b2365a..5397a3c31c586 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -9,7 +9,6 @@ #include "common/common/logger.h" -#include "extensions/filters/common/expr/evaluator.h" #include "extensions/filters/common/rbac/engine_impl.h" #include "extensions/filters/common/rbac/utility.h" @@ -48,8 +47,6 @@ class RoleBasedAccessControlFilterConfig { engine(const Router::RouteConstSharedPtr route, Filters::Common::RBAC::EnforcementMode mode) const; - Filters::Common::Expr::Expression* expr() { return expr_.get(); } - private: const absl::optional& engine(Filters::Common::RBAC::EnforcementMode mode) const { @@ -60,7 +57,6 @@ class RoleBasedAccessControlFilterConfig { const absl::optional engine_; const absl::optional shadow_engine_; - const Filters::Common::Expr::ExpressionPtr expr_; }; using RoleBasedAccessControlFilterConfigSharedPtr = diff --git a/source/extensions/filters/network/rbac/rbac_filter.cc b/source/extensions/filters/network/rbac/rbac_filter.cc index 0c5008c88b1d8..a6ab38df16238 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.cc +++ b/source/extensions/filters/network/rbac/rbac_filter.cc @@ -80,8 +80,7 @@ RoleBasedAccessControlFilter::checkEngine(Filters::Common::RBAC::EnforcementMode const auto& engine = config_->engine(mode); if (engine.has_value()) { std::string effective_policy_id; - if (engine->allowed(callbacks_->connection(), - callbacks_->connection().streamInfo().dynamicMetadata(), + if (engine->allowed(callbacks_->connection(), callbacks_->connection().streamInfo(), &effective_policy_id)) { if (mode == Filters::Common::RBAC::EnforcementMode::Shadow) { ENVOY_LOG(debug, "shadow allowed"); From c31b4a7ebca4a4b65911deec9968fb1057912cfa Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 30 Jul 2019 14:43:21 -0700 Subject: [PATCH 12/32] make arena explicit Signed-off-by: Kuat Yessenov --- source/extensions/filters/common/expr/evaluator.cc | 10 ++++------ source/extensions/filters/common/expr/evaluator.h | 7 +++++-- source/extensions/filters/common/rbac/engine_impl.cc | 3 ++- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index dc22872fc60d6..d031144425be9 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -2,8 +2,6 @@ #include "envoy/common/exception.h" -#include "common/protobuf/protobuf.h" - #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" @@ -35,17 +33,17 @@ ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr) { return std::move(cel_expression_status.ValueOrDie()); } -absl::optional evaluate(const Expression& expr, const StreamInfo::StreamInfo& info, +absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, + const StreamInfo::StreamInfo& info, const Http::HeaderMap& headers) { - Protobuf::Arena arena; google::api::expr::runtime::Activation activation; const RequestWrapper request(headers, info); const ConnectionWrapper connection(info); activation.InsertValue(Request, CelValue::CreateMap(&request)); - activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), &arena)); + activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), arena)); activation.InsertValue(Connection, CelValue::CreateMap(&connection)); - auto eval_status = expr.Evaluate(activation, &arena); + auto eval_status = expr.Evaluate(activation, arena); if (!eval_status.ok()) { return {}; } diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 3034f207cff81..0f551f2ec0847 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -3,6 +3,7 @@ #include "envoy/stream_info/stream_info.h" #include "common/http/headers.h" +#include "common/protobuf/protobuf.h" #include "extensions/filters/common/expr/context.h" @@ -22,8 +23,10 @@ using ExpressionPtr = std::unique_ptr; // Throws an exception if fails to construct a runtime expression. ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr); -// Evaluates an expression for a request. -absl::optional evaluate(const Expression& expr, const StreamInfo::StreamInfo& info, +// Evaluates an expression for a request. The arena is used to hold intermediate computational +// results and potentially the final value. +absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, + const StreamInfo::StreamInfo& info, const Http::HeaderMap& headers); } // namespace Expr diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index cc33b489d3858..9135eb0500eaa 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -35,7 +35,8 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec } if (!matched && expr_ != nullptr) { - auto eval_status = Expr::evaluate(*expr_, info, headers); + Protobuf::Arena arena; + auto eval_status = Expr::evaluate(*expr_, &arena, info, headers); // evaluation error is effectively a denial if (!eval_status.has_value()) { From e309dd848001b36f2bd02ae3a7f1489510ac2d50 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 30 Jul 2019 16:10:25 -0700 Subject: [PATCH 13/32] more attributes Signed-off-by: Kuat Yessenov --- .../extensions/filters/common/expr/context.cc | 47 +++++++++++++++++++ .../extensions/filters/common/expr/context.h | 25 ++++++++-- .../filters/common/expr/evaluator.cc | 2 + test/extensions/filters/common/rbac/BUILD | 1 + .../filters/common/rbac/engine_impl_test.cc | 6 ++- 5 files changed, 77 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 5267dbac10a7f..9b48a3335a6cd 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -63,6 +63,27 @@ absl::optional RequestWrapper::operator[](CelValue key) const { } } else if (value == TotalSize) { return CelValue::CreateInt64(info_.bytesReceived() + wrapper_.headers_.byteSize()); + } else if (value == Duration) { + auto duration = info_.requestComplete(); + if (duration.has_value()) { + return CelValue::CreateDuration(absl::FromChrono(duration.value())); + } + } + return {}; +} + +absl::optional ResponseWrapper::operator[](CelValue key) const { + if (!key.IsString()) { + return {}; + } + auto value = key.StringOrDie().value(); + if (value == Code) { + auto code = info_.responseCode(); + if (code.has_value()) { + return CelValue::CreateInt64(code.value()); + } + } else if (value == Size) { + return CelValue::CreateInt64(info_.bytesSent()); } return {}; } @@ -74,8 +95,34 @@ absl::optional ConnectionWrapper::operator[](CelValue key) const { auto value = key.StringOrDie().value(); if (value == LocalAddress) { return CelValue::CreateString(info_.downstreamLocalAddress()->asStringView()); + } else if (value == LocalPort) { + if (info_.downstreamLocalAddress()->ip() != nullptr) { + return CelValue::CreateInt64(info_.downstreamLocalAddress()->ip()->port()); + } } else if (value == RemoteAddress) { return CelValue::CreateString(info_.downstreamRemoteAddress()->asStringView()); + } else if (value == RemotePort) { + if (info_.downstreamRemoteAddress()->ip() != nullptr) { + return CelValue::CreateInt64(info_.downstreamRemoteAddress()->ip()->port()); + } + } else if (value == UpstreamAddress) { + auto upstream_host = info_.upstreamHost(); + if (upstream_host != nullptr && upstream_host->address() != nullptr) { + return CelValue::CreateString(upstream_host->address()->asStringView()); + } + } else if (value == UpstreamPort) { + auto upstream_host = info_.upstreamHost(); + if (upstream_host != nullptr && upstream_host->address() != nullptr && + upstream_host->address()->ip() != nullptr) { + return CelValue::CreateInt64(upstream_host->address()->ip()->port()); + } + } + + auto downstream_ssl = info_.downstreamSslConnection(); + if (downstream_ssl != nullptr) { + if (value == MTLS) { + return CelValue::CreateBool(downstream_ssl->peerCertificatePresented()); + } } return {}; diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 78102bbca4160..3cf920838d241 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -14,7 +14,7 @@ namespace Expr { using CelValue = google::api::expr::runtime::CelValue; -// Symbols for traversing the request properties in the expressions +// Symbols for traversing the request properties constexpr absl::string_view Request = "request"; constexpr absl::string_view Path = "path"; constexpr absl::string_view Host = "host"; @@ -27,14 +27,24 @@ constexpr absl::string_view ID = "id"; constexpr absl::string_view UserAgent = "useragent"; constexpr absl::string_view Size = "size"; constexpr absl::string_view TotalSize = "total_size"; +constexpr absl::string_view Duration = "duration"; -// Per-request metadata +// Symbols for traversing the response properties +constexpr absl::string_view Response = "response"; +constexpr absl::string_view Code = "code"; + +// Per-request or per-connection metadata constexpr absl::string_view Metadata = "metadata"; -// Downstream connection properties +// Connection properties constexpr absl::string_view Connection = "connection"; constexpr absl::string_view LocalAddress = "local_address"; +constexpr absl::string_view LocalPort = "local_port"; constexpr absl::string_view RemoteAddress = "remote_address"; +constexpr absl::string_view RemotePort = "remote_port"; +constexpr absl::string_view UpstreamAddress = "upstream_address"; +constexpr absl::string_view UpstreamPort = "upstream_port"; +constexpr absl::string_view MTLS = "mtls"; class RequestWrapper; @@ -69,6 +79,15 @@ class RequestWrapper : public BaseWrapper { const StreamInfo::StreamInfo& info_; }; +class ResponseWrapper : public BaseWrapper { +public: + ResponseWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} + absl::optional operator[](CelValue key) const override; + +private: + const StreamInfo::StreamInfo& info_; +}; + class ConnectionWrapper : public BaseWrapper { public: ConnectionWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index d031144425be9..2a8514a3233e6 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -38,8 +38,10 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena const Http::HeaderMap& headers) { google::api::expr::runtime::Activation activation; const RequestWrapper request(headers, info); + const ResponseWrapper response(info); const ConnectionWrapper connection(info); activation.InsertValue(Request, CelValue::CreateMap(&request)); + activation.InsertValue(Response, CelValue::CreateMap(&response)); activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), arena)); activation.InsertValue(Connection, CelValue::CreateMap(&connection)); diff --git a/test/extensions/filters/common/rbac/BUILD b/test/extensions/filters/common/rbac/BUILD index d9b47acebf6ae..1b0671adbcbc5 100644 --- a/test/extensions/filters/common/rbac/BUILD +++ b/test/extensions/filters/common/rbac/BUILD @@ -32,6 +32,7 @@ envoy_extension_cc_test( "//source/extensions/filters/common/rbac:engine_lib", "//test/mocks/network:network_mocks", "//test/mocks/ssl:ssl_mocks", + "//test/mocks/stream_info:stream_info_mocks", "//test/test_common:utility_lib", ], ) diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index bdca0c55ae5b0..84589949b6d62 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -4,6 +4,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/ssl/mocks.h" +#include "test/mocks/stream_info/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -25,7 +26,10 @@ void checkEngine(const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expe const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata(), std::string* policy_id = nullptr) { - EXPECT_EQ(expected, engine.allowed(connection, headers, metadata, policy_id)); + NiceMock info; + EXPECT_CALL(info, dynamicMetadata()) + .WillRepeatedly(ReturnRef(const_cast(metadata))); + EXPECT_EQ(expected, engine.allowed(connection, headers, info, policy_id)); } TEST(RoleBasedAccessControlEngineImpl, Disabled) { From 6cdbe8e03d907e241c75ed29220323ebbfe87fd0 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 30 Jul 2019 18:25:16 -0700 Subject: [PATCH 14/32] build fix Signed-off-by: Kuat Yessenov --- source/extensions/filters/common/rbac/engine_impl.cc | 2 +- test/extensions/filters/common/rbac/mocks.h | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 9135eb0500eaa..90e8821cc2f61 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -45,7 +45,7 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec auto result = eval_status.value(); // condition is effectively OR-ed with policy matchers - // non-bool is effectivey "false" + // non-bool is effectively "false" matched = result.IsBool() ? result.BoolOrDie() : false; } diff --git a/test/extensions/filters/common/rbac/mocks.h b/test/extensions/filters/common/rbac/mocks.h index 6b14a834eec79..50555419dd4cd 100644 --- a/test/extensions/filters/common/rbac/mocks.h +++ b/test/extensions/filters/common/rbac/mocks.h @@ -17,11 +17,10 @@ class MockEngine : public RoleBasedAccessControlEngineImpl { MOCK_CONST_METHOD4(allowed, bool(const Envoy::Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&, std::string* effective_policy_id)); + const StreamInfo::StreamInfo&, std::string* effective_policy_id)); - MOCK_CONST_METHOD3(allowed, - bool(const Envoy::Network::Connection&, const envoy::api::v2::core::Metadata&, - std::string* effective_policy_id)); + MOCK_CONST_METHOD3(allowed, bool(const Envoy::Network::Connection&, const StreamInfo::StreamInfo&, + std::string* effective_policy_id)); }; } // namespace RBAC From f934dda17048e67bdafe6577430b7d4ee4f8dfa0 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 00:42:18 -0700 Subject: [PATCH 15/32] refactor Signed-off-by: Kuat Yessenov --- api/envoy/config/rbac/v2/rbac.proto | 9 +++-- .../filters/common/expr/evaluator.cc | 19 ++++++++- .../filters/common/expr/evaluator.h | 13 ++++++- source/extensions/filters/common/rbac/BUILD | 2 +- .../filters/common/rbac/engine_impl.cc | 31 ++++++--------- .../filters/common/rbac/engine_impl.h | 5 +-- .../filters/common/rbac/matchers.cc | 33 ++++++++-------- .../extensions/filters/common/rbac/matchers.h | 39 ++++++++++++------- 8 files changed, 91 insertions(+), 60 deletions(-) diff --git a/api/envoy/config/rbac/v2/rbac.proto b/api/envoy/config/rbac/v2/rbac.proto index 6cb871c4bb002..5a53cf76577a7 100644 --- a/api/envoy/config/rbac/v2/rbac.proto +++ b/api/envoy/config/rbac/v2/rbac.proto @@ -79,14 +79,11 @@ message RBAC { // Maps from policy name to policy. A match occurs when at least one policy matches the request. map policies = 2; - - // A symbolic expression specifying an access control condition. - google.api.expr.v1alpha1.Expr condition = 3; } // Policy specifies a role and the principals that are assigned/denied the role. A policy matches if // and only if at least one of its permissions match the action taking place AND at least one of its -// principals match the downstream. +// principals match the downstream AND the condition is true if specified. message Policy { // Required. The set of permissions that define a role. Each permission is matched with OR // semantics. To match all actions for this policy, a single Permission with the `any` field set @@ -97,6 +94,10 @@ message Policy { // principal is matched with OR semantics. To match all downstreams for this policy, a single // Principal with the `any` field set to true should be used. repeated Principal principals = 2 [(validate.rules).repeated .min_items = 1]; + + // An optional symbolic expression specifying an access control condition. + // The condition is combined with AND semantics. + google.api.expr.v1alpha1.Expr condition = 3; } // Permission defines an action (or actions) that a principal can take. diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index 2a8514a3233e6..8649e7046e488 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -11,7 +11,7 @@ namespace Filters { namespace Common { namespace Expr { -ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr) { +BuilderPtr createBuilder() { google::api::expr::runtime::InterpreterOptions options; // Conformance with spec/go runtimes requires this setting @@ -24,8 +24,12 @@ ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr) { throw EnvoyException( absl::StrCat("failed to register built-in functions: ", register_status.message())); } + return builder; +} + +ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alpha1::Expr& expr) { google::api::expr::v1alpha1::SourceInfo source_info; - auto cel_expression_status = builder->CreateExpression(&expr, &source_info); + auto cel_expression_status = builder.CreateExpression(&expr, &source_info); if (!cel_expression_status.ok()) { throw EnvoyException( absl::StrCat("failed to create an expression: ", cel_expression_status.status().message())); @@ -53,6 +57,17 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena return eval_status.ValueOrDie(); } +bool matches(const Expression& expr, const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers) { + Protobuf::Arena arena; + auto eval_status = Expr::evaluate(expr, &arena, info, headers); + if (!eval_status.has_value()) { + return false; + } + auto result = eval_status.value(); + return result.IsBool() ? result.BoolOrDie() : false; +} + } // namespace Expr } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 0f551f2ec0847..24d098109d68b 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -16,12 +16,18 @@ namespace Filters { namespace Common { namespace Expr { +using Builder = google::api::expr::runtime::CelExpressionBuilder; +using BuilderPtr = std::unique_ptr; using Expression = google::api::expr::runtime::CelExpression; using ExpressionPtr = std::unique_ptr; +// Creates an expression builder. +// Throws an exception if fails to construct an expression builder. +BuilderPtr createBuilder(); + // Creates an interpretable expression from a protobuf representation. // Throws an exception if fails to construct a runtime expression. -ExpressionPtr create(const google::api::expr::v1alpha1::Expr& expr); +ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alpha1::Expr& expr); // Evaluates an expression for a request. The arena is used to hold intermediate computational // results and potentially the final value. @@ -29,6 +35,11 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena const StreamInfo::StreamInfo& info, const Http::HeaderMap& headers); +// Evaluates an expression and returns true if the expression evaluates to "true". +// Returns false if the expression fails to evaluate. +bool matches(const Expression& expr, const StreamInfo::StreamInfo& info, + const Http::HeaderMap& headers); + } // namespace Expr } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/rbac/BUILD b/source/extensions/filters/common/rbac/BUILD index 363fe29f2fea9..94398324482f5 100644 --- a/source/extensions/filters/common/rbac/BUILD +++ b/source/extensions/filters/common/rbac/BUILD @@ -33,6 +33,7 @@ envoy_cc_library( "//source/common/common:matchers_lib", "//source/common/http:header_utility_lib", "//source/common/network:cidr_range_lib", + "//source/extensions/filters/common/expr:evaluator_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/config/rbac/v2:rbac_cc", ], @@ -53,7 +54,6 @@ envoy_cc_library( srcs = ["engine_impl.cc"], hdrs = ["engine_impl.h"], deps = [ - "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/common/rbac:engine_interface", "//source/extensions/filters/common/rbac:matchers_lib", "@envoy_api//envoy/api/v2/core:base_cc", diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 90e8821cc2f61..3493a0cf5d0f2 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -11,10 +11,18 @@ namespace RBAC { RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( const envoy::config::rbac::v2::RBAC& rules) : allowed_if_matched_(rules.action() == - envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW), - expr_(rules.has_condition() ? Expr::create(rules.condition()) : nullptr) { + envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW) { + // guard expression builder by presence of a condition in policies for (const auto& policy : rules.policies()) { - policies_.insert(std::make_pair(policy.first, policy.second)); + if (policy.second.has_condition()) { + builder_ = Expr::createBuilder(); + break; + } + } + + for (const auto& policy : rules.policies()) { + policies_.insert(std::make_pair( + policy.first, std::make_unique(policy.second, builder_.get()))); } } @@ -25,7 +33,7 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec bool matched = false; for (auto it = policies_.begin(); it != policies_.end(); it++) { - if (it->second.matches(connection, headers, info.dynamicMetadata())) { + if (it->second->matches(connection, headers, info)) { matched = true; if (effective_policy_id != nullptr) { *effective_policy_id = it->first; @@ -34,21 +42,6 @@ bool RoleBasedAccessControlEngineImpl::allowed(const Network::Connection& connec } } - if (!matched && expr_ != nullptr) { - Protobuf::Arena arena; - auto eval_status = Expr::evaluate(*expr_, &arena, info, headers); - - // evaluation error is effectively a denial - if (!eval_status.has_value()) { - return false; - } - auto result = eval_status.value(); - - // condition is effectively OR-ed with policy matchers - // non-bool is effectively "false" - matched = result.IsBool() ? result.BoolOrDie() : false; - } - // only allowed if: // - matched and ALLOW action // - not matched and DENY action diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index 85e9dd8a15ea7..91d5a36c720ae 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -2,7 +2,6 @@ #include "envoy/config/filter/http/rbac/v2/rbac.pb.h" -#include "extensions/filters/common/expr/evaluator.h" #include "extensions/filters/common/rbac/engine.h" #include "extensions/filters/common/rbac/matchers.h" @@ -25,9 +24,9 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { private: const bool allowed_if_matched_; - std::map policies_; + std::map> policies_; - Expr::ExpressionPtr expr_; + Expr::BuilderPtr builder_; }; } // namespace RBAC diff --git a/source/extensions/filters/common/rbac/matchers.cc b/source/extensions/filters/common/rbac/matchers.cc index d718e8b34167c..961b15866f09d 100644 --- a/source/extensions/filters/common/rbac/matchers.cc +++ b/source/extensions/filters/common/rbac/matchers.cc @@ -70,9 +70,9 @@ AndMatcher::AndMatcher(const envoy::config::rbac::v2::Principal_Set& set) { bool AndMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const { + const StreamInfo::StreamInfo& info) const { for (const auto& matcher : matchers_) { - if (!matcher->matches(connection, headers, metadata)) { + if (!matcher->matches(connection, headers, info)) { return false; } } @@ -95,9 +95,9 @@ OrMatcher::OrMatcher(const Protobuf::RepeatedPtrField<::envoy::config::rbac::v2: bool OrMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const { + const StreamInfo::StreamInfo& info) const { for (const auto& matcher : matchers_) { - if (matcher->matches(connection, headers, metadata)) { + if (matcher->matches(connection, headers, info)) { return true; } } @@ -107,17 +107,17 @@ bool OrMatcher::matches(const Network::Connection& connection, bool NotMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const { - return !matcher_->matches(connection, headers, metadata); + const StreamInfo::StreamInfo& info) const { + return !matcher_->matches(connection, headers, info); } bool HeaderMatcher::matches(const Network::Connection&, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const { + const StreamInfo::StreamInfo&) const { return Envoy::Http::HeaderUtility::matchHeaders(headers, header_); } bool IPMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&) const { + const StreamInfo::StreamInfo&) const { const Envoy::Network::Address::InstanceConstSharedPtr& ip = destination_ ? connection.localAddress() : connection.remoteAddress(); @@ -125,14 +125,14 @@ bool IPMatcher::matches(const Network::Connection& connection, const Envoy::Http } bool PortMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&) const { + const StreamInfo::StreamInfo&) const { const Envoy::Network::Address::Ip* ip = connection.localAddress().get()->ip(); return ip && ip->port() == port_; } bool AuthenticatedMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&) const { + const StreamInfo::StreamInfo&) const { const auto* ssl = connection.ssl(); if (!ssl) { // connection was not authenticated return false; @@ -152,20 +152,21 @@ bool AuthenticatedMatcher::matches(const Network::Connection& connection, } bool MetadataMatcher::matches(const Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata& metadata) const { - return matcher_.match(metadata); + const StreamInfo::StreamInfo& info) const { + return matcher_.match(info.dynamicMetadata()); } bool PolicyMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const { - return permissions_.matches(connection, headers, metadata) && - principals_.matches(connection, headers, metadata); + const StreamInfo::StreamInfo& info) const { + return permissions_.matches(connection, headers, info) && + principals_.matches(connection, headers, info) && + (expr_ == nullptr ? true : Expr::matches(*expr_, info, headers)); } bool RequestedServerNameMatcher::matches(const Network::Connection& connection, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&) const { + const StreamInfo::StreamInfo&) const { return match(connection.requestedServerName()); } diff --git a/source/extensions/filters/common/rbac/matchers.h b/source/extensions/filters/common/rbac/matchers.h index 28b81846e1149..28796763631b2 100644 --- a/source/extensions/filters/common/rbac/matchers.h +++ b/source/extensions/filters/common/rbac/matchers.h @@ -11,6 +11,8 @@ #include "common/http/header_utility.h" #include "common/network/cidr_range.h" +#include "extensions/filters/common/expr/evaluator.h" + namespace Envoy { namespace Extensions { namespace Filters { @@ -36,7 +38,7 @@ class Matcher { * @param metadata the additional information about the action/principal. */ virtual bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const PURE; + const StreamInfo::StreamInfo& info) const PURE; /** * Creates a shared instance of a matcher based off the rules defined in the Permission config @@ -57,7 +59,7 @@ class Matcher { class AlwaysMatcher : public Matcher { public: bool matches(const Network::Connection&, const Envoy::Http::HeaderMap&, - const envoy::api::v2::core::Metadata&) const override { + const StreamInfo::StreamInfo&) const override { return true; } }; @@ -72,7 +74,7 @@ class AndMatcher : public Matcher { AndMatcher(const envoy::config::rbac::v2::Principal_Set& ids); bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: std::vector matchers_; @@ -90,7 +92,7 @@ class OrMatcher : public Matcher { OrMatcher(const Protobuf::RepeatedPtrField<::envoy::config::rbac::v2::Principal>& ids); bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: std::vector matchers_; @@ -104,7 +106,7 @@ class NotMatcher : public Matcher { : matcher_(Matcher::create(principal)) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: MatcherConstSharedPtr matcher_; @@ -119,7 +121,7 @@ class HeaderMatcher : public Matcher { HeaderMatcher(const envoy::api::v2::route::HeaderMatcher& matcher) : header_(matcher) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: const Envoy::Http::HeaderUtility::HeaderData header_; @@ -135,7 +137,7 @@ class IPMatcher : public Matcher { : range_(Network::Address::CidrRange::create(range)), destination_(destination) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: const Network::Address::CidrRange range_; @@ -150,7 +152,7 @@ class PortMatcher : public Matcher { PortMatcher(const uint32_t port) : port_(port) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: const uint32_t port_; @@ -168,7 +170,7 @@ class AuthenticatedMatcher : public Matcher { : absl::nullopt) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: const absl::optional matcher_; @@ -177,18 +179,27 @@ class AuthenticatedMatcher : public Matcher { /** * Matches a Policy which is a collection of permission and principal matchers. If any action * matches a permission, the principals are then checked for a match. + * The condition is a conjunction clause. */ class PolicyMatcher : public Matcher { public: - PolicyMatcher(const envoy::config::rbac::v2::Policy& policy) - : permissions_(policy.permissions()), principals_(policy.principals()) {} + PolicyMatcher(const envoy::config::rbac::v2::Policy& policy, Expr::Builder* builder) + : permissions_(policy.permissions()), principals_(policy.principals()), + condition_(policy.condition()) { + if (policy.has_condition()) { + expr_ = Expr::createExpression(*builder, condition_); + } + } bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; private: const OrMatcher permissions_; const OrMatcher principals_; + + const google::api::expr::v1alpha1::Expr condition_; + Expr::ExpressionPtr expr_; }; class MetadataMatcher : public Matcher { @@ -196,7 +207,7 @@ class MetadataMatcher : public Matcher { MetadataMatcher(const Envoy::Matchers::MetadataMatcher& matcher) : matcher_(matcher) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata& metadata) const override; + const StreamInfo::StreamInfo& info) const override; private: const Envoy::Matchers::MetadataMatcher matcher_; @@ -212,7 +223,7 @@ class RequestedServerNameMatcher : public Matcher, Envoy::Matchers::StringMatche : Envoy::Matchers::StringMatcher(requested_server_name) {} bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers, - const envoy::api::v2::core::Metadata&) const override; + const StreamInfo::StreamInfo&) const override; }; } // namespace RBAC From bf99900c27a3ceae0021073038a667f5cb317c41 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 11:04:40 -0700 Subject: [PATCH 16/32] fix unit tests Signed-off-by: Kuat Yessenov --- source/extensions/filters/common/expr/context.cc | 10 +++------- source/extensions/filters/common/rbac/engine_impl.cc | 3 +-- .../extensions/filters/common/rbac/engine_impl_test.cc | 3 +-- test/extensions/filters/common/rbac/matchers_test.cc | 6 ++++-- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 9b48a3335a6cd..b55f844b117f4 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -116,13 +116,9 @@ absl::optional ConnectionWrapper::operator[](CelValue key) const { upstream_host->address()->ip() != nullptr) { return CelValue::CreateInt64(upstream_host->address()->ip()->port()); } - } - - auto downstream_ssl = info_.downstreamSslConnection(); - if (downstream_ssl != nullptr) { - if (value == MTLS) { - return CelValue::CreateBool(downstream_ssl->peerCertificatePresented()); - } + } else if (value == MTLS) { + return CelValue::CreateBool(info_.downstreamSslConnection() != nullptr && + info_.downstreamSslConnection()->peerCertificatePresented()); } return {}; diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 3493a0cf5d0f2..540d44c6c9639 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -21,8 +21,7 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( } for (const auto& policy : rules.policies()) { - policies_.insert(std::make_pair( - policy.first, std::make_unique(policy.second, builder_.get()))); + policies_.emplace(policy.first, std::make_unique(policy.second, builder_.get())); } } diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index 84589949b6d62..f510d59790d7e 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -27,8 +27,7 @@ void checkEngine(const RBAC::RoleBasedAccessControlEngineImpl& engine, bool expe const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata(), std::string* policy_id = nullptr) { NiceMock info; - EXPECT_CALL(info, dynamicMetadata()) - .WillRepeatedly(ReturnRef(const_cast(metadata))); + EXPECT_CALL(Const(info), dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); EXPECT_EQ(expected, engine.allowed(connection, headers, info, policy_id)); } diff --git a/test/extensions/filters/common/rbac/matchers_test.cc b/test/extensions/filters/common/rbac/matchers_test.cc index 424c1c56a74ef..4999a876e5097 100644 --- a/test/extensions/filters/common/rbac/matchers_test.cc +++ b/test/extensions/filters/common/rbac/matchers_test.cc @@ -25,7 +25,9 @@ void checkMatcher( const Envoy::Network::Connection& connection = Envoy::Network::MockConnection(), const Envoy::Http::HeaderMap& headers = Envoy::Http::HeaderMapImpl(), const envoy::api::v2::core::Metadata& metadata = envoy::api::v2::core::Metadata()) { - EXPECT_EQ(expected, matcher.matches(connection, headers, metadata)); + NiceMock info; + EXPECT_CALL(Const(info), dynamicMetadata()).WillRepeatedly(ReturnRef(metadata)); + EXPECT_EQ(expected, matcher.matches(connection, headers, info)); } TEST(AlwaysMatcher, AlwaysMatches) { checkMatcher(RBAC::AlwaysMatcher(), true); } @@ -270,7 +272,7 @@ TEST(PolicyMatcher, PolicyMatcher) { policy.add_principals()->mutable_authenticated()->mutable_principal_name()->set_exact("foo"); policy.add_principals()->mutable_authenticated()->mutable_principal_name()->set_exact("bar"); - RBAC::PolicyMatcher matcher(policy); + RBAC::PolicyMatcher matcher(policy, nullptr); Envoy::Network::MockConnection conn; Envoy::Ssl::MockConnectionInfo ssl; From b27790a41c9c7c316031abbfe45c232d9cc3d74d Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 15:44:34 -0700 Subject: [PATCH 17/32] unit tests Signed-off-by: Kuat Yessenov --- test/extensions/filters/common/expr/BUILD | 25 ++ .../filters/common/expr/context_test.cc | 247 ++++++++++++++++++ .../filters/common/rbac/engine_impl_test.cc | 76 ++++++ 3 files changed, 348 insertions(+) create mode 100644 test/extensions/filters/common/expr/BUILD create mode 100644 test/extensions/filters/common/expr/context_test.cc diff --git a/test/extensions/filters/common/expr/BUILD b/test/extensions/filters/common/expr/BUILD new file mode 100644 index 0000000000000..8ce5555328bf0 --- /dev/null +++ b/test/extensions/filters/common/expr/BUILD @@ -0,0 +1,25 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +envoy_package() + +envoy_extension_cc_test( + name = "context_test", + srcs = ["context_test.cc"], + extension_name = "envoy.filters.http.rbac", + deps = [ + "//source/extensions/filters/common/expr:context_lib", + "//test/mocks/ssl:ssl_mocks", + "//test/mocks/stream_info:stream_info_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc new file mode 100644 index 0000000000000..8a559dcf37bef --- /dev/null +++ b/test/extensions/filters/common/expr/context_test.cc @@ -0,0 +1,247 @@ +#include "common/network/utility.h" + +#include "extensions/filters/common/expr/context.h" + +#include "test/mocks/ssl/mocks.h" +#include "test/mocks/stream_info/mocks.h" +#include "test/mocks/upstream/mocks.h" + +#include "absl/time/time.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Const; +using testing::Return; +using testing::ReturnRef; + +namespace Envoy { +namespace Extensions { +namespace Filters { +namespace Common { +namespace Expr { +namespace { + +constexpr absl::string_view Undefined = "undefined"; + +TEST(Context, RequestAttributes) { + NiceMock info; + Http::TestHeaderMapImpl header_map{ + {":method", "POST"}, {":scheme", "http"}, {":path", "/meow"}, + {":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"}, + {"content-length", "10"}, {"x-request-id", "blah"}, + }; + RequestWrapper request(header_map, info); + + EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); + // "2018-04-03T23:06:09.123Z". + const SystemTime start_time(std::chrono::milliseconds(1522796769123)); + EXPECT_CALL(info, startTime()).WillRepeatedly(Return(start_time)); + absl::optional dur = std::chrono::nanoseconds(15000000); + EXPECT_CALL(info, requestComplete()).WillRepeatedly(Return(dur)); + + { + auto value = request[CelValue::CreateString(Undefined)]; + EXPECT_FALSE(value.has_value()); + } + + { + auto value = request[CelValue::CreateString(Scheme)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("http", value.value().StringOrDie().value()); + } + { + auto value = request[CelValue::CreateString(Host)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("kittens.com", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(Path)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("/meow", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(Method)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("POST", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(Referer)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("dogs.com", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(UserAgent)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("envoy-mobile", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(ID)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("blah", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(Size)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(10, value.value().Int64OrDie()); + } + + { + auto value = request[CelValue::CreateString(TotalSize)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + // this includes the headers size + EXPECT_EQ(132, value.value().Int64OrDie()); + } + + { + auto value = request[CelValue::CreateString(Time)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsTimestamp()); + EXPECT_EQ("2018-04-03T23:06:09.123+00:00", absl::FormatTime(value.value().TimestampOrDie())); + } + + { + auto value = request[CelValue::CreateString(Headers)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsMap()); + auto& map = *value.value().MapOrDie(); + EXPECT_FALSE(map.empty()); + EXPECT_EQ(8, map.size()); + + auto header = map[CelValue::CreateString(Referer)]; + EXPECT_TRUE(header.has_value()); + ASSERT_TRUE(header.value().IsString()); + EXPECT_EQ("dogs.com", header.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(Duration)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsDuration()); + EXPECT_EQ("15ms", absl::FormatDuration(value.value().DurationOrDie())); + } +} + +TEST(Context, ResponseAttributes) { + NiceMock info; + ResponseWrapper response(info); + + EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404)); + EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123)); + + { + auto value = response[CelValue::CreateString(Undefined)]; + EXPECT_FALSE(value.has_value()); + } + + { + auto value = response[CelValue::CreateString(Size)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(123, value.value().Int64OrDie()); + } + + { + auto value = response[CelValue::CreateString(Code)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(404, value.value().Int64OrDie()); + } +} + +TEST(Context, ConnectionAttributes) { + NiceMock info; + std::shared_ptr> host( + new NiceMock()); + NiceMock connection_info; + ConnectionWrapper connection(info); + + Network::Address::InstanceConstSharedPtr local = + Network::Utility::parseInternetAddress("1.2.3.4", 123, false); + Network::Address::InstanceConstSharedPtr remote = + Network::Utility::parseInternetAddress("10.20.30.40", 456, false); + Network::Address::InstanceConstSharedPtr upstream = + Network::Utility::parseInternetAddress("10.1.2.3", 679, false); + EXPECT_CALL(info, downstreamLocalAddress()).WillRepeatedly(ReturnRef(local)); + EXPECT_CALL(info, downstreamRemoteAddress()).WillRepeatedly(ReturnRef(remote)); + EXPECT_CALL(info, downstreamSslConnection()).WillRepeatedly(Return(&connection_info)); + EXPECT_CALL(info, upstreamHost()).WillRepeatedly(Return(host)); + EXPECT_CALL(connection_info, peerCertificatePresented()).WillRepeatedly(Return(true)); + EXPECT_CALL(*host, address()).WillRepeatedly(Return(upstream)); + + { + auto value = connection[CelValue::CreateString(Undefined)]; + EXPECT_FALSE(value.has_value()); + } + + { + auto value = connection[CelValue::CreateString(LocalAddress)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("1.2.3.4:123", value.value().StringOrDie().value()); + } + + { + auto value = connection[CelValue::CreateString(LocalPort)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(123, value.value().Int64OrDie()); + } + + { + auto value = connection[CelValue::CreateString(RemoteAddress)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("10.20.30.40:456", value.value().StringOrDie().value()); + } + + { + auto value = connection[CelValue::CreateString(RemotePort)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(456, value.value().Int64OrDie()); + } + + { + auto value = connection[CelValue::CreateString(UpstreamAddress)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("10.1.2.3:679", value.value().StringOrDie().value()); + } + + { + auto value = connection[CelValue::CreateString(UpstreamPort)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(679, value.value().Int64OrDie()); + } + + { + auto value = connection[CelValue::CreateString(MTLS)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsBool()); + EXPECT_TRUE(value.value().BoolOrDie()); + } +} + +} // namespace +} // namespace Expr +} // namespace Common +} // namespace Filters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index f510d59790d7e..f1713e3306e69 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/ssl/mocks.h" #include "test/mocks/stream_info/mocks.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -82,6 +83,81 @@ TEST(RoleBasedAccessControlEngineImpl, DeniedBlacklist) { checkEngine(engine, true, conn); } +TEST(RoleBasedAccessControlEngineImpl, BasicCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + const_expr: + bool_value: false + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + checkEngine(engine, false); +} + +TEST(RoleBasedAccessControlEngineImpl, HeaderCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + call_expr: + function: _==_ + args: + - call_expr: + function: _[_] + args: + - select_expr: + operand: + ident_expr: + name: request + field: headers + - const_expr: + string_value: foo + - const_expr: + string_value: bar + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + + Envoy::Http::HeaderMapImpl headers; + Envoy::Http::LowerCaseString key("foo"); + std::string value = "bar"; + headers.setReference(key, value); + + checkEngine(engine, true, Envoy::Network::MockConnection(), headers); +} + +TEST(RoleBasedAccessControlEngineImpl, ConjunctiveCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_destination_port(123); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + const_expr: + bool_value: false + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + + Envoy::Network::MockConnection conn; + Envoy::Network::Address::InstanceConstSharedPtr addr = + Envoy::Network::Utility::parseInternetAddress("1.2.3.4", 123, false); + EXPECT_CALL(conn, localAddress()).WillOnce(ReturnRef(addr)); + checkEngine(engine, false, conn); +} + } // namespace } // namespace RBAC } // namespace Common From 9393ca923c419fb7a0ad7db24774bb066b6a41e1 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 16:25:19 -0700 Subject: [PATCH 18/32] fix api Signed-off-by: Kuat Yessenov --- api/bazel/api_build_system.bzl | 7 +++--- api/envoy/config/filter/http/rbac/v2/BUILD | 1 - api/envoy/config/filter/network/rbac/v2/BUILD | 1 - api/envoy/config/rbac/v2/BUILD | 25 +++++++++++-------- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl index 9eb8e434a5319..d9a3e2d943e63 100644 --- a/api/bazel/api_build_system.bzl +++ b/api/bazel/api_build_system.bzl @@ -23,13 +23,13 @@ def _LibrarySuffix(library_name, suffix): # TODO(htuch): Convert this to native py_proto_library once # https://github.com/bazelbuild/bazel/issues/3935 and/or # https://github.com/bazelbuild/bazel/issues/2626 are resolved. -def api_py_proto_library(name, srcs = [], deps = [], has_services = 0): +def api_py_proto_library(name, srcs = [], deps = [], external_py_proto_deps = [], has_services = 0): _py_proto_library( name = _Suffix(name, _PY_SUFFIX), srcs = srcs, default_runtime = "@com_google_protobuf//:protobuf_python", protoc = "@com_google_protobuf//:protoc", - deps = [_LibrarySuffix(d, _PY_SUFFIX) for d in deps] + [ + deps = [_LibrarySuffix(d, _PY_SUFFIX) for d in deps] + external_py_proto_deps + [ "@com_envoyproxy_protoc_gen_validate//validate:validate_py", "@com_google_googleapis//google/rpc:status_py_proto", "@com_google_googleapis//google/api:annotations_py_proto", @@ -116,6 +116,7 @@ def api_proto_library( deps = [], external_proto_deps = [], external_cc_proto_deps = [], + external_py_proto_deps = [], has_services = 0, linkstatic = None, require_py = 1): @@ -152,7 +153,7 @@ def api_proto_library( ) py_export_suffixes = [] if (require_py == 1): - api_py_proto_library(name, srcs, deps, has_services) + api_py_proto_library(name, srcs, deps, external_py_proto_deps, has_services) py_export_suffixes = ["_py", "_py_genproto"] # Allow unlimited visibility for consumers diff --git a/api/envoy/config/filter/http/rbac/v2/BUILD b/api/envoy/config/filter/http/rbac/v2/BUILD index 5edeb6fcdb996..6182fe26748a0 100644 --- a/api/envoy/config/filter/http/rbac/v2/BUILD +++ b/api/envoy/config/filter/http/rbac/v2/BUILD @@ -5,6 +5,5 @@ licenses(["notice"]) # Apache 2 api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], - require_py = False, deps = ["//envoy/config/rbac/v2:rbac"], ) diff --git a/api/envoy/config/filter/network/rbac/v2/BUILD b/api/envoy/config/filter/network/rbac/v2/BUILD index 5edeb6fcdb996..6182fe26748a0 100644 --- a/api/envoy/config/filter/network/rbac/v2/BUILD +++ b/api/envoy/config/filter/network/rbac/v2/BUILD @@ -5,6 +5,5 @@ licenses(["notice"]) # Apache 2 api_proto_library_internal( name = "rbac", srcs = ["rbac.proto"], - require_py = False, deps = ["//envoy/config/rbac/v2:rbac"], ) diff --git a/api/envoy/config/rbac/v2/BUILD b/api/envoy/config/rbac/v2/BUILD index c05ea950612f9..fac50eb66f9b0 100644 --- a/api/envoy/config/rbac/v2/BUILD +++ b/api/envoy/config/rbac/v2/BUILD @@ -11,7 +11,9 @@ api_proto_library_internal( external_proto_deps = [ "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto", ], - require_py = False, + external_py_proto_deps = [ + "@com_google_googleapis//google/api/expr/v1alpha1:syntax_py_proto", + ], visibility = ["//visibility:public"], deps = [ "//envoy/api/v2/core:address", @@ -21,13 +23,14 @@ api_proto_library_internal( ], ) -#api_go_proto_library( -# name = "rbac", -# proto = ":rbac", -# deps = [ -# "//envoy/api/v2/core:address_go_proto", -# "//envoy/api/v2/route:route_go_proto", -# "//envoy/type/matcher:metadata_go_proto", -# "//envoy/type/matcher:string_go_proto", -# ], -#) +api_go_proto_library( + name = "rbac", + proto = ":rbac", + deps = [ + "//envoy/api/v2/core:address_go_proto", + "//envoy/api/v2/route:route_go_proto", + "//envoy/type/matcher:metadata_go_proto", + "//envoy/type/matcher:string_go_proto", + "@com_google_googleapis//google/api/expr/v1alpha1:cel_go_proto", + ], +) From 5d44fea8173375105a933df38275d3aedcfb300b Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 16:33:18 -0700 Subject: [PATCH 19/32] add metadata test Signed-off-by: Kuat Yessenov --- .../filters/common/rbac/engine_impl_test.cc | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index f1713e3306e69..69b8431f688c2 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -136,6 +136,49 @@ TEST(RoleBasedAccessControlEngineImpl, HeaderCondition) { checkEngine(engine, true, Envoy::Network::MockConnection(), headers); } +TEST(RoleBasedAccessControlEngineImpl, MetadataCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + call_expr: + function: _==_ + args: + - call_expr: + function: _[_] + args: + - call_expr: + function: _[_] + args: + - select_expr: + operand: + ident_expr: + name: metadata + field: filter_metadata + - const_expr: + string_value: other + - const_expr: + string_value: label + - const_expr: + string_value: prod + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + + Envoy::Http::HeaderMapImpl headers; + + auto label = MessageUtil::keyValueStruct("label", "prod"); + envoy::api::v2::core::Metadata metadata; + metadata.mutable_filter_metadata()->insert( + Protobuf::MapPair("other", label)); + + checkEngine(engine, true, Envoy::Network::MockConnection(), headers, metadata); +} + TEST(RoleBasedAccessControlEngineImpl, ConjunctiveCondition) { envoy::config::rbac::v2::Policy policy; policy.add_permissions()->set_destination_port(123); From 784a970d88fc8b30620bc599d965ff3c27592cf4 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 19:36:35 -0700 Subject: [PATCH 20/32] release note Signed-off-by: Kuat Yessenov --- docs/root/intro/version_history.rst | 1 + .../extensions/filters/common/expr/context.cc | 9 +++++++++ .../extensions/filters/common/expr/context.h | 2 ++ .../extensions/filters/common/rbac/matchers.h | 2 +- .../filters/common/expr/context_test.cc | 20 +++++++++++++++++-- 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 758247d95cf11..b769dbf5328ec 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -16,6 +16,7 @@ Version history * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. +* rbac: added conditions to the policy, see :ref:`requested server name `. 1.11.0 (July 11, 2019) ====================== diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index b55f844b117f4..78533efb4115e 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -35,6 +35,13 @@ absl::optional RequestWrapper::operator[](CelValue key) const { auto value = key.StringOrDie().value(); if (value == Path) { return convertHeaderEntry(wrapper_.headers_.Path()); + } else if (value == UrlPath) { + absl::string_view path = wrapper_.headers_.Path()->value().getStringView(); + size_t query_offset = path.find('?'); + if (query_offset == absl::string_view::npos) { + return CelValue::CreateString(path); + } + return CelValue::CreateString(path.substr(0, query_offset)); } else if (value == Host) { return convertHeaderEntry(wrapper_.headers_.Host()); } else if (value == Scheme) { @@ -119,6 +126,8 @@ absl::optional ConnectionWrapper::operator[](CelValue key) const { } else if (value == MTLS) { return CelValue::CreateBool(info_.downstreamSslConnection() != nullptr && info_.downstreamSslConnection()->peerCertificatePresented()); + } else if (value == RequestedServerName) { + return CelValue::CreateString(info_.requestedServerName()); } return {}; diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 3cf920838d241..229bcdaf83134 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -17,6 +17,7 @@ using CelValue = google::api::expr::runtime::CelValue; // Symbols for traversing the request properties constexpr absl::string_view Request = "request"; constexpr absl::string_view Path = "path"; +constexpr absl::string_view UrlPath = "url_path"; constexpr absl::string_view Host = "host"; constexpr absl::string_view Scheme = "scheme"; constexpr absl::string_view Method = "method"; @@ -45,6 +46,7 @@ constexpr absl::string_view RemotePort = "remote_port"; constexpr absl::string_view UpstreamAddress = "upstream_address"; constexpr absl::string_view UpstreamPort = "upstream_port"; constexpr absl::string_view MTLS = "mtls"; +constexpr absl::string_view RequestedServerName = "requested_server_name"; class RequestWrapper; diff --git a/source/extensions/filters/common/rbac/matchers.h b/source/extensions/filters/common/rbac/matchers.h index 28796763631b2..98f369d49e20b 100644 --- a/source/extensions/filters/common/rbac/matchers.h +++ b/source/extensions/filters/common/rbac/matchers.h @@ -181,7 +181,7 @@ class AuthenticatedMatcher : public Matcher { * matches a permission, the principals are then checked for a match. * The condition is a conjunction clause. */ -class PolicyMatcher : public Matcher { +class PolicyMatcher : public Matcher, NonCopyable { public: PolicyMatcher(const envoy::config::rbac::v2::Policy& policy, Expr::Builder* builder) : permissions_(policy.permissions()), principals_(policy.principals()), diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index 8a559dcf37bef..3242b11a7ac45 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -27,7 +27,7 @@ constexpr absl::string_view Undefined = "undefined"; TEST(Context, RequestAttributes) { NiceMock info; Http::TestHeaderMapImpl header_map{ - {":method", "POST"}, {":scheme", "http"}, {":path", "/meow"}, + {":method", "POST"}, {":scheme", "http"}, {":path", "/meow?yes=1"}, {":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"}, {"content-length", "10"}, {"x-request-id", "blah"}, }; @@ -62,6 +62,13 @@ TEST(Context, RequestAttributes) { auto value = request[CelValue::CreateString(Path)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("/meow?yes=1", value.value().StringOrDie().value()); + } + + { + auto value = request[CelValue::CreateString(UrlPath)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); EXPECT_EQ("/meow", value.value().StringOrDie().value()); } @@ -105,7 +112,7 @@ TEST(Context, RequestAttributes) { EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); // this includes the headers size - EXPECT_EQ(132, value.value().Int64OrDie()); + EXPECT_EQ(138, value.value().Int64OrDie()); } { @@ -177,10 +184,12 @@ TEST(Context, ConnectionAttributes) { Network::Utility::parseInternetAddress("10.20.30.40", 456, false); Network::Address::InstanceConstSharedPtr upstream = Network::Utility::parseInternetAddress("10.1.2.3", 679, false); + const std::string sni_name = "kittens.com"; EXPECT_CALL(info, downstreamLocalAddress()).WillRepeatedly(ReturnRef(local)); EXPECT_CALL(info, downstreamRemoteAddress()).WillRepeatedly(ReturnRef(remote)); EXPECT_CALL(info, downstreamSslConnection()).WillRepeatedly(Return(&connection_info)); EXPECT_CALL(info, upstreamHost()).WillRepeatedly(Return(host)); + EXPECT_CALL(info, requestedServerName()).WillRepeatedly(ReturnRef(sni_name)); EXPECT_CALL(connection_info, peerCertificatePresented()).WillRepeatedly(Return(true)); EXPECT_CALL(*host, address()).WillRepeatedly(Return(upstream)); @@ -237,6 +246,13 @@ TEST(Context, ConnectionAttributes) { ASSERT_TRUE(value.value().IsBool()); EXPECT_TRUE(value.value().BoolOrDie()); } + + { + auto value = connection[CelValue::CreateString(RequestedServerName)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ(sni_name, value.value().StringOrDie().value()); + } } } // namespace From c081695dabcdc6477f7d0cd0deb184a7c7600863 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 31 Jul 2019 19:37:35 -0700 Subject: [PATCH 21/32] typo Signed-off-by: Kuat Yessenov --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index b769dbf5328ec..f9758d5eb7c27 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -16,7 +16,7 @@ Version history * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. -* rbac: added conditions to the policy, see :ref:`requested server name `. +* rbac: added conditions to the policy, see :ref:`condition `. 1.11.0 (July 11, 2019) ====================== From 184fe6ba48e51c68baf9e50d28ae4b77d1a9eae2 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 1 Aug 2019 18:53:47 -0700 Subject: [PATCH 22/32] add constant folding; use unique_ptr to avoid copying the engine Signed-off-by: Kuat Yessenov --- .../filters/common/expr/evaluator.cc | 7 ++++++- .../filters/common/expr/evaluator.h | 5 +++-- .../filters/common/rbac/engine_impl.cc | 2 +- .../filters/common/rbac/engine_impl.h | 3 ++- .../extensions/filters/common/rbac/utility.h | 12 +++++------ .../filters/http/rbac/rbac_filter.cc | 10 +++++----- .../filters/http/rbac/rbac_filter.h | 20 ++++++++++--------- .../filters/network/rbac/rbac_filter.cc | 4 ++-- .../filters/network/rbac/rbac_filter.h | 9 +++++---- 9 files changed, 41 insertions(+), 31 deletions(-) diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index 8649e7046e488..c66d725c120ad 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -11,12 +11,17 @@ namespace Filters { namespace Common { namespace Expr { -BuilderPtr createBuilder() { +BuilderPtr createBuilder(Protobuf::Arena* arena) { google::api::expr::runtime::InterpreterOptions options; // Conformance with spec/go runtimes requires this setting options.partial_string_match = true; + if (arena != nullptr) { + options.constant_folding = true; + options.constant_arena = arena; + } + auto builder = google::api::expr::runtime::CreateCelExpressionBuilder(options); auto register_status = google::api::expr::runtime::RegisterBuiltinFunctions(builder->GetRegistry()); diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 24d098109d68b..8f3b98583f1fb 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -21,9 +21,10 @@ using BuilderPtr = std::unique_ptr; using Expression = google::api::expr::runtime::CelExpression; using ExpressionPtr = std::unique_ptr; -// Creates an expression builder. +// Creates an expression builder. The optional arena is used to enable constant folding +// for intermediate evaluation results. // Throws an exception if fails to construct an expression builder. -BuilderPtr createBuilder(); +BuilderPtr createBuilder(Protobuf::Arena* arena); // Creates an interpretable expression from a protobuf representation. // Throws an exception if fails to construct a runtime expression. diff --git a/source/extensions/filters/common/rbac/engine_impl.cc b/source/extensions/filters/common/rbac/engine_impl.cc index 540d44c6c9639..ffe8039c81388 100644 --- a/source/extensions/filters/common/rbac/engine_impl.cc +++ b/source/extensions/filters/common/rbac/engine_impl.cc @@ -15,7 +15,7 @@ RoleBasedAccessControlEngineImpl::RoleBasedAccessControlEngineImpl( // guard expression builder by presence of a condition in policies for (const auto& policy : rules.policies()) { if (policy.second.has_condition()) { - builder_ = Expr::createBuilder(); + builder_ = Expr::createBuilder(&constant_arena_); break; } } diff --git a/source/extensions/filters/common/rbac/engine_impl.h b/source/extensions/filters/common/rbac/engine_impl.h index 91d5a36c720ae..43b71fe5d8b03 100644 --- a/source/extensions/filters/common/rbac/engine_impl.h +++ b/source/extensions/filters/common/rbac/engine_impl.h @@ -11,7 +11,7 @@ namespace Filters { namespace Common { namespace RBAC { -class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { +class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine, NonCopyable { public: RoleBasedAccessControlEngineImpl(const envoy::config::rbac::v2::RBAC& rules); @@ -26,6 +26,7 @@ class RoleBasedAccessControlEngineImpl : public RoleBasedAccessControlEngine { std::map> policies_; + Protobuf::Arena constant_arena_; Expr::BuilderPtr builder_; }; diff --git a/source/extensions/filters/common/rbac/utility.h b/source/extensions/filters/common/rbac/utility.h index bcf934f41feb1..684c4204ecb6f 100644 --- a/source/extensions/filters/common/rbac/utility.h +++ b/source/extensions/filters/common/rbac/utility.h @@ -47,16 +47,16 @@ RoleBasedAccessControlFilterStats generateStats(const std::string& prefix, Stats enum class EnforcementMode { Enforced, Shadow }; template -absl::optional createEngine(const ConfigType& config) { - return config.has_rules() ? absl::make_optional(config.rules()) - : absl::nullopt; +std::unique_ptr createEngine(const ConfigType& config) { + return config.has_rules() ? std::make_unique(config.rules()) + : nullptr; } template -absl::optional createShadowEngine(const ConfigType& config) { +std::unique_ptr createShadowEngine(const ConfigType& config) { return config.has_shadow_rules() - ? absl::make_optional(config.shadow_rules()) - : absl::nullopt; + ? std::make_unique(config.shadow_rules()) + : nullptr; } } // namespace RBAC diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index ec3dff972d4d5..98e6db79b5527 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -26,7 +26,7 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( engine_(Filters::Common::RBAC::createEngine(proto_config)), shadow_engine_(Filters::Common::RBAC::createShadowEngine(proto_config)) {} -const absl::optional& +const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* RoleBasedAccessControlFilterConfig::engine(const Router::RouteConstSharedPtr route, Filters::Common::RBAC::EnforcementMode mode) const { if (!route || !route->routeEntry()) { @@ -70,10 +70,10 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head headers, callbacks_->streamInfo().dynamicMetadata().DebugString()); std::string effective_policy_id; - const auto& shadow_engine = + const auto shadow_engine = config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Shadow); - if (shadow_engine.has_value()) { + if (shadow_engine != nullptr) { std::string shadow_resp_code = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; if (shadow_engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), @@ -102,9 +102,9 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().Rbac, metrics); } - const auto& engine = + const auto engine = config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Enforced); - if (engine.has_value()) { + if (engine != nullptr) { if (engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index 5397a3c31c586..eafd602537aee 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -22,14 +22,15 @@ class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpec RoleBasedAccessControlRouteSpecificFilterConfig( const envoy::config::filter::http::rbac::v2::RBACPerRoute& per_route_config); - const absl::optional& + const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* engine(Filters::Common::RBAC::EnforcementMode mode) const { - return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_ : shadow_engine_; + return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_.get() + : shadow_engine_.get(); } private: - const absl::optional engine_; - const absl::optional shadow_engine_; + std::unique_ptr engine_; + std::unique_ptr shadow_engine_; }; /** @@ -43,20 +44,21 @@ class RoleBasedAccessControlFilterConfig { Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; } - const absl::optional& + const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* engine(const Router::RouteConstSharedPtr route, Filters::Common::RBAC::EnforcementMode mode) const; private: - const absl::optional& + const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* engine(Filters::Common::RBAC::EnforcementMode mode) const { - return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_ : shadow_engine_; + return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_.get() + : shadow_engine_.get(); } Filters::Common::RBAC::RoleBasedAccessControlFilterStats stats_; - const absl::optional engine_; - const absl::optional shadow_engine_; + std::unique_ptr engine_; + std::unique_ptr shadow_engine_; }; using RoleBasedAccessControlFilterConfigSharedPtr = diff --git a/source/extensions/filters/network/rbac/rbac_filter.cc b/source/extensions/filters/network/rbac/rbac_filter.cc index a6ab38df16238..9bc369f246dcd 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.cc +++ b/source/extensions/filters/network/rbac/rbac_filter.cc @@ -77,8 +77,8 @@ void RoleBasedAccessControlFilter::setDynamicMetadata(std::string shadow_engine_ EngineResult RoleBasedAccessControlFilter::checkEngine(Filters::Common::RBAC::EnforcementMode mode) { - const auto& engine = config_->engine(mode); - if (engine.has_value()) { + const auto engine = config_->engine(mode); + if (engine != nullptr) { std::string effective_policy_id; if (engine->allowed(callbacks_->connection(), callbacks_->connection().streamInfo(), &effective_policy_id)) { diff --git a/source/extensions/filters/network/rbac/rbac_filter.h b/source/extensions/filters/network/rbac/rbac_filter.h index b548424c63de0..a42214004c5c6 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.h +++ b/source/extensions/filters/network/rbac/rbac_filter.h @@ -26,9 +26,10 @@ class RoleBasedAccessControlFilterConfig { Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; } - const absl::optional& + const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* engine(Filters::Common::RBAC::EnforcementMode mode) const { - return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_ : shadow_engine_; + return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_.get() + : shadow_engine_.get(); } envoy::config::filter::network::rbac::v2::RBAC::EnforcementType enforcementType() const { @@ -38,8 +39,8 @@ class RoleBasedAccessControlFilterConfig { private: Filters::Common::RBAC::RoleBasedAccessControlFilterStats stats_; - const absl::optional engine_; - const absl::optional shadow_engine_; + std::unique_ptr engine_; + std::unique_ptr shadow_engine_; const envoy::config::filter::network::rbac::v2::RBAC::EnforcementType enforcement_type_; }; From 684d473c86a6befa3f865fbe784bf0528d8c6315 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 5 Aug 2019 19:05:19 -0700 Subject: [PATCH 23/32] apply a patch for gcc Signed-off-by: Kuat Yessenov --- bazel/com_google_cel_cpp.patch | 67 ++++++++++++++++++++++++++++++++++ bazel/repositories.bzl | 12 +++++- 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 bazel/com_google_cel_cpp.patch diff --git a/bazel/com_google_cel_cpp.patch b/bazel/com_google_cel_cpp.patch new file mode 100644 index 0000000000000..5db317116f502 --- /dev/null +++ b/bazel/com_google_cel_cpp.patch @@ -0,0 +1,67 @@ +diff --git a/eval/public/cel_function_adapter.h b/eval/public/cel_function_adapter.h +index d99239c..4dc8cae 100644 +--- a/eval/public/cel_function_adapter.h ++++ b/eval/public/cel_function_adapter.h +@@ -118,6 +118,34 @@ class FunctionAdapter : public CelFunction { + return registry->Register(std::move(status.ValueOrDie())); + } + ++#if defined(__GNUC__) || defined(__GNUG__) ++ inline cel_base::Status RunWrap(std::function func, ++ const absl::Span argset, ++ ::google::protobuf::Arena* arena, CelValue* result, ++ int arg_index) const { ++ return CreateReturnValue(func(), arena, result); ++ } ++ ++ template ++ inline cel_base::Status RunWrap(std::function func, ++ const absl::Span argset, ++ ::google::protobuf::Arena* arena, CelValue* result, ++ int arg_index) const { ++ Arg argument; ++ if (!ConvertFromValue(argset[arg_index], &argument)) { ++ return cel_base::Status(cel_base::StatusCode::kInvalidArgument, ++ "Type conversion failed"); ++ } ++ ++ std::function wrapped_func = ++ [func, argument](Args... args) -> ReturnType { ++ return func(argument, args...); ++ }; ++ ++ return RunWrap(std::move(wrapped_func), argset, arena, result, ++ arg_index + 1); ++ } ++#else + template + inline cel_base::Status RunWrap(absl::Span arguments, + std::tuple<::google::protobuf::Arena*, Arguments...> input, +@@ -137,6 +165,7 @@ class FunctionAdapter : public CelFunction { + ::google::protobuf::Arena* arena) const { + return CreateReturnValue(absl::apply(handler_, input), arena, result); + } ++#endif + + ::cel_base::Status Evaluate(absl::Span arguments, + CelValue* result, +@@ -146,9 +175,19 @@ class FunctionAdapter : public CelFunction { + "Argument number mismatch"); + } + ++ ++#if defined(__GNUC__) || defined(__GNUG__) ++ const auto* handler = &handler_; ++ std::function wrapped_handler = ++ [handler, arena](Arguments... args) -> ReturnType { ++ return (*handler)(arena, args...); ++ }; ++ return RunWrap(std::move(wrapped_handler), arguments, arena, result, 0); ++#else + std::tuple<::google::protobuf::Arena*, Arguments...> input; + std::get<0>(input) = arena; + return RunWrap<0>(arguments, input, result, arena); ++#endif + } + + private: diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 0b55cfb12dcf3..0052d00ee7ef6 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -154,7 +154,7 @@ def envoy_dependencies(skip_targets = []): _io_opentracing_cpp() _net_zlib() _repository_impl("com_google_re2") - _repository_impl("com_google_cel_cpp") + _com_google_cel_cpp() _repository_impl("bazel_toolchains") _python_deps() @@ -317,6 +317,16 @@ def _net_zlib(): actual = "@envoy//bazel/foreign_cc:zlib", ) +def _com_google_cel_cpp(): + location = REPOSITORY_LOCATIONS["com_google_cel_cpp"] + http_archive( + name = "com_google_cel_cpp", + # TODO(kyessenov): requires C++17 partial template specialization available in clang-8 but not gcc + patch_args = ["-p1"], + patches = ["@envoy//bazel:com_google_cel_cpp.patch"], + **location + ) + def _com_github_nghttp2_nghttp2(): location = REPOSITORY_LOCATIONS["com_github_nghttp2_nghttp2"] http_archive( From 08fe702a90d28d4f1723084bf1a4eae42144f830 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Mon, 5 Aug 2019 19:14:22 -0700 Subject: [PATCH 24/32] more specific patch Signed-off-by: Kuat Yessenov --- bazel/com_google_cel_cpp.patch | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/com_google_cel_cpp.patch b/bazel/com_google_cel_cpp.patch index 5db317116f502..c7491ec226d4d 100644 --- a/bazel/com_google_cel_cpp.patch +++ b/bazel/com_google_cel_cpp.patch @@ -6,7 +6,7 @@ index d99239c..4dc8cae 100644 return registry->Register(std::move(status.ValueOrDie())); } -+#if defined(__GNUC__) || defined(__GNUG__) ++#if defined(__GNUC__) && !defined(__clang__) + inline cel_base::Status RunWrap(std::function func, + const absl::Span argset, + ::google::protobuf::Arena* arena, CelValue* result, @@ -50,7 +50,7 @@ index d99239c..4dc8cae 100644 } + -+#if defined(__GNUC__) || defined(__GNUG__) ++#if defined(__GNUC__) && !defined(__clang__) + const auto* handler = &handler_; + std::function wrapped_handler = + [handler, arena](Arguments... args) -> ReturnType { From 580a79fe757e3f87504ded5e516ae599801c5c9b Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 7 Aug 2019 15:30:49 -0700 Subject: [PATCH 25/32] fix the macro specializer Signed-off-by: Kuat Yessenov --- bazel/com_google_cel_cpp.patch | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/com_google_cel_cpp.patch b/bazel/com_google_cel_cpp.patch index c7491ec226d4d..49d97d663d104 100644 --- a/bazel/com_google_cel_cpp.patch +++ b/bazel/com_google_cel_cpp.patch @@ -6,7 +6,7 @@ index d99239c..4dc8cae 100644 return registry->Register(std::move(status.ValueOrDie())); } -+#if defined(__GNUC__) && !defined(__clang__) ++#if defined(__clang_major_version__) && __clang_major_version__ >= 8 && !defined(__APPLE__) + inline cel_base::Status RunWrap(std::function func, + const absl::Span argset, + ::google::protobuf::Arena* arena, CelValue* result, @@ -50,7 +50,7 @@ index d99239c..4dc8cae 100644 } + -+#if defined(__GNUC__) && !defined(__clang__) ++#if defined(__clang_major_version__) && __clang_major_version__ >= 8 && !defined(__APPLE__) + const auto* handler = &handler_; + std::function wrapped_handler = + [handler, arena](Arguments... args) -> ReturnType { From 8f72a50f694e126078ef75bde74a036c2904230b Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 7 Aug 2019 18:44:14 -0700 Subject: [PATCH 26/32] oops, reverse the patch Signed-off-by: Kuat Yessenov --- bazel/com_google_cel_cpp.patch | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/com_google_cel_cpp.patch b/bazel/com_google_cel_cpp.patch index 49d97d663d104..173a0545cf0d4 100644 --- a/bazel/com_google_cel_cpp.patch +++ b/bazel/com_google_cel_cpp.patch @@ -6,7 +6,7 @@ index d99239c..4dc8cae 100644 return registry->Register(std::move(status.ValueOrDie())); } -+#if defined(__clang_major_version__) && __clang_major_version__ >= 8 && !defined(__APPLE__) ++#if !defined(__clang_major_version__) || (defined(__clang_major_version__) && __clang_major_version__ < 8) || defined(__APPLE__) + inline cel_base::Status RunWrap(std::function func, + const absl::Span argset, + ::google::protobuf::Arena* arena, CelValue* result, @@ -50,7 +50,7 @@ index d99239c..4dc8cae 100644 } + -+#if defined(__clang_major_version__) && __clang_major_version__ >= 8 && !defined(__APPLE__) ++#if !defined(__clang_major_version__) || (defined(__clang_major_version__) && __clang_major_version__ < 8) || defined(__APPLE__) + const auto* handler = &handler_; + std::function wrapped_handler = + [handler, arena](Arguments... args) -> ReturnType { From f86eadfd9e8ad125edfa2633aa8d1e74375a14e0 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 8 Aug 2019 15:25:58 -0700 Subject: [PATCH 27/32] update re2 import Signed-off-by: Kuat Yessenov --- bazel/repositories.bzl | 2 +- bazel/repository_locations.bzl | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 0052d00ee7ef6..0972ea3e0943d 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -153,7 +153,7 @@ def envoy_dependencies(skip_targets = []): _com_lightstep_tracer_cpp() _io_opentracing_cpp() _net_zlib() - _repository_impl("com_google_re2") + _repository_impl("com_googlesource_code_re2") _com_google_cel_cpp() _repository_impl("bazel_toolchains") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 3aa2e9c05dacf..89142ccdfa708 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -249,11 +249,11 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://storage.googleapis.com/quiche-envoy-integration/2a930469533c3b541443488a629fe25cd8ff53d0.tar.gz"], ), com_google_cel_cpp = dict( - sha256 = "a0e6a6ccf25c1e57ba3c7a997edd9279f8e0b0112e0a3ac705a5568fa32792fc", - strip_prefix = "cel-cpp-d56f26adb53d0f41508a909d15e74d9ffb0e8a6c", - urls = ["https://github.com/google/cel-cpp/archive/d56f26adb53d0f41508a909d15e74d9ffb0e8a6c.tar.gz"], + sha256 = "82186be314a2a9c6b9eb2477f15c4f3704b5ac9b4b26bf65694e231a48f4c1f1", + strip_prefix = "cel-cpp-71fb0562a59c05239f92025d3e7beb63169c3923", + urls = ["https://github.com/google/cel-cpp/archive/71fb0562a59c05239f92025d3e7beb63169c3923.tar.gz"], ), - com_google_re2 = dict( + com_googlesource_code_re2 = dict( sha256 = "f31db9cd224d018a7e4fe88ef84aaa874b0b3ed91d4d98ee5a1531101d3fdc64", strip_prefix = "re2-87e2ad45e7b18738e1551474f7ee5886ff572059", urls = ["https://github.com/google/re2/archive/87e2ad45e7b18738e1551474f7ee5886ff572059.tar.gz"], From 532ed674ef3f2b853b10aadc0db28424705ab04e Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 8 Aug 2019 23:23:41 -0700 Subject: [PATCH 28/32] align with ext_authz by using source and destination Signed-off-by: Kuat Yessenov --- .../extensions/filters/common/expr/context.cc | 111 +++++++++++------- .../extensions/filters/common/expr/context.h | 43 +++++-- .../filters/common/expr/evaluator.cc | 14 ++- .../filters/common/expr/evaluator.h | 4 +- .../filters/http/rbac/rbac_filter.h | 4 +- .../filters/common/expr/context_test.cc | 14 ++- 6 files changed, 121 insertions(+), 69 deletions(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 78533efb4115e..a8867a8d95f2e 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -24,7 +24,7 @@ absl::optional HeadersWrapper::operator[](CelValue key) const { if (!key.IsString()) { return {}; } - auto out = headers_.get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); + auto out = value_->get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); return convertHeaderEntry(out); } @@ -33,49 +33,58 @@ absl::optional RequestWrapper::operator[](CelValue key) const { return {}; } auto value = key.StringOrDie().value(); - if (value == Path) { - return convertHeaderEntry(wrapper_.headers_.Path()); - } else if (value == UrlPath) { - absl::string_view path = wrapper_.headers_.Path()->value().getStringView(); - size_t query_offset = path.find('?'); - if (query_offset == absl::string_view::npos) { - return CelValue::CreateString(path); - } - return CelValue::CreateString(path.substr(0, query_offset)); - } else if (value == Host) { - return convertHeaderEntry(wrapper_.headers_.Host()); - } else if (value == Scheme) { - return convertHeaderEntry(wrapper_.headers_.Scheme()); - } else if (value == Method) { - return convertHeaderEntry(wrapper_.headers_.Method()); - } else if (value == Referer) { - return convertHeaderEntry(wrapper_.headers_.Referer()); - } else if (value == Headers) { - return CelValue::CreateMap(&wrapper_); + + if (value == Headers) { + return CelValue::CreateMap(&headers_); } else if (value == Time) { return CelValue::CreateTimestamp(absl::FromChrono(info_.startTime())); - } else if (value == ID) { - return convertHeaderEntry(wrapper_.headers_.RequestId()); - } else if (value == UserAgent) { - return convertHeaderEntry(wrapper_.headers_.UserAgent()); } else if (value == Size) { // it is important to make a choice whether to rely on content-length vs stream info // (which is not available at the time of the request headers) - auto length_header = wrapper_.headers_.ContentLength(); - if (length_header != nullptr) { - int64_t length; - if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { - return CelValue::CreateInt64(length); + if (headers_.value_ != nullptr) { + auto length_header = headers_.value_->ContentLength(); + if (length_header != nullptr) { + int64_t length; + if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { + return CelValue::CreateInt64(length); + } } + } else { + return CelValue::CreateInt64(info_.bytesReceived()); } - } else if (value == TotalSize) { - return CelValue::CreateInt64(info_.bytesReceived() + wrapper_.headers_.byteSize()); } else if (value == Duration) { auto duration = info_.requestComplete(); if (duration.has_value()) { return CelValue::CreateDuration(absl::FromChrono(duration.value())); } } + + if (headers_.value_ != nullptr) { + if (value == Path) { + return convertHeaderEntry(headers_.value_->Path()); + } else if (value == UrlPath) { + absl::string_view path = headers_.value_->Path()->value().getStringView(); + size_t query_offset = path.find('?'); + if (query_offset == absl::string_view::npos) { + return CelValue::CreateString(path); + } + return CelValue::CreateString(path.substr(0, query_offset)); + } else if (value == Host) { + return convertHeaderEntry(headers_.value_->Host()); + } else if (value == Scheme) { + return convertHeaderEntry(headers_.value_->Scheme()); + } else if (value == Method) { + return convertHeaderEntry(headers_.value_->Method()); + } else if (value == Referer) { + return convertHeaderEntry(headers_.value_->Referer()); + } else if (value == ID) { + return convertHeaderEntry(headers_.value_->RequestId()); + } else if (value == UserAgent) { + return convertHeaderEntry(headers_.value_->UserAgent()); + } else if (value == TotalSize) { + return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize()); + } + } return {}; } @@ -100,19 +109,7 @@ absl::optional ConnectionWrapper::operator[](CelValue key) const { return {}; } auto value = key.StringOrDie().value(); - if (value == LocalAddress) { - return CelValue::CreateString(info_.downstreamLocalAddress()->asStringView()); - } else if (value == LocalPort) { - if (info_.downstreamLocalAddress()->ip() != nullptr) { - return CelValue::CreateInt64(info_.downstreamLocalAddress()->ip()->port()); - } - } else if (value == RemoteAddress) { - return CelValue::CreateString(info_.downstreamRemoteAddress()->asStringView()); - } else if (value == RemotePort) { - if (info_.downstreamRemoteAddress()->ip() != nullptr) { - return CelValue::CreateInt64(info_.downstreamRemoteAddress()->ip()->port()); - } - } else if (value == UpstreamAddress) { + if (value == UpstreamAddress) { auto upstream_host = info_.upstreamHost(); if (upstream_host != nullptr && upstream_host->address() != nullptr) { return CelValue::CreateString(upstream_host->address()->asStringView()); @@ -133,6 +130,32 @@ absl::optional ConnectionWrapper::operator[](CelValue key) const { return {}; } +absl::optional PeerWrapper::operator[](CelValue key) const { + if (!key.IsString()) { + return {}; + } + auto value = key.StringOrDie().value(); + if (value == Address) { + if (local_) { + return CelValue::CreateString(info_.downstreamLocalAddress()->asStringView()); + } else { + return CelValue::CreateString(info_.downstreamRemoteAddress()->asStringView()); + } + } else if (value == Port) { + if (local_) { + if (info_.downstreamLocalAddress()->ip() != nullptr) { + return CelValue::CreateInt64(info_.downstreamLocalAddress()->ip()->port()); + } + } else { + if (info_.downstreamRemoteAddress()->ip() != nullptr) { + return CelValue::CreateInt64(info_.downstreamRemoteAddress()->ip()->port()); + } + } + } + + return {}; +} + } // namespace Expr } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 229bcdaf83134..c8c88cad6e309 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -33,34 +33,39 @@ constexpr absl::string_view Duration = "duration"; // Symbols for traversing the response properties constexpr absl::string_view Response = "response"; constexpr absl::string_view Code = "code"; +constexpr absl::string_view Trailers = "trailers"; // Per-request or per-connection metadata constexpr absl::string_view Metadata = "metadata"; // Connection properties constexpr absl::string_view Connection = "connection"; -constexpr absl::string_view LocalAddress = "local_address"; -constexpr absl::string_view LocalPort = "local_port"; -constexpr absl::string_view RemoteAddress = "remote_address"; -constexpr absl::string_view RemotePort = "remote_port"; constexpr absl::string_view UpstreamAddress = "upstream_address"; constexpr absl::string_view UpstreamPort = "upstream_port"; constexpr absl::string_view MTLS = "mtls"; constexpr absl::string_view RequestedServerName = "requested_server_name"; +// Source properties +constexpr absl::string_view Source = "source"; +constexpr absl::string_view Address = "address"; +constexpr absl::string_view Port = "port"; + +// Destination properties +constexpr absl::string_view Destination = "destination"; + class RequestWrapper; class HeadersWrapper : public google::api::expr::runtime::CelMap { public: - HeadersWrapper(const Http::HeaderMap& headers) : headers_(headers) {} + HeadersWrapper(const Http::HeaderMap* value) : value_(value) {} absl::optional operator[](CelValue key) const override; - int size() const override { return headers_.size(); } - bool empty() const override { return headers_.empty(); } + int size() const override { return value_ == nullptr ? 0 : value_->size(); } + bool empty() const override { return value_ == nullptr ? true : value_->empty(); } const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } private: friend class RequestWrapper; - const Http::HeaderMap& headers_; + const Http::HeaderMap* value_; }; class BaseWrapper : public google::api::expr::runtime::CelMap { @@ -72,21 +77,25 @@ class BaseWrapper : public google::api::expr::runtime::CelMap { class RequestWrapper : public BaseWrapper { public: - RequestWrapper(const Http::HeaderMap& headers, const StreamInfo::StreamInfo& info) - : wrapper_(headers), info_(info) {} + RequestWrapper(const Http::HeaderMap* headers, const StreamInfo::StreamInfo& info) + : headers_(headers), info_(info) {} absl::optional operator[](CelValue key) const override; private: - const HeadersWrapper wrapper_; + const HeadersWrapper headers_; const StreamInfo::StreamInfo& info_; }; class ResponseWrapper : public BaseWrapper { public: - ResponseWrapper(const StreamInfo::StreamInfo& info) : info_(info) {} + ResponseWrapper(const Http::HeaderMap* headers, const Http::HeaderMap* trailers, + const StreamInfo::StreamInfo& info) + : headers_(headers), trailers_(trailers), info_(info) {} absl::optional operator[](CelValue key) const override; private: + const HeadersWrapper headers_; + const HeadersWrapper trailers_; const StreamInfo::StreamInfo& info_; }; @@ -99,6 +108,16 @@ class ConnectionWrapper : public BaseWrapper { const StreamInfo::StreamInfo& info_; }; +class PeerWrapper : public BaseWrapper { +public: + PeerWrapper(const StreamInfo::StreamInfo& info, bool local) : info_(info), local_(local) {} + absl::optional operator[](CelValue key) const override; + +private: + const StreamInfo::StreamInfo& info_; + const bool local_; +}; + } // namespace Expr } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index c66d725c120ad..bd25da52975f8 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -44,15 +44,21 @@ ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alph absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, const StreamInfo::StreamInfo& info, - const Http::HeaderMap& headers) { + const Http::HeaderMap* request_headers, + const Http::HeaderMap* response_headers, + const Http::HeaderMap* response_trailers) { google::api::expr::runtime::Activation activation; - const RequestWrapper request(headers, info); - const ResponseWrapper response(info); + const RequestWrapper request(request_headers, info); + const ResponseWrapper response(response_headers, response_trailers, info); const ConnectionWrapper connection(info); + const PeerWrapper source(info, false); + const PeerWrapper destination(info, true); activation.InsertValue(Request, CelValue::CreateMap(&request)); activation.InsertValue(Response, CelValue::CreateMap(&response)); activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), arena)); activation.InsertValue(Connection, CelValue::CreateMap(&connection)); + activation.InsertValue(Source, CelValue::CreateMap(&source)); + activation.InsertValue(Destination, CelValue::CreateMap(&destination)); auto eval_status = expr.Evaluate(activation, arena); if (!eval_status.ok()) { @@ -65,7 +71,7 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena bool matches(const Expression& expr, const StreamInfo::StreamInfo& info, const Http::HeaderMap& headers) { Protobuf::Arena arena; - auto eval_status = Expr::evaluate(expr, &arena, info, headers); + auto eval_status = Expr::evaluate(expr, &arena, info, &headers, nullptr, nullptr); if (!eval_status.has_value()) { return false; } diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 8f3b98583f1fb..92ccea420d216 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -34,7 +34,9 @@ ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alph // results and potentially the final value. absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, const StreamInfo::StreamInfo& info, - const Http::HeaderMap& headers); + const Http::HeaderMap* request_headers, + const Http::HeaderMap* response_headers, + const Http::HeaderMap* response_trailers); // Evaluates an expression and returns true if the expression evaluates to "true". // Returns false if the expression fails to evaluate. diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index eafd602537aee..e6b0044580520 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -57,8 +57,8 @@ class RoleBasedAccessControlFilterConfig { Filters::Common::RBAC::RoleBasedAccessControlFilterStats stats_; - std::unique_ptr engine_; - std::unique_ptr shadow_engine_; + std::unique_ptr engine_; + std::unique_ptr shadow_engine_; }; using RoleBasedAccessControlFilterConfigSharedPtr = diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index 3242b11a7ac45..bda15cfb8dd61 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -31,7 +31,7 @@ TEST(Context, RequestAttributes) { {":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"}, {"content-length", "10"}, {"x-request-id", "blah"}, }; - RequestWrapper request(header_map, info); + RequestWrapper request(&header_map, info); EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); // "2018-04-03T23:06:09.123Z". @@ -146,7 +146,7 @@ TEST(Context, RequestAttributes) { TEST(Context, ResponseAttributes) { NiceMock info; - ResponseWrapper response(info); + ResponseWrapper response(nullptr, nullptr, info); EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404)); EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123)); @@ -177,6 +177,8 @@ TEST(Context, ConnectionAttributes) { new NiceMock()); NiceMock connection_info; ConnectionWrapper connection(info); + PeerWrapper source(info, false); + PeerWrapper destination(info, true); Network::Address::InstanceConstSharedPtr local = Network::Utility::parseInternetAddress("1.2.3.4", 123, false); @@ -199,28 +201,28 @@ TEST(Context, ConnectionAttributes) { } { - auto value = connection[CelValue::CreateString(LocalAddress)]; + auto value = destination[CelValue::CreateString(Address)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsString()); EXPECT_EQ("1.2.3.4:123", value.value().StringOrDie().value()); } { - auto value = connection[CelValue::CreateString(LocalPort)]; + auto value = destination[CelValue::CreateString(Port)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); EXPECT_EQ(123, value.value().Int64OrDie()); } { - auto value = connection[CelValue::CreateString(RemoteAddress)]; + auto value = source[CelValue::CreateString(Address)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsString()); EXPECT_EQ("10.20.30.40:456", value.value().StringOrDie().value()); } { - auto value = connection[CelValue::CreateString(RemotePort)]; + auto value = source[CelValue::CreateString(Port)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); EXPECT_EQ(456, value.value().Int64OrDie()); From c1f189056d9fa2a47601e93c870e4b5d5b641889 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Fri, 9 Aug 2019 10:57:45 -0700 Subject: [PATCH 29/32] bump up coverage Signed-off-by: Kuat Yessenov --- .../extensions/filters/common/expr/context.cc | 4 +++ .../filters/common/expr/context_test.cc | 34 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index a8867a8d95f2e..24cf3bca66e5a 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -100,6 +100,10 @@ absl::optional ResponseWrapper::operator[](CelValue key) const { } } else if (value == Size) { return CelValue::CreateInt64(info_.bytesSent()); + } else if (value == Headers) { + return CelValue::CreateMap(&headers_); + } else if (value == Trailers) { + return CelValue::CreateMap(&trailers_); } return {}; } diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index bda15cfb8dd61..ced342da5e5c9 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -146,7 +146,11 @@ TEST(Context, RequestAttributes) { TEST(Context, ResponseAttributes) { NiceMock info; - ResponseWrapper response(nullptr, nullptr, info); + const std::string header_name = "test-header"; + const std::string trailer_name = "test-trailer"; + Http::TestHeaderMapImpl header_map{{header_name, "a"}}; + Http::TestHeaderMapImpl trailer_map{{trailer_name, "b"}}; + ResponseWrapper response(&header_map, &trailer_map, info); EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404)); EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123)); @@ -169,6 +173,34 @@ TEST(Context, ResponseAttributes) { ASSERT_TRUE(value.value().IsInt64()); EXPECT_EQ(404, value.value().Int64OrDie()); } + + { + auto value = response[CelValue::CreateString(Headers)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsMap()); + auto& map = *value.value().MapOrDie(); + EXPECT_FALSE(map.empty()); + EXPECT_EQ(1, map.size()); + + auto header = map[CelValue::CreateString(header_name)]; + EXPECT_TRUE(header.has_value()); + ASSERT_TRUE(header.value().IsString()); + EXPECT_EQ("a", header.value().StringOrDie().value()); + } + + { + auto value = response[CelValue::CreateString(Trailers)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsMap()); + auto& map = *value.value().MapOrDie(); + EXPECT_FALSE(map.empty()); + EXPECT_EQ(1, map.size()); + + auto header = map[CelValue::CreateString(trailer_name)]; + EXPECT_TRUE(header.has_value()); + ASSERT_TRUE(header.value().IsString()); + EXPECT_EQ("b", header.value().StringOrDie().value()); + } } TEST(Context, ConnectionAttributes) { From 76b6788ef462d72c0f15b23a162ef25c0fc50ad1 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 15 Aug 2019 07:48:09 -0700 Subject: [PATCH 30/32] update cel-cpp Signed-off-by: Kuat Yessenov --- bazel/com_google_cel_cpp.patch | 67 ---------------------------------- bazel/repositories.bzl | 9 +---- bazel/repository_locations.bzl | 6 +-- 3 files changed, 4 insertions(+), 78 deletions(-) delete mode 100644 bazel/com_google_cel_cpp.patch diff --git a/bazel/com_google_cel_cpp.patch b/bazel/com_google_cel_cpp.patch deleted file mode 100644 index 173a0545cf0d4..0000000000000 --- a/bazel/com_google_cel_cpp.patch +++ /dev/null @@ -1,67 +0,0 @@ -diff --git a/eval/public/cel_function_adapter.h b/eval/public/cel_function_adapter.h -index d99239c..4dc8cae 100644 ---- a/eval/public/cel_function_adapter.h -+++ b/eval/public/cel_function_adapter.h -@@ -118,6 +118,34 @@ class FunctionAdapter : public CelFunction { - return registry->Register(std::move(status.ValueOrDie())); - } - -+#if !defined(__clang_major_version__) || (defined(__clang_major_version__) && __clang_major_version__ < 8) || defined(__APPLE__) -+ inline cel_base::Status RunWrap(std::function func, -+ const absl::Span argset, -+ ::google::protobuf::Arena* arena, CelValue* result, -+ int arg_index) const { -+ return CreateReturnValue(func(), arena, result); -+ } -+ -+ template -+ inline cel_base::Status RunWrap(std::function func, -+ const absl::Span argset, -+ ::google::protobuf::Arena* arena, CelValue* result, -+ int arg_index) const { -+ Arg argument; -+ if (!ConvertFromValue(argset[arg_index], &argument)) { -+ return cel_base::Status(cel_base::StatusCode::kInvalidArgument, -+ "Type conversion failed"); -+ } -+ -+ std::function wrapped_func = -+ [func, argument](Args... args) -> ReturnType { -+ return func(argument, args...); -+ }; -+ -+ return RunWrap(std::move(wrapped_func), argset, arena, result, -+ arg_index + 1); -+ } -+#else - template - inline cel_base::Status RunWrap(absl::Span arguments, - std::tuple<::google::protobuf::Arena*, Arguments...> input, -@@ -137,6 +165,7 @@ class FunctionAdapter : public CelFunction { - ::google::protobuf::Arena* arena) const { - return CreateReturnValue(absl::apply(handler_, input), arena, result); - } -+#endif - - ::cel_base::Status Evaluate(absl::Span arguments, - CelValue* result, -@@ -146,9 +175,19 @@ class FunctionAdapter : public CelFunction { - "Argument number mismatch"); - } - -+ -+#if !defined(__clang_major_version__) || (defined(__clang_major_version__) && __clang_major_version__ < 8) || defined(__APPLE__) -+ const auto* handler = &handler_; -+ std::function wrapped_handler = -+ [handler, arena](Arguments... args) -> ReturnType { -+ return (*handler)(arena, args...); -+ }; -+ return RunWrap(std::move(wrapped_handler), arguments, arena, result, 0); -+#else - std::tuple<::google::protobuf::Arena*, Arguments...> input; - std::get<0>(input) = arena; - return RunWrap<0>(arguments, input, result, arena); -+#endif - } - - private: diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 96bebe409f958..319d374286bd4 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -318,14 +318,7 @@ def _net_zlib(): ) def _com_google_cel_cpp(): - location = REPOSITORY_LOCATIONS["com_google_cel_cpp"] - http_archive( - name = "com_google_cel_cpp", - # TODO(kyessenov): requires C++17 partial template specialization available in clang-8 but not gcc - patch_args = ["-p1"], - patches = ["@envoy//bazel:com_google_cel_cpp.patch"], - **location - ) + _repository_impl("com_google_cel_cpp") def _com_github_nghttp2_nghttp2(): location = REPOSITORY_LOCATIONS["com_github_nghttp2_nghttp2"] diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 89142ccdfa708..fcb0e4b79a343 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -249,9 +249,9 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://storage.googleapis.com/quiche-envoy-integration/2a930469533c3b541443488a629fe25cd8ff53d0.tar.gz"], ), com_google_cel_cpp = dict( - sha256 = "82186be314a2a9c6b9eb2477f15c4f3704b5ac9b4b26bf65694e231a48f4c1f1", - strip_prefix = "cel-cpp-71fb0562a59c05239f92025d3e7beb63169c3923", - urls = ["https://github.com/google/cel-cpp/archive/71fb0562a59c05239f92025d3e7beb63169c3923.tar.gz"], + sha256 = "f027c551d57d38fb9f0b5e4f21a2b0b8663987119e23b1fd8dfcc7588e9a2350", + strip_prefix = "cel-cpp-d9d02b20ab85da2444dbdd03410bac6822141364", + urls = ["https://github.com/google/cel-cpp/archive/d9d02b20ab85da2444dbdd03410bac6822141364.tar.gz"], ), com_googlesource_code_re2 = dict( sha256 = "f31db9cd224d018a7e4fe88ef84aaa874b0b3ed91d4d98ee5a1531101d3fdc64", From 753b3522b93162b19b215b72e6c15f7c9f8aa342 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 15 Aug 2019 13:12:35 -0700 Subject: [PATCH 31/32] merge fix Signed-off-by: Kuat Yessenov --- docs/root/intro/version_history.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 20b9f7acc96fa..b0f6f49275aba 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -24,13 +24,13 @@ Version history * rbac: added support for DNS SAN as :ref:`principal_name `. * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings. * performance: new buffer implementation enabled by default (to disable add "--use-libevent-buffers 1" to the command-line arguments when starting Envoy). +* rbac: added conditions to the policy, see :ref:`condition `. * router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * router check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. * tls: added verification of IP address SAN fields in certificates against configured SANs in the certificate validation context. * upstream: added network filter chains to upstream connections, see :ref:`filters`. -* rbac: added conditions to the policy, see :ref:`condition `. * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1. * zookeeper: parse responses and emit latency stats. From 8df3414fdc50e0ebf2a9829b2150393000e8d4bc Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 15 Aug 2019 15:18:46 -0700 Subject: [PATCH 32/32] bump up coverage Signed-off-by: Kuat Yessenov --- .../extensions/filters/common/expr/context.cc | 13 ++-- .../extensions/filters/common/expr/context.h | 8 ++- .../filters/common/expr/context_test.cc | 66 +++++++++++++++++++ .../filters/common/rbac/engine_impl_test.cc | 63 ++++++++++++++++++ 4 files changed, 140 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 24cf3bca66e5a..5ee1e91e69b35 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -21,7 +21,7 @@ absl::optional convertHeaderEntry(const Http::HeaderEntry* header) { } // namespace absl::optional HeadersWrapper::operator[](CelValue key) const { - if (!key.IsString()) { + if (value_ == nullptr || !key.IsString()) { return {}; } auto out = value_->get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); @@ -41,13 +41,10 @@ absl::optional RequestWrapper::operator[](CelValue key) const { } else if (value == Size) { // it is important to make a choice whether to rely on content-length vs stream info // (which is not available at the time of the request headers) - if (headers_.value_ != nullptr) { - auto length_header = headers_.value_->ContentLength(); - if (length_header != nullptr) { - int64_t length; - if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { - return CelValue::CreateInt64(length); - } + if (headers_.value_ != nullptr && headers_.value_->ContentLength() != nullptr) { + int64_t length; + if (absl::SimpleAtoi(headers_.value_->ContentLength()->value().getStringView(), &length)) { + return CelValue::CreateInt64(length); } } else { return CelValue::CreateInt64(info_.bytesReceived()); diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index c8c88cad6e309..7b59c41a5a101 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -61,7 +61,9 @@ class HeadersWrapper : public google::api::expr::runtime::CelMap { absl::optional operator[](CelValue key) const override; int size() const override { return value_ == nullptr ? 0 : value_->size(); } bool empty() const override { return value_ == nullptr ? true : value_->empty(); } - const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } + const google::api::expr::runtime::CelList* ListKeys() const override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } private: friend class RequestWrapper; @@ -72,7 +74,9 @@ class BaseWrapper : public google::api::expr::runtime::CelMap { public: int size() const override { return 0; } bool empty() const override { return false; } - const google::api::expr::runtime::CelList* ListKeys() const override { return nullptr; } + const google::api::expr::runtime::CelList* ListKeys() const override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } }; class RequestWrapper : public BaseWrapper { diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index ced342da5e5c9..0e79abe362ecd 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -24,6 +24,14 @@ namespace { constexpr absl::string_view Undefined = "undefined"; +TEST(Context, EmptyHeadersAttributes) { + HeadersWrapper headers(nullptr); + auto header = headers[CelValue::CreateString(Referer)]; + EXPECT_FALSE(header.has_value()); + EXPECT_EQ(0, headers.size()); + EXPECT_TRUE(headers.empty()); +} + TEST(Context, RequestAttributes) { NiceMock info; Http::TestHeaderMapImpl header_map{ @@ -40,11 +48,20 @@ TEST(Context, RequestAttributes) { absl::optional dur = std::chrono::nanoseconds(15000000); EXPECT_CALL(info, requestComplete()).WillRepeatedly(Return(dur)); + // stub methods + EXPECT_EQ(0, request.size()); + EXPECT_FALSE(request.empty()); + { auto value = request[CelValue::CreateString(Undefined)]; EXPECT_FALSE(value.has_value()); } + { + auto value = request[CelValue::CreateInt64(13)]; + EXPECT_FALSE(value.has_value()); + } + { auto value = request[CelValue::CreateString(Scheme)]; EXPECT_TRUE(value.has_value()); @@ -144,6 +161,32 @@ TEST(Context, RequestAttributes) { } } +TEST(Context, RequestFallbackAttributes) { + NiceMock info; + Http::TestHeaderMapImpl header_map{ + {":method", "POST"}, + {":scheme", "http"}, + {":path", "/meow?yes=1"}, + }; + RequestWrapper request(&header_map, info); + + EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); + + { + auto value = request[CelValue::CreateString(Size)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsInt64()); + EXPECT_EQ(10, value.value().Int64OrDie()); + } + + { + auto value = request[CelValue::CreateString(UrlPath)]; + EXPECT_TRUE(value.has_value()); + ASSERT_TRUE(value.value().IsString()); + EXPECT_EQ("/meow", value.value().StringOrDie().value()); + } +} + TEST(Context, ResponseAttributes) { NiceMock info; const std::string header_name = "test-header"; @@ -160,6 +203,11 @@ TEST(Context, ResponseAttributes) { EXPECT_FALSE(value.has_value()); } + { + auto value = response[CelValue::CreateInt64(13)]; + EXPECT_FALSE(value.has_value()); + } + { auto value = response[CelValue::CreateString(Size)]; EXPECT_TRUE(value.has_value()); @@ -186,6 +234,9 @@ TEST(Context, ResponseAttributes) { EXPECT_TRUE(header.has_value()); ASSERT_TRUE(header.value().IsString()); EXPECT_EQ("a", header.value().StringOrDie().value()); + + auto missing = map[CelValue::CreateString(Undefined)]; + EXPECT_FALSE(missing.has_value()); } { @@ -232,6 +283,21 @@ TEST(Context, ConnectionAttributes) { EXPECT_FALSE(value.has_value()); } + { + auto value = connection[CelValue::CreateInt64(13)]; + EXPECT_FALSE(value.has_value()); + } + + { + auto value = source[CelValue::CreateString(Undefined)]; + EXPECT_FALSE(value.has_value()); + } + + { + auto value = source[CelValue::CreateInt64(13)]; + EXPECT_FALSE(value.has_value()); + } + { auto value = destination[CelValue::CreateString(Address)]; EXPECT_TRUE(value.has_value()); diff --git a/test/extensions/filters/common/rbac/engine_impl_test.cc b/test/extensions/filters/common/rbac/engine_impl_test.cc index 69b8431f688c2..6d346dca62cf3 100644 --- a/test/extensions/filters/common/rbac/engine_impl_test.cc +++ b/test/extensions/filters/common/rbac/engine_impl_test.cc @@ -100,6 +100,69 @@ TEST(RoleBasedAccessControlEngineImpl, BasicCondition) { checkEngine(engine, false); } +TEST(RoleBasedAccessControlEngineImpl, MalformedCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + call_expr: + function: undefined_extent + args: + - const_expr: + bool_value: false + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + + EXPECT_THROW_WITH_REGEX(RBAC::RoleBasedAccessControlEngineImpl engine(rbac), EnvoyException, + "failed to create an expression: .*"); +} + +TEST(RoleBasedAccessControlEngineImpl, MistypedCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + const_expr: + int64_value: 13 + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + checkEngine(engine, false); +} + +TEST(RoleBasedAccessControlEngineImpl, ErrorCondition) { + envoy::config::rbac::v2::Policy policy; + policy.add_permissions()->set_any(true); + policy.add_principals()->set_any(true); + policy.mutable_condition()->MergeFrom( + TestUtility::parseYaml(R"EOF( + call_expr: + function: _[_] + args: + - select_expr: + operand: + ident_expr: + name: request + field: undefined + - const_expr: + string_value: foo + )EOF")); + + envoy::config::rbac::v2::RBAC rbac; + rbac.set_action(envoy::config::rbac::v2::RBAC_Action::RBAC_Action_ALLOW); + (*rbac.mutable_policies())["foo"] = policy; + RBAC::RoleBasedAccessControlEngineImpl engine(rbac); + checkEngine(engine, false, Envoy::Network::MockConnection()); +} + TEST(RoleBasedAccessControlEngineImpl, HeaderCondition) { envoy::config::rbac::v2::Policy policy; policy.add_permissions()->set_any(true);