grpc_json_transcoder: Fine-tune configurability of strict http request validation (envoyproxy#15019)

nareddyt · tangyan · web-flow · commit f174fba476a9 · 2021-02-12T12:44:10.000-05:00
Follow-up on envoyproxy#14715 to allow finer configuration of the strict http request validation behavior. As mentioned in envoyproxy#14972, there are some cases where rejecting unknown paths is undesirable. So we provide more configurability to fit all use cases. Risk Level: Low. This is a breaking API change, but it changes the API introduces < 14 days ago in envoyproxy#14715. This is compliant. All behavior change only occurs when the option is enabled. Testing: Integration tests Docs Changes: Proto config changes Release Notes: None. envoyproxy#14715 includes release notes that still apply Signed-off-by: Teju Nareddy <nareddyt@google.com> Co-authored-by: Yan Tang <tangyan@gmail.com>
diff --git a/api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto b/api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto
@@ -69,6 +69,25 @@ message GrpcJsonTranscoder {
     bool preserve_proto_field_names = 4;
   }
 
+  message RequestValidationOptions {
+    // By default, a request that cannot be mapped to any specified gRPC
+    // :ref:`services <envoy_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.services>`
+    // will pass-through this filter.
+    // When set to true, the request will be rejected with a ``HTTP 404 Not Found``.
+    bool reject_unknown_method = 1;
+
+    // By default, a request with query parameters that cannot be mapped to the gRPC request message
+    // will pass-through this filter.
+    // When set to true, the request will be rejected with a ``HTTP 400 Bad Request``.
+    //
+    // The fields
+    // :ref:`ignore_unknown_query_parameters <envoy_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.ignore_unknown_query_parameters>`
+    // and
+    // :ref:`ignored_query_parameters <envoy_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.ignored_query_parameters>`
+    // have priority over this strict validation behavior.
+    bool reject_unknown_query_parameters = 2;
+  }
+
   oneof descriptor_set {
     option (validate.required) = true;
 
@@ -88,7 +107,12 @@ message GrpcJsonTranscoder {
   // the transcoder will translate. If the service name doesn't exist in ``proto_descriptor``,
   // Envoy will fail at startup. The ``proto_descriptor`` may contain more services than
   // the service names specified here, but they won't be translated.
+  //
+  // By default, the filter will pass through requests that do not map to any specified services.
   // If the list of services is empty, filter is considered disabled.
+  // However, this behavior changes if
+  // :ref:`reject_unknown_method <envoy_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.RequestValidationOptions.reject_unknown_method>`
+  // is enabled.
   repeated string services = 2;
 
   // Control options for response JSON. These options are passed directly to
@@ -193,25 +217,19 @@ message GrpcJsonTranscoder {
   // If this setting is not specified, the value defaults to :ref:`ALL_CHARACTERS_EXCEPT_RESERVED<envoy_api_enum_value_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.UrlUnescapeSpec.ALL_CHARACTERS_EXCEPT_RESERVED>`.
   UrlUnescapeSpec url_unescape_spec = 10 [(validate.rules).enum = {defined_only: true}];
 
-  // Whether to reject requests that cannot be transcoded.
+  // Configure the behavior when handling requests that cannot be transcoded.
   //
-  // By default, the transcoder will silently pass through HTTP requests that all malformed.
+  // By default, the transcoder will silently pass through HTTP requests that are malformed.
   // This includes requests with unknown query parameters, unregister paths, etc.
   //
-  // Set this flag to enable strict HTTP request validation, resulting in the transcoder rejecting
-  // these requests with ``HTTP 400 Bad Request``.
+  // Set these options to enable strict HTTP request validation, resulting in the transcoder rejecting
+  // such requests with a ``HTTP 4xx``. See each individual option for more details on the validation.
   // gRPC requests will still silently pass through without transcoding.
   //
-  // The benefit of this flag is a proper error message to the downstream.
-  // Without this flag, the gRPC upstream will reset the TCP connection when a
-  // malformed HTTP request is silently passed through without transcoding. The downstream will
+  // The benefit is a proper error message to the downstream.
+  // If the upstream is a gRPC server, it cannot handle the passed-through HTTP requests and will reset
+  // the TCP connection. The downstream will then
   // receive a ``HTTP 503 Service Unavailable`` due to the upstream connection reset.
   // This incorrect error message may conflict with other Envoy components, such as retry policies.
-  //
-  // Do not use this flag if the upstream supports both HTTP and gRPC.
-  // Only use this flag if the upstream is gRPC and all its services have been
-  // specified in the descriptor file in the filter config.
-  //
-  // Defaults to false for backwards compatibility.
-  bool strict_http_request_validation = 11;
+  RequestValidationOptions request_validation_options = 11;
 }
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -85,7 +85,7 @@ New Features
 * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash.
 * ext_authz: added :ref:`response_headers_to_add <envoy_v3_api_field_service.auth.v3.OkHttpResponse.response_headers_to_add>` to support sending response headers to downstream clients on OK authorization checks via gRPC.
 * ext_authz: added :ref:`allowed_client_headers_on_success <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.AuthorizationResponse.allowed_client_headers_on_success>` to support sending response headers to downstream clients on OK external authorization checks via HTTP.
-* grpc_json_transcoder: added option :ref:`strict_http_request_validation <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.strict_http_request_validation>` to reject invalid requests early.
+* grpc_json_transcoder: added :ref:`request_validation_options <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.request_validation_options>` to reject invalid requests early.
 * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route.
 * http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 dispatching. Crashes while inside the dispatching loop should dump debug information.
 * http: added support for :ref:`preconnecting <envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1.
diff --git a/generated_api_shadow/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto b/generated_api_shadow/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto
diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
@@ -223,7 +223,7 @@ JsonTranscoderConfig::JsonTranscoderConfig(
 
   match_incoming_request_route_ = proto_config.match_incoming_request_route();
   ignore_unknown_query_parameters_ = proto_config.ignore_unknown_query_parameters();
-  strict_http_request_validation_ = proto_config.strict_http_request_validation();
+  request_validation_options_ = proto_config.request_validation_options();
 }
 
 void JsonTranscoderConfig::addFileDescriptor(const Protobuf::FileDescriptorProto& file) {
@@ -444,18 +444,33 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::RequestHeade
   if (!status.ok()) {
     ENVOY_LOG(debug, "Failed to transcode request headers: {}", status.error_message());
 
-    if (config_.strict_http_request_validation_) {
-      ENVOY_LOG(debug, "Request is rejected due to strict rejection policy.");
-      error_ = true;
-      decoder_callbacks_->sendLocalReply(
-          Http::Code::BadRequest, absl::StrCat("Bad request: ", status.error_message().ToString()),
-          nullptr, absl::nullopt,
-          absl::StrCat(RcDetails::get().GrpcTranscodeFailedEarly, "{BAD_REQUEST}"));
-      return Http::FilterHeadersStatus::StopIteration;
-    } else {
-      ENVOY_LOG(debug, "Request is passed through without transcoding.");
+    if (status.code() == Code::NOT_FOUND &&
+        !config_.request_validation_options_.reject_unknown_method()) {
+      ENVOY_LOG(debug, "Request is passed through without transcoding because it cannot be mapped "
+                       "to a gRPC method.");
       return Http::FilterHeadersStatus::Continue;
     }
+
+    if (status.code() == Code::INVALID_ARGUMENT &&
+        !config_.request_validation_options_.reject_unknown_query_parameters()) {
+      ENVOY_LOG(debug, "Request is passed through without transcoding because it contains unknown "
+                       "query parameters.");
+      return Http::FilterHeadersStatus::Continue;
+    }
+
+    // protobuf::util::Status.error_code is the same as Envoy GrpcStatus
+    // This cast is safe.
+    auto http_code = Envoy::Grpc::Utility::grpcToHttpStatus(
+        static_cast<Envoy::Grpc::Status::GrpcStatus>(status.code()));
+
+    ENVOY_LOG(debug, "Request is rejected due to strict rejection policy.");
+    error_ = true;
+    decoder_callbacks_->sendLocalReply(static_cast<Http::Code>(http_code),
+                                       status.error_message().ToString(), nullptr, absl::nullopt,
+                                       absl::StrCat(RcDetails::get().GrpcTranscodeFailedEarly, "{",
+                                                    MessageUtil::CodeEnumToString(status.code()),
+                                                    "}"));
+    return Http::FilterHeadersStatus::StopIteration;
   }
 
   if (method_->request_type_is_http_body_) {
diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h
@@ -71,13 +71,15 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config>,
       Api::Api& api);
 
   /**
-   * Create an instance of Transcoder interface based on incoming request
-   * @param headers headers received from decoder
-   * @param request_input a ZeroCopyInputStream reading from downstream request body
-   * @param response_input a TranscoderInputStream reading from upstream response body
-   * @param transcoder output parameter for the instance of Transcoder interface
-   * @param method_descriptor output parameter for the method looked up from config
-   * @return status whether the Transcoder instance are successfully created or not
+   * Create an instance of Transcoder interface based on incoming request.
+   * @param headers headers received from decoder.
+   * @param request_input a ZeroCopyInputStream reading from downstream request body.
+   * @param response_input a TranscoderInputStream reading from upstream response body.
+   * @param transcoder output parameter for the instance of Transcoder interface.
+   * @param method_descriptor output parameter for the method looked up from config.
+   * @return status whether the Transcoder instance are successfully created or not. If the method
+   *         is not found, status with Code::NOT_FOUND is returned. If the method is found, but
+   * fields cannot be resolved, status with Code::INVALID_ARGUMENT is returned.
    */
   ProtobufUtil::Status
   createTranscoder(const Http::RequestHeaderMap& headers,
@@ -107,7 +109,8 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config>,
 
   bool disabled() const { return disabled_; }
 
-  bool strict_http_request_validation_{false};
+  envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
+      RequestValidationOptions request_validation_options_{};
 
 private:
   /**
diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc

-Original file line number
+Diff line change
       false);
+}
 -TEST_P(GrpcJsonTranscoderIntegrationTest, DisableStrictRequestValidation) {
 +TEST_P(GrpcJsonTranscoderIntegrationTest, DisableRequestValidation) {
   HttpIntegrationTest::initialize();
   // Transcoding does not occur from a request with the gRPC content type.
       "", true, false, R"({ "theme" : "Children")");
+}
 -TEST_P(GrpcJsonTranscoderIntegrationTest, EnableStrictRequestValidation) {
 +TEST_P(GrpcJsonTranscoderIntegrationTest, RejectUnknownMethod) {
   const std::string filter =
       R"EOF(
             name: grpc_json_transcoder
             typed_config:
               "@type": type.googleapis.com/envoy.extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder
               proto_descriptor : "{}"
               services : "bookstore.Bookstore"
 -              strict_http_request_validation : true
 +              request_validation_options:
 +                reject_unknown_method: true
             )EOF";
   config_helper_.addFilter(
       fmt::format(filter, TestEnvironment::runfilesPath("test/proto/bookstore.descriptor")));
   HttpIntegrationTest::initialize();
 -  // Transcoding does not occur from a request with the gRPC content type.
 -  // We verify the request is not transcoded because the upstream receives the same JSON body.
 +  // Transcoding does not occur from a request with the gRPC content type, even with an unknown
 +  // path. We verify the request is not transcoded because the upstream receives the same JSON body.
   // We verify the response is not transcoded because the HTTP status code does not match the gRPC
   // status.
   testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
       Http::TestRequestHeaderMapImpl{{":method", "GET"},
 -                                     {":path", "/shelves/100"},
 +                                     {":path", "/unknown/path"},
                                      {":authority", "host"},
                                      {"content-type", "application/grpc"}},
       R"({ "theme" : "Children")", {}, {}, Status(Code::NOT_FOUND, "Shelf 9999 Not Found"),
                                      {":path", "/unknown/path"},
                                      {":authority", "host"},
                                      {"content-type", "application/json"}},
 -      "", {}, {}, Status(), Http::TestResponseHeaderMapImpl{{":status", "400"}},
 -      "Bad request: Could not resolve /unknown/path to a method.", true, false, "", false);
 +      "", {}, {}, Status(), Http::TestResponseHeaderMapImpl{{":status", "404"}},
 +      "Could not resolve /unknown/path to a method.", true, false, "", false);
++
 +  // Transcoding does not occur when unknown query param is included.
 +  // HTTP Request to is passed directly to gRPC backend.
 +  // gRPC response is passed directly to HTTP client.
 +  testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
 +      Http::TestRequestHeaderMapImpl{{":method", "GET"},
 +                                     {":path", "/shelves/100?unknown=1"},
 +                                     {":authority", "host"},
 +                                     {"content-type", "application/json"}},
 +      R"({ "theme" : "Children")", {}, {}, Status(Code::NOT_FOUND, "Shelf 9999 Not Found"),
 +      Http::TestResponseHeaderMapImpl{
 +          {":status", "200"}, {"grpc-status", "5"}, {"grpc-message", "Shelf 9999 Not Found"}},
 +      "", true, false, R"({ "theme" : "Children")");
 +}
++
 +TEST_P(GrpcJsonTranscoderIntegrationTest, RejectUnknownQueryParam) {
 +  const std::string filter =
 +      R"EOF(
 +            name: grpc_json_transcoder
 +            typed_config:
 +              "@type": type.googleapis.com/envoy.extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder
 +              proto_descriptor : "{}"
 +              services : "bookstore.Bookstore"
 +              request_validation_options:
 +                reject_unknown_query_parameters: true
 +            )EOF";
 +  config_helper_.addFilter(
 +      fmt::format(filter, TestEnvironment::runfilesPath("test/proto/bookstore.descriptor")));
 +  HttpIntegrationTest::initialize();
++
 +  // Transcoding does not occur from a request with the gRPC content type, even with unknown query
 +  // params. We verify the request is not transcoded because the upstream receives the same JSON
 +  // body. We verify the response is not transcoded because the HTTP status code does not match the
 +  // gRPC status.
 +  testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
 +      Http::TestRequestHeaderMapImpl{{":method", "GET"},
 +                                     {":path", "/shelves/100?unknown=1"},
 +                                     {":authority", "host"},
 +                                     {"content-type", "application/grpc"}},
 +      R"({ "theme" : "Children")", {}, {}, Status(Code::NOT_FOUND, "Shelf 9999 Not Found"),
 +      Http::TestResponseHeaderMapImpl{
 +          {":status", "200"}, {"grpc-status", "5"}, {"grpc-message", "Shelf 9999 Not Found"}},
 +      "", true, false, R"({ "theme" : "Children")");
++
 +  // Transcoding does not occur when unknown path is called.
 +  // HTTP Request to is passed directly to gRPC backend.
 +  // gRPC response is passed directly to HTTP client.
 +  testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
 +      Http::TestRequestHeaderMapImpl{{":method", "GET"},
 +                                     {":path", "/unknown/path"},
 +                                     {":authority", "host"},
 +                                     {"content-type", "application/json"}},
 +      R"({ "theme" : "Children")", {}, {}, Status(Code::NOT_FOUND, "Shelf 9999 Not Found"),
 +      Http::TestResponseHeaderMapImpl{
 +          {":status", "200"}, {"grpc-status", "5"}, {"grpc-message", "Shelf 9999 Not Found"}},
 +      "", true, false, R"({ "theme" : "Children")");
   // Transcoding does not occur when unknown query param is included.
   // The request is rejected.
                                      {":authority", "host"},
                                      {"content-type", "application/json"}},
       "", {}, {}, Status(), Http::TestResponseHeaderMapImpl{{":status", "400"}},
 -      "Bad request: Could not find field \"unknown\" in the type \"bookstore.GetShelfRequest\".",
 -      true, false, "", false);
 +      "Could not find field \"unknown\" in the type \"bookstore.GetShelfRequest\".", true, false,
 +      "", false);
+}
 -TEST_P(GrpcJsonTranscoderIntegrationTest, EnableStrictRequestValidationIgnoreQueryParam) {
 +TEST_P(GrpcJsonTranscoderIntegrationTest, EnableRequestValidationIgnoreQueryParam) {
   const std::string filter =
       R"EOF(
             name: grpc_json_transcoder
             typed_config:
               "@type": type.googleapis.com/envoy.extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder
               proto_descriptor : "{}"
               services : "bookstore.Bookstore"
 -              strict_http_request_validation : true
               ignore_unknown_query_parameters : true
 +              request_validation_options:
 +                reject_unknown_method: true
 +                reject_unknown_query_parameters: true
             )EOF";
   config_helper_.addFilter(
       fmt::format(filter, TestEnvironment::runfilesPath("test/proto/bookstore.descriptor")));
       Http::TestResponseHeaderMapImpl{
           {":status", "404"}, {"grpc-status", "5"}, {"grpc-message", "Shelf 9999 Not Found"}},
       "");
++
 +  // Transcoding does not occur when unknown path is called.
 +  // The request is rejected, even though it has unknown query params.
 +  testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
 +      Http::TestRequestHeaderMapImpl{{":method", "GET"},
 +                                     {":path", "/unknown/path?unknown=1"},
 +                                     {":authority", "host"},
 +                                     {"content-type", "application/json"}},
 +      "", {}, {}, Status(), Http::TestResponseHeaderMapImpl{{":status", "404"}},
 +      "Could not resolve /unknown/path to a method.", true, false, "", false);
+}
 TEST_P(GrpcJsonTranscoderIntegrationTest, RouteDisabled) {