From f5af373fae4b54668ef79d77b077f9cc5f50312f Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 22 May 2023 15:27:46 +0200 Subject: [PATCH 1/5] Restore HTTP payload size limit, make it configurable Fixes https://github.com/apollographql/router/issues/2000 Configuration: ```yaml preview_operation_limits: http_max_request_bytes: 2000000 # Default value: 2 MB ``` --- .changesets/feat_limit_request_size.md | 17 +++ apollo-router/src/configuration/mod.rs | 7 +- ...nfiguration__tests__schema_generation.snap | 10 +- apollo-router/src/services/router_service.rs | 140 +++++++++++++++--- docs/source/configuration/overview.mdx | 17 ++- 5 files changed, 165 insertions(+), 26 deletions(-) create mode 100644 .changesets/feat_limit_request_size.md diff --git a/.changesets/feat_limit_request_size.md b/.changesets/feat_limit_request_size.md new file mode 100644 index 0000000000..50e50843ea --- /dev/null +++ b/.changesets/feat_limit_request_size.md @@ -0,0 +1,17 @@ +### Restore HTTP payload size limit, make it configurable ([Issue #2000](https://github.com/apollographql/router/issues/2000)) + +Early versions of Apollo Router used to rely on a part of the Axum web framework +that imposed a 2 MB limit on the size of the HTTP request body. +Version 1.7 changed to read the body directly, unintentionally removing this limit. + +The limit is now restored to help protect against unbounded memory usage, but is now configurable: + +```yaml +preview_operation_limits: + http_max_request_bytes: 2000000 # Default value: 2 MB +``` + +This limit is checked while reading from the network, before JSON parsing. +Both the GraphQL document and associated variables count toward it. + +By [@SimonSapin](https://github.com/SimonSapin in https://github.com/apollographql/router/pull/3130 diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index b9c26795fd..e5358feec2 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -621,6 +621,10 @@ pub(crate) struct OperationLimits { /// Limit the number of tokens the GraphQL parser processes before aborting. pub(crate) parser_max_tokens: usize, + + /// Limit the size of incoming HTTP requests read from the network, + /// to protect against running out of memory. Default: 2000000 (2 MB) + pub(crate) http_max_request_bytes: usize, } impl Default for OperationLimits { @@ -632,12 +636,13 @@ impl Default for OperationLimits { max_root_fields: None, max_aliases: None, warn_only: false, + http_max_request_bytes: 2_000_000, + parser_max_tokens: 15_000, // This is `apollo-parser`’s default, which protects against stack overflow // but is still very high for "reasonable" queries. // https://docs.rs/apollo-parser/0.2.8/src/apollo_parser/parser/mod.rs.html#368 parser_max_recursion: 4096, - parser_max_tokens: 15_000, } } } diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index f0cae7a315..fa1eb309a1 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -1129,10 +1129,18 @@ expression: "&schema" "max_aliases": null, "warn_only": false, "parser_max_recursion": 4096, - "parser_max_tokens": 15000 + "parser_max_tokens": 15000, + "http_max_request_bytes": 2000000 }, "type": "object", "properties": { + "http_max_request_bytes": { + "description": "Limit the size of incoming HTTP requests read from the network, to protect against running out of memory. Default: 2000000 (2 MB)", + "default": 2000000, + "type": "integer", + "format": "uint", + "minimum": 0.0 + }, "max_aliases": { "description": "If set, requests with operations with more aliases than this maximum are rejected with a HTTP 400 Bad Request response and GraphQL error with `\"extensions\": {\"code\": \"MAX_ALIASES_LIMIT\"}`", "default": null, diff --git a/apollo-router/src/services/router_service.rs b/apollo-router/src/services/router_service.rs index b79310e413..4b2f4ad441 100644 --- a/apollo-router/src/services/router_service.rs +++ b/apollo-router/src/services/router_service.rs @@ -66,16 +66,22 @@ where { supergraph_creator: Arc, apq_layer: APQLayer, + http_max_request_bytes: usize, } impl RouterService where SF: ServiceFactory + Clone + Send + Sync + 'static, { - pub(crate) fn new(supergraph_creator: Arc, apq_layer: APQLayer) -> Self { + pub(crate) fn new( + supergraph_creator: Arc, + apq_layer: APQLayer, + http_max_request_bytes: usize, + ) -> Self { RouterService { supergraph_creator, apq_layer, + http_max_request_bytes, } } } @@ -177,9 +183,11 @@ where let supergraph_creator = self.supergraph_creator.clone(); let apq = self.apq_layer.clone(); + let http_max_request_bytes = self.http_max_request_bytes; let fut = async move { - let graphql_request: Result = if parts.method + let graphql_request: Result = if parts + .method == Method::GET { parts @@ -188,32 +196,68 @@ where .map(|q| { graphql::Request::from_urlencoded_query(q.to_string()).map_err(|e| { ( + StatusCode::BAD_REQUEST, "failed to decode a valid GraphQL request from path", format!("failed to decode a valid GraphQL request from path {e}"), ) }) }) .unwrap_or_else(|| { - Err(("There was no GraphQL operation to execute. Use the `query` parameter to send an operation, using either GET or POST.", "There was no GraphQL operation to execute. Use the `query` parameter to send an operation, using either GET or POST.".to_string())) + Err(( + StatusCode::BAD_REQUEST, + "There was no GraphQL operation to execute. Use the `query` parameter to send an operation, using either GET or POST.", + "There was no GraphQL operation to execute. Use the `query` parameter to send an operation, using either GET or POST.".to_string() + )) }) } else { - hyper::body::to_bytes(body) - .instrument(tracing::debug_span!("receive_body")) - .await - .map_err(|e| { - ( - "failed to get the request body", - format!("failed to get the request body: {e}"), - ) - }) - .and_then(|bytes| { - serde_json::from_reader(bytes.reader()).map_err(|err| { - ( - "failed to deserialize the request body into JSON", - format!("failed to deserialize the request body into JSON: {err}"), - ) + // FIXME: use a try block when available: https://github.com/rust-lang/rust/issues/31436 + let content_length = (|| { + parts + .headers + .get(http::header::CONTENT_LENGTH)? + .to_str() + .ok()? + .parse() + .ok() + })(); + if content_length.unwrap_or(0) > http_max_request_bytes { + Err(( + StatusCode::PAYLOAD_TOO_LARGE, + "payload too large for the `http_max_request_bytes` configuration", + "payload too large".to_string(), + )) + } else { + let body = http_body::Limited::new(body, http_max_request_bytes); + hyper::body::to_bytes(body) + .instrument(tracing::debug_span!("receive_body")) + .await + .map_err(|e| { + if e.is::() { + ( + StatusCode::PAYLOAD_TOO_LARGE, + "payload too large for the `http_max_request_bytes` configuration", + "payload too large".to_string(), + ) + } else { + ( + StatusCode::BAD_REQUEST, + "failed to get the request body", + format!("failed to get the request body: {e}"), + ) + } }) - }) + .and_then(|bytes| { + serde_json::from_reader(bytes.reader()).map_err(|err| { + ( + StatusCode::BAD_REQUEST, + "failed to deserialize the request body into JSON", + format!( + "failed to deserialize the request body into JSON: {err}" + ), + ) + }) + }) + } }; match graphql_request { @@ -378,11 +422,10 @@ where } } } - Err((error, extension_details)) => { - // BAD REQUEST + Err((status_code, error, extension_details)) => { ::tracing::error!( monotonic_counter.apollo_router_http_requests_total = 1u64, - status = %400, + status = %status_code.as_u16(), error = %error, %error ); @@ -395,7 +438,7 @@ where .extension("details", extension_details) .build(), ) - .status_code(StatusCode::BAD_REQUEST) + .status_code(status_code) .header(CONTENT_TYPE, APPLICATION_JSON.essence_str()) .context(context) .build() @@ -423,6 +466,7 @@ where supergraph_creator: Arc, static_page: StaticPageLayer, apq_layer: APQLayer, + http_max_request_bytes: usize, } impl ServiceFactory for RouterCreator @@ -486,6 +530,9 @@ where supergraph_creator, static_page, apq_layer, + http_max_request_bytes: configuration + .preview_operation_limits + .http_max_request_bytes, } } @@ -500,6 +547,7 @@ where let router_service = content_negociation::RouterLayer::default().layer(RouterService::new( self.supergraph_creator.clone(), self.apq_layer.clone(), + self.http_max_request_bytes, )); ServiceBuilder::new() @@ -691,4 +739,50 @@ mod tests { assert_eq!(expected_error, actual_error); assert!(response.errors[0].extensions.contains_key("code")); } + + #[tokio::test] + async fn test_http_max_request_bytes() { + /// Size of the JSON serialization of the request created by `fn canned_new` + /// in `apollo-router/src/services/supergraph.rs` + const CANNED_REQUEST_LEN: usize = 391; + + async fn with_config(http_max_request_bytes: usize) -> router::Response { + let http_request = supergraph::Request::canned_builder() + .build() + .unwrap() + .supergraph_request + .map(|body| { + let json_bytes = serde_json::to_vec(&body).unwrap(); + assert_eq!( + json_bytes.len(), + CANNED_REQUEST_LEN, + "The request generated by `fn canned_new` \ + in `apollo-router/src/services/supergraph.rs` has changed. \ + Please update `CANNED_REQUEST_LEN` accordingly." + ); + hyper::Body::from(json_bytes) + }); + let config = serde_json::json!({ + "preview_operation_limits": { + "http_max_request_bytes": http_max_request_bytes + } + }); + crate::TestHarness::builder() + .configuration_json(config) + .unwrap() + .build_router() + .await + .unwrap() + .oneshot(router::Request::from(http_request)) + .await + .unwrap() + } + // Send a request just at (under) the limit + let response = with_config(CANNED_REQUEST_LEN).await.response; + assert_eq!(response.status(), http::StatusCode::OK); + + // Send a request just over the limit + let response = with_config(CANNED_REQUEST_LEN - 1).await.response; + assert_eq!(response.status(), http::StatusCode::PAYLOAD_TOO_LARGE); + } } diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index 8c363ff686..db2873e522 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -513,8 +513,9 @@ tls: > > To set request limits, you must run v1.17 or later of the Apollo Router. [Download the latest version.](../quickstart#download-options) -The Apollo Router supports enforcing two types of request limits for enhanced security: +The Apollo Router supports enforcing three types of request limits for enhanced security: +- Network-based limits - Lexical, parser-based limits - Semantic, operation-based limits (this is an [Enterprise feature](../enterprise-features/)) @@ -522,6 +523,9 @@ The router rejects any request that violates at least one of these limits. ```yaml title="router.yaml" preview_operation_limits: + # Network-based limits + http_max_request_bytes: 2000000 # Default value: 2 MB + # Parser-based limits parser_max_tokens: 15000 # Default value parser_max_recursion: 4096 # Default value @@ -537,6 +541,17 @@ preview_operation_limits: See [this article](./operation-limits/). +#### Network-based limits + +##### `http_max_request_bytes` + +Limits the amount of data read from the network for the body of HTTP requests, +to protects against unbounded memory consumption. +This limit is checked before JSON parsing. +Both the GraphQL document and associated variables count toward it. + +The default value is `2000000` bytes, 2 MB. + #### Parser-based limits ##### `parser_max_tokens` From 171957af1dc6fc22d3d27c459210a1ecfe35db35 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 30 May 2023 17:48:32 +0200 Subject: [PATCH 2/5] Add perf warning --- .changesets/feat_limit_request_size.md | 4 ++++ docs/source/configuration/overview.mdx | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/.changesets/feat_limit_request_size.md b/.changesets/feat_limit_request_size.md index 50e50843ea..fe7aa63825 100644 --- a/.changesets/feat_limit_request_size.md +++ b/.changesets/feat_limit_request_size.md @@ -14,4 +14,8 @@ preview_operation_limits: This limit is checked while reading from the network, before JSON parsing. Both the GraphQL document and associated variables count toward it. +Before increasing this limit significantly consider doing performance testing +in an environment similar to your production, especially if some clients are untrusted. +Many concurrent large requests could can the Router to run out of memory. + By [@SimonSapin](https://github.com/SimonSapin in https://github.com/apollographql/router/pull/3130 diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index db2873e522..86a181dc67 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -552,6 +552,10 @@ Both the GraphQL document and associated variables count toward it. The default value is `2000000` bytes, 2 MB. +Before increasing this limit significantly consider doing performance testing +in an environment similar to your production, especially if some clients are untrusted. +Many concurrent large requests could can the Router to run out of memory. + #### Parser-based limits ##### `parser_max_tokens` From 0e9ed5891c979ed767903949de9f2c221a794f3f Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 30 May 2023 17:50:54 +0200 Subject: [PATCH 3/5] Mark http_max_request_bytes as experimental --- .changesets/feat_limit_request_size.md | 2 +- apollo-router/src/configuration/mod.rs | 4 +-- ...nfiguration__tests__schema_generation.snap | 4 +-- apollo-router/src/services/router_service.rs | 30 +++++++++---------- docs/source/configuration/overview.mdx | 4 ++- 5 files changed, 23 insertions(+), 21 deletions(-) diff --git a/.changesets/feat_limit_request_size.md b/.changesets/feat_limit_request_size.md index fe7aa63825..775c80acd5 100644 --- a/.changesets/feat_limit_request_size.md +++ b/.changesets/feat_limit_request_size.md @@ -8,7 +8,7 @@ The limit is now restored to help protect against unbounded memory usage, but is ```yaml preview_operation_limits: - http_max_request_bytes: 2000000 # Default value: 2 MB + experimental_http_max_request_bytes: 2000000 # Default value: 2 MB ``` This limit is checked while reading from the network, before JSON parsing. diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index e5358feec2..15752f5a85 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -624,7 +624,7 @@ pub(crate) struct OperationLimits { /// Limit the size of incoming HTTP requests read from the network, /// to protect against running out of memory. Default: 2000000 (2 MB) - pub(crate) http_max_request_bytes: usize, + pub(crate) experimental_http_max_request_bytes: usize, } impl Default for OperationLimits { @@ -636,7 +636,7 @@ impl Default for OperationLimits { max_root_fields: None, max_aliases: None, warn_only: false, - http_max_request_bytes: 2_000_000, + experimental_http_max_request_bytes: 2_000_000, parser_max_tokens: 15_000, // This is `apollo-parser`’s default, which protects against stack overflow diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index fa1eb309a1..c301990862 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -1130,11 +1130,11 @@ expression: "&schema" "warn_only": false, "parser_max_recursion": 4096, "parser_max_tokens": 15000, - "http_max_request_bytes": 2000000 + "experimental_http_max_request_bytes": 2000000 }, "type": "object", "properties": { - "http_max_request_bytes": { + "experimental_http_max_request_bytes": { "description": "Limit the size of incoming HTTP requests read from the network, to protect against running out of memory. Default: 2000000 (2 MB)", "default": 2000000, "type": "integer", diff --git a/apollo-router/src/services/router_service.rs b/apollo-router/src/services/router_service.rs index 4b2f4ad441..8f166c719a 100644 --- a/apollo-router/src/services/router_service.rs +++ b/apollo-router/src/services/router_service.rs @@ -66,7 +66,7 @@ where { supergraph_creator: Arc, apq_layer: APQLayer, - http_max_request_bytes: usize, + experimental_http_max_request_bytes: usize, } impl RouterService @@ -76,12 +76,12 @@ where pub(crate) fn new( supergraph_creator: Arc, apq_layer: APQLayer, - http_max_request_bytes: usize, + experimental_http_max_request_bytes: usize, ) -> Self { RouterService { supergraph_creator, apq_layer, - http_max_request_bytes, + experimental_http_max_request_bytes, } } } @@ -183,7 +183,7 @@ where let supergraph_creator = self.supergraph_creator.clone(); let apq = self.apq_layer.clone(); - let http_max_request_bytes = self.http_max_request_bytes; + let experimental_http_max_request_bytes = self.experimental_http_max_request_bytes; let fut = async move { let graphql_request: Result = if parts @@ -220,14 +220,14 @@ where .parse() .ok() })(); - if content_length.unwrap_or(0) > http_max_request_bytes { + if content_length.unwrap_or(0) > experimental_http_max_request_bytes { Err(( StatusCode::PAYLOAD_TOO_LARGE, - "payload too large for the `http_max_request_bytes` configuration", + "payload too large for the `experimental_http_max_request_bytes` configuration", "payload too large".to_string(), )) } else { - let body = http_body::Limited::new(body, http_max_request_bytes); + let body = http_body::Limited::new(body, experimental_http_max_request_bytes); hyper::body::to_bytes(body) .instrument(tracing::debug_span!("receive_body")) .await @@ -235,7 +235,7 @@ where if e.is::() { ( StatusCode::PAYLOAD_TOO_LARGE, - "payload too large for the `http_max_request_bytes` configuration", + "payload too large for the `experimental_http_max_request_bytes` configuration", "payload too large".to_string(), ) } else { @@ -466,7 +466,7 @@ where supergraph_creator: Arc, static_page: StaticPageLayer, apq_layer: APQLayer, - http_max_request_bytes: usize, + experimental_http_max_request_bytes: usize, } impl ServiceFactory for RouterCreator @@ -530,9 +530,9 @@ where supergraph_creator, static_page, apq_layer, - http_max_request_bytes: configuration + experimental_http_max_request_bytes: configuration .preview_operation_limits - .http_max_request_bytes, + .experimental_http_max_request_bytes, } } @@ -547,7 +547,7 @@ where let router_service = content_negociation::RouterLayer::default().layer(RouterService::new( self.supergraph_creator.clone(), self.apq_layer.clone(), - self.http_max_request_bytes, + self.experimental_http_max_request_bytes, )); ServiceBuilder::new() @@ -741,12 +741,12 @@ mod tests { } #[tokio::test] - async fn test_http_max_request_bytes() { + async fn test_experimental_http_max_request_bytes() { /// Size of the JSON serialization of the request created by `fn canned_new` /// in `apollo-router/src/services/supergraph.rs` const CANNED_REQUEST_LEN: usize = 391; - async fn with_config(http_max_request_bytes: usize) -> router::Response { + async fn with_config(experimental_http_max_request_bytes: usize) -> router::Response { let http_request = supergraph::Request::canned_builder() .build() .unwrap() @@ -764,7 +764,7 @@ mod tests { }); let config = serde_json::json!({ "preview_operation_limits": { - "http_max_request_bytes": http_max_request_bytes + "experimental_http_max_request_bytes": experimental_http_max_request_bytes } }); crate::TestHarness::builder() diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index 86a181dc67..d22fa6640d 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -524,7 +524,7 @@ The router rejects any request that violates at least one of these limits. ```yaml title="router.yaml" preview_operation_limits: # Network-based limits - http_max_request_bytes: 2000000 # Default value: 2 MB + experimental_http_max_request_bytes: 2000000 # Default value: 2 MB # Parser-based limits parser_max_tokens: 15000 # Default value @@ -545,6 +545,8 @@ See [this article](./operation-limits/). ##### `http_max_request_bytes` +> **This configuration is currently [experimental](/resources/product-launch-stages#experimental-features).** + Limits the amount of data read from the network for the body of HTTP requests, to protects against unbounded memory consumption. This limit is checked before JSON parsing. From 2be2f3eadfaf18327d0fcfa9a85190b173e2ac13 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 31 May 2023 18:09:09 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Chandrika Srinivasan --- .changesets/feat_limit_request_size.md | 2 +- docs/source/configuration/overview.mdx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changesets/feat_limit_request_size.md b/.changesets/feat_limit_request_size.md index 775c80acd5..493b319399 100644 --- a/.changesets/feat_limit_request_size.md +++ b/.changesets/feat_limit_request_size.md @@ -16,6 +16,6 @@ Both the GraphQL document and associated variables count toward it. Before increasing this limit significantly consider doing performance testing in an environment similar to your production, especially if some clients are untrusted. -Many concurrent large requests could can the Router to run out of memory. +Many concurrent large requests could cause the Router to run out of memory. By [@SimonSapin](https://github.com/SimonSapin in https://github.com/apollographql/router/pull/3130 diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index d22fa6640d..665a28fdc0 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -554,9 +554,9 @@ Both the GraphQL document and associated variables count toward it. The default value is `2000000` bytes, 2 MB. -Before increasing this limit significantly consider doing performance testing +Before increasing this limit significantly consider testing performance in an environment similar to your production, especially if some clients are untrusted. -Many concurrent large requests could can the Router to run out of memory. +Many concurrent large requests could cause the Router to run out of memory. #### Parser-based limits From 19f349144613ee57727009da2717122521f4c9b6 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 31 May 2023 18:10:11 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Chandrika Srinivasan --- .changesets/feat_limit_request_size.md | 2 +- docs/source/configuration/overview.mdx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changesets/feat_limit_request_size.md b/.changesets/feat_limit_request_size.md index 493b319399..d17f9feedc 100644 --- a/.changesets/feat_limit_request_size.md +++ b/.changesets/feat_limit_request_size.md @@ -14,7 +14,7 @@ preview_operation_limits: This limit is checked while reading from the network, before JSON parsing. Both the GraphQL document and associated variables count toward it. -Before increasing this limit significantly consider doing performance testing +Before increasing this limit significantly consider testing performance in an environment similar to your production, especially if some clients are untrusted. Many concurrent large requests could cause the Router to run out of memory. diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index 665a28fdc0..e61e305155 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -548,7 +548,7 @@ See [this article](./operation-limits/). > **This configuration is currently [experimental](/resources/product-launch-stages#experimental-features).** Limits the amount of data read from the network for the body of HTTP requests, -to protects against unbounded memory consumption. +to protect against unbounded memory consumption. This limit is checked before JSON parsing. Both the GraphQL document and associated variables count toward it.