From 7dee90107a88b836fc72e78993913988f4f73ca2 Mon Sep 17 00:00:00 2001 From: Tim Gretler Date: Wed, 10 Jul 2024 12:56:39 +0000 Subject: [PATCH] fix: revert: reqwest https outcalls --- Cargo.Bazel.Fuzzing.json.lock | 158 ++++++++++++++- Cargo.Bazel.Fuzzing.toml.lock | 29 +++ Cargo.Bazel.json.lock | 158 ++++++++++++++- Cargo.Bazel.toml.lock | 29 +++ Cargo.lock | 36 +++- bazel/external_crates.bzl | 8 +- hs/spec_compliance/src/IC/Test/Spec/HTTP.hs | 2 +- rs/https_outcalls/adapter/BUILD.bazel | 5 +- rs/https_outcalls/adapter/Cargo.toml | 5 +- rs/https_outcalls/adapter/src/cli.rs | 29 ++- rs/https_outcalls/adapter/src/config.rs | 7 +- rs/https_outcalls/adapter/src/lib.rs | 88 ++++----- rs/https_outcalls/adapter/src/rpc_server.rs | 180 +++++++++--------- .../adapter/tests/server_test.rs | 8 +- .../canister_http_correctness_test.rs | 4 +- 15 files changed, 562 insertions(+), 184 deletions(-) diff --git a/Cargo.Bazel.Fuzzing.json.lock b/Cargo.Bazel.Fuzzing.json.lock index 571028f8d3b..d2ce38ee30d 100644 --- a/Cargo.Bazel.Fuzzing.json.lock +++ b/Cargo.Bazel.Fuzzing.json.lock @@ -1,5 +1,5 @@ { - "checksum": "2d95b9a0598d619db8e9dd746b898c65a5d9aa83aa59ff3504b923589385190b", + "checksum": "3c34a999dab2ac53fd2a720fc2a53ecf6f3d37d038f75ac4bfaf1a820202751e", "crates": { "abnf 0.12.0": { "name": "abnf", @@ -3629,6 +3629,67 @@ ], "license_file": null }, + "async-socks5 0.5.1": { + "name": "async-socks5", + "version": "0.5.1", + "package_url": "https://github.com/ark0f/async-socks5", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/async-socks5/0.5.1/download", + "sha256": "77f634add2445eb2c1f785642a67ca1073fedd71e73dc3ca69435ef9b9bdedc7" + } + }, + "targets": [ + { + "Library": { + "crate_name": "async_socks5", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": false, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "async_socks5", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "deps": { + "common": [ + { + "id": "thiserror 1.0.57", + "target": "thiserror" + }, + { + "id": "tokio 1.38.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2018", + "proc_macro_deps": { + "common": [ + { + "id": "async-trait 0.1.74", + "target": "async_trait" + } + ], + "selects": {} + }, + "version": "0.5.1" + }, + "license": "Apache-2.0 OR MIT", + "license_ids": [ + "Apache-2.0", + "MIT" + ], + "license_file": "LICENSE-APACHE.md" + }, "async-stream 0.3.5": { "name": "async-stream", "version": "0.3.5", @@ -17413,6 +17474,10 @@ "target": "hyper_rustls", "alias": "hyper_rustls_0_27_x" }, + { + "id": "hyper-socks2 0.8.0", + "target": "hyper_socks2" + }, { "id": "hyper-util 0.1.3", "target": "hyper_util" @@ -27092,6 +27157,96 @@ ], "license_file": "LICENSE" }, + "hyper-socks2 0.8.0": { + "name": "hyper-socks2", + "version": "0.8.0", + "package_url": "https://github.com/ark0f/hyper-socks2", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/hyper-socks2/0.8.0/download", + "sha256": "cc38166fc2732d450e9372388d269eb38ff0b75a3cfb4c542e65b2f6893629c4" + } + }, + "targets": [ + { + "Library": { + "crate_name": "hyper_socks2", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": false, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "hyper_socks2", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "crate_features": { + "common": [ + "hyper-rustls", + "rustls", + "rustls-native-certs", + "rusttls" + ], + "selects": {} + }, + "deps": { + "common": [ + { + "id": "async-socks5 0.5.1", + "target": "async_socks5" + }, + { + "id": "futures 0.3.30", + "target": "futures" + }, + { + "id": "http 0.2.12", + "target": "http" + }, + { + "id": "hyper 0.14.27", + "target": "hyper" + }, + { + "id": "hyper-rustls 0.24.2", + "target": "hyper_rustls" + }, + { + "id": "rustls 0.21.12", + "target": "rustls", + "alias": "rusttls" + }, + { + "id": "rustls-native-certs 0.6.3", + "target": "rustls_native_certs" + }, + { + "id": "thiserror 1.0.57", + "target": "thiserror" + }, + { + "id": "tokio 1.38.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2018", + "version": "0.8.0" + }, + "license": "Apache-2.0 OR MIT", + "license_ids": [ + "Apache-2.0", + "MIT" + ], + "license_file": "LICENSE-APACHE.md" + }, "hyper-timeout 0.4.1": { "name": "hyper-timeout", "version": "0.4.1", @@ -77254,6 +77409,7 @@ "hyper 1.4.0", "hyper-rustls 0.24.2", "hyper-rustls 0.27.1", + "hyper-socks2 0.8.0", "hyper-util 0.1.3", "hyperlocal-next 0.9.0", "ic-agent 0.35.0", diff --git a/Cargo.Bazel.Fuzzing.toml.lock b/Cargo.Bazel.Fuzzing.toml.lock index 3f8ebca8f4c..72f41dc44c8 100644 --- a/Cargo.Bazel.Fuzzing.toml.lock +++ b/Cargo.Bazel.Fuzzing.toml.lock @@ -637,6 +637,17 @@ dependencies = [ "tokio", ] +[[package]] +name = "async-socks5" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77f634add2445eb2c1f785642a67ca1073fedd71e73dc3ca69435ef9b9bdedc7" +dependencies = [ + "async-trait", + "thiserror", + "tokio", +] + [[package]] name = "async-stream" version = "0.3.5" @@ -2992,6 +3003,7 @@ dependencies = [ "hyper 1.4.0", "hyper-rustls 0.24.2", "hyper-rustls 0.27.1", + "hyper-socks2", "hyper-util", "hyperlocal-next", "ic-agent", @@ -4700,6 +4712,23 @@ dependencies = [ "tower-service", ] +[[package]] +name = "hyper-socks2" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc38166fc2732d450e9372388d269eb38ff0b75a3cfb4c542e65b2f6893629c4" +dependencies = [ + "async-socks5", + "futures", + "http 0.2.12", + "hyper 0.14.27", + "hyper-rustls 0.24.2", + "rustls 0.21.12", + "rustls-native-certs 0.6.3", + "thiserror", + "tokio", +] + [[package]] name = "hyper-timeout" version = "0.4.1" diff --git a/Cargo.Bazel.json.lock b/Cargo.Bazel.json.lock index 0c85a2ed3d1..4ff33c3324e 100644 --- a/Cargo.Bazel.json.lock +++ b/Cargo.Bazel.json.lock @@ -1,5 +1,5 @@ { - "checksum": "4a40e9491172c70d605b9a7fd48be7c3c8ca41c566d360964d8fb5926d5e98a3", + "checksum": "a403e94bfadb3390ba1c9ac01c64cc8f65950e754839c9c40b14c539902c4db7", "crates": { "abnf 0.12.0": { "name": "abnf", @@ -3643,6 +3643,67 @@ ], "license_file": null }, + "async-socks5 0.5.1": { + "name": "async-socks5", + "version": "0.5.1", + "package_url": "https://github.com/ark0f/async-socks5", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/async-socks5/0.5.1/download", + "sha256": "77f634add2445eb2c1f785642a67ca1073fedd71e73dc3ca69435ef9b9bdedc7" + } + }, + "targets": [ + { + "Library": { + "crate_name": "async_socks5", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": false, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "async_socks5", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "deps": { + "common": [ + { + "id": "thiserror 1.0.57", + "target": "thiserror" + }, + { + "id": "tokio 1.38.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2018", + "proc_macro_deps": { + "common": [ + { + "id": "async-trait 0.1.73", + "target": "async_trait" + } + ], + "selects": {} + }, + "version": "0.5.1" + }, + "license": "Apache-2.0 OR MIT", + "license_ids": [ + "Apache-2.0", + "MIT" + ], + "license_file": "LICENSE-APACHE.md" + }, "async-stream 0.3.5": { "name": "async-stream", "version": "0.3.5", @@ -17246,6 +17307,10 @@ "target": "hyper_rustls", "alias": "hyper_rustls_0_27_x" }, + { + "id": "hyper-socks2 0.8.0", + "target": "hyper_socks2" + }, { "id": "hyper-util 0.1.3", "target": "hyper_util" @@ -26995,6 +27060,96 @@ ], "license_file": "LICENSE" }, + "hyper-socks2 0.8.0": { + "name": "hyper-socks2", + "version": "0.8.0", + "package_url": "https://github.com/ark0f/hyper-socks2", + "repository": { + "Http": { + "url": "https://static.crates.io/crates/hyper-socks2/0.8.0/download", + "sha256": "cc38166fc2732d450e9372388d269eb38ff0b75a3cfb4c542e65b2f6893629c4" + } + }, + "targets": [ + { + "Library": { + "crate_name": "hyper_socks2", + "crate_root": "src/lib.rs", + "srcs": { + "allow_empty": false, + "include": [ + "**/*.rs" + ] + } + } + } + ], + "library_target_name": "hyper_socks2", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "crate_features": { + "common": [ + "hyper-rustls", + "rustls", + "rustls-native-certs", + "rusttls" + ], + "selects": {} + }, + "deps": { + "common": [ + { + "id": "async-socks5 0.5.1", + "target": "async_socks5" + }, + { + "id": "futures 0.3.30", + "target": "futures" + }, + { + "id": "http 0.2.12", + "target": "http" + }, + { + "id": "hyper 0.14.27", + "target": "hyper" + }, + { + "id": "hyper-rustls 0.24.2", + "target": "hyper_rustls" + }, + { + "id": "rustls 0.21.12", + "target": "rustls", + "alias": "rusttls" + }, + { + "id": "rustls-native-certs 0.6.3", + "target": "rustls_native_certs" + }, + { + "id": "thiserror 1.0.57", + "target": "thiserror" + }, + { + "id": "tokio 1.38.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2018", + "version": "0.8.0" + }, + "license": "Apache-2.0 OR MIT", + "license_ids": [ + "Apache-2.0", + "MIT" + ], + "license_file": "LICENSE-APACHE.md" + }, "hyper-timeout 0.4.1": { "name": "hyper-timeout", "version": "0.4.1", @@ -77405,6 +77560,7 @@ "hyper 1.4.0", "hyper-rustls 0.24.2", "hyper-rustls 0.27.1", + "hyper-socks2 0.8.0", "hyper-util 0.1.3", "hyperlocal-next 0.9.0", "ic-agent 0.35.0", diff --git a/Cargo.Bazel.toml.lock b/Cargo.Bazel.toml.lock index e9580311edc..f31478f941c 100644 --- a/Cargo.Bazel.toml.lock +++ b/Cargo.Bazel.toml.lock @@ -639,6 +639,17 @@ dependencies = [ "tokio", ] +[[package]] +name = "async-socks5" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77f634add2445eb2c1f785642a67ca1073fedd71e73dc3ca69435ef9b9bdedc7" +dependencies = [ + "async-trait", + "thiserror", + "tokio", +] + [[package]] name = "async-stream" version = "0.3.5" @@ -2982,6 +2993,7 @@ dependencies = [ "hyper 1.4.0", "hyper-rustls 0.24.2", "hyper-rustls 0.27.1", + "hyper-socks2", "hyper-util", "hyperlocal-next", "ic-agent", @@ -4697,6 +4709,23 @@ dependencies = [ "tower-service", ] +[[package]] +name = "hyper-socks2" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc38166fc2732d450e9372388d269eb38ff0b75a3cfb4c542e65b2f6893629c4" +dependencies = [ + "async-socks5", + "futures", + "http 0.2.12", + "hyper 0.14.27", + "hyper-rustls 0.24.2", + "rustls 0.21.12", + "rustls-native-certs 0.6.3", + "thiserror", + "tokio", +] + [[package]] name = "hyper-timeout" version = "0.4.1" diff --git a/Cargo.lock b/Cargo.lock index 75b2d782826..66ab09181a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -636,6 +636,17 @@ dependencies = [ "tokio", ] +[[package]] +name = "async-socks5" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "77f634add2445eb2c1f785642a67ca1073fedd71e73dc3ca69435ef9b9bdedc7" +dependencies = [ + "async-trait", + "thiserror", + "tokio", +] + [[package]] name = "async-stream" version = "0.3.5" @@ -4968,6 +4979,21 @@ dependencies = [ "tower-service", ] +[[package]] +name = "hyper-socks2" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc38166fc2732d450e9372388d269eb38ff0b75a3cfb4c542e65b2f6893629c4" +dependencies = [ + "async-socks5", + "futures", + "http 0.2.12", + "hyper 0.14.26", + "hyper-tls", + "thiserror", + "tokio", +] + [[package]] name = "hyper-timeout" version = "0.4.1" @@ -8048,6 +8074,10 @@ dependencies = [ "byte-unit", "clap 3.2.25", "futures", + "http 0.2.12", + "hyper 0.14.26", + "hyper-rustls 0.24.2", + "hyper-socks2", "ic-adapter-metrics-server", "ic-async-utils", "ic-config", @@ -8057,7 +8087,6 @@ dependencies = [ "once_cell", "prometheus", "rand 0.8.5", - "reqwest 0.12.4", "serde", "serde_json", "slog", @@ -14434,10 +14463,11 @@ checksum = "e5ce46fe64a9d73be07dcbe690a38ce1b293be448fd8ce1e6c1b8062c9f72c6a" [[package]] name = "native-tls" -version = "0.2.12" +version = "0.2.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8614eb2c83d59d1c8cc974dd3f920198647674a0a035e1af1fa58707e317466" +checksum = "07226173c32f2926027b63cce4bcd8076c3552846cbe7925f3aaffeac0a3b92e" dependencies = [ + "lazy_static", "libc", "log", "openssl", diff --git a/bazel/external_crates.bzl b/bazel/external_crates.bzl index 7e397d3b2ad..ac18284b8a9 100644 --- a/bazel/external_crates.bzl +++ b/bazel/external_crates.bzl @@ -502,6 +502,13 @@ def external_crates_repository(name, cargo_lockfile, lockfile, sanitizers_enable "full", ], ), + "hyper-socks2": crate.spec( + version = "^0.8.0", + default_features = False, + features = [ + "rustls", + ], + ), "hyper-util": crate.spec( version = "^0.1.3", features = [ @@ -1016,7 +1023,6 @@ def external_crates_repository(name, cargo_lockfile, lockfile, sanitizers_enable "rustls-tls", "rustls-tls-native-roots", "stream", - "socks", ], ), "ring": crate.spec( diff --git a/hs/spec_compliance/src/IC/Test/Spec/HTTP.hs b/hs/spec_compliance/src/IC/Test/Spec/HTTP.hs index ff8abf85b58..9c6774ebfed 100644 --- a/hs/spec_compliance/src/IC/Test/Spec/HTTP.hs +++ b/hs/spec_compliance/src/IC/Test/Spec/HTTP.hs @@ -112,7 +112,7 @@ list_subset :: (Eq a) => [a] -> [a] -> Bool list_subset xs ys = all (\x -> elem x ys) xs headers_match :: [(T.Text, T.Text)] -> [(T.Text, T.Text)] -> Bool -headers_match xs ys = all (\x -> elem x ys) xs && all (\(n, v) -> elem (n, v) xs || n == "host" || n == "content-length" || n == "accept" || n == "user-agent" && v == "ic/1.0") ys +headers_match xs ys = all (\x -> elem x ys) xs && all (\(n, v) -> elem (n, v) xs || n == "host" || n == "content-length" || n == "accept-encoding" || n == "user-agent" && v == "ic/1.0") ys check_http_json :: String -> [(T.Text, T.Text)] -> BS.ByteString -> Maybe HttpRequest -> Assertion check_http_json _ _ _ Nothing = assertFailure "Could not parse the original HttpRequest from the response" diff --git a/rs/https_outcalls/adapter/BUILD.bazel b/rs/https_outcalls/adapter/BUILD.bazel index eb3bd8490d3..821ea5f83dc 100644 --- a/rs/https_outcalls/adapter/BUILD.bazel +++ b/rs/https_outcalls/adapter/BUILD.bazel @@ -12,7 +12,10 @@ DEPENDENCIES = [ "@crate_index//:byte-unit", "@crate_index//:clap_3_2_25", "@crate_index//:futures", - "@crate_index//:reqwest", + "@crate_index//:http_0_2_12", + "@crate_index//:hyper_0_14_27", + "@crate_index//:hyper-socks2", + "@crate_index//:hyper-rustls", "@crate_index//:prometheus", "@crate_index//:serde", "@crate_index//:serde_json", diff --git a/rs/https_outcalls/adapter/Cargo.toml b/rs/https_outcalls/adapter/Cargo.toml index 22cea1d412f..92a3d655db6 100644 --- a/rs/https_outcalls/adapter/Cargo.toml +++ b/rs/https_outcalls/adapter/Cargo.toml @@ -8,6 +8,10 @@ edition = "2021" byte-unit = "4.0.14" clap = { version = "3.2.25", features = ["derive"] } futures = { workspace = true } +http = "0.2.12" +hyper = { version = "0.14.18", features = ["full"] } +hyper-socks2 = "^0.8.0" +hyper-rustls = { workspace = true } ic-adapter-metrics-server = { path = "../../monitoring/adapter_metrics/server" } ic-async-utils = { path = "../../async_utils" } ic-config = { path = "../../config" } @@ -15,7 +19,6 @@ ic-https-outcalls-service = { path = "../service" } ic-logger = { path = "../../monitoring/logger" } ic-metrics = { path = "../../monitoring/metrics" } prometheus = { workspace = true } -reqwest = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } slog = { workspace = true } diff --git a/rs/https_outcalls/adapter/src/cli.rs b/rs/https_outcalls/adapter/src/cli.rs index 7c70472b143..33099dd2ee9 100644 --- a/rs/https_outcalls/adapter/src/cli.rs +++ b/rs/https_outcalls/adapter/src/cli.rs @@ -4,7 +4,7 @@ use crate::config::Config; use clap::Parser; -use reqwest::Url; +use http::Uri; use slog::Level; use std::{fs::File, io, path::PathBuf}; use thiserror::Error; @@ -48,20 +48,17 @@ impl Cli { let config: Config = serde_json::from_reader(file).map_err(|err| CliError::Deserialize(err.to_string()))?; - // The socks_proxy default value is an empty string which causes the socks proxy client to be None. - if !config.socks_proxy.is_empty() { - // Validate proxy URL. - // Check for general validation errors. - let uri = &config - .socks_proxy - .parse::() - .map_err(|_| CliError::Validation("Failed to parse socks_proxy url".to_string()))?; - // scheme, host, port should be present. 'socks5://someproxy.com:80' - if uri.scheme().is_empty() || uri.host().is_none() || uri.port().is_none() { - return Err(CliError::Validation( - "Make sure socks proxy url contains (scheme,host,port)".to_string(), - )); - } + // Validate proxy URL. + // Check for general validation errors. + let uri = &config + .socks_proxy + .parse::() + .map_err(|_| CliError::Validation("Failed to parse socks_proxy url".to_string()))?; + // scheme, host, port should be present. 'socks5://someproxy.com:80' + if uri.scheme().is_none() || uri.host().is_none() || uri.port().is_none() { + return Err(CliError::Validation( + "Make sure socks proxy url contains (scheme,host,port)".to_string(), + )); } Ok(config) @@ -203,7 +200,7 @@ pub mod test { assert!(result.is_err()); let error = result.unwrap_err(); let matches = match error { - CliError::Validation(message) => message.contains("Make sure socks proxy url contains"), + CliError::Validation(message) => message.contains("Failed to parse socks_proxy url"), _ => false, }; assert!(matches); diff --git a/rs/https_outcalls/adapter/src/config.rs b/rs/https_outcalls/adapter/src/config.rs index 5efbda1332d..272eebb52e1 100644 --- a/rs/https_outcalls/adapter/src/config.rs +++ b/rs/https_outcalls/adapter/src/config.rs @@ -25,7 +25,10 @@ pub struct Config { pub incoming_source: IncomingSource, pub logger: LoggerConfig, /// Socks proxy docs: https://github.com/dfinity/ic/blob/master/ic-os/boundary-guestos/docs/Components.adoc#user-content-socks-proxy - /// If the socks_proxy is an empty String, the socks proxy client will be None. + /// Proxy url is validated and needs to have scheme, host and port specified. I.e socks5://socksproxy.com:1080 + /// `Option` can't be used because the decision on using a proxy is based on the subnet and this information + /// is not present at adapter startup. So to enable/disable the proxy there exists a `socks_proxy_allowed` field in + /// the adapter request. pub socks_proxy: String, } @@ -36,7 +39,7 @@ impl Default for Config { http_request_timeout_secs: DEFAULT_HTTP_REQUEST_TIMEOUT_SECS, incoming_source: IncomingSource::default(), logger: LoggerConfig::default(), - socks_proxy: String::default(), + socks_proxy: "socks5://notaproxy:1080".to_string(), } } } diff --git a/rs/https_outcalls/adapter/src/lib.rs b/rs/https_outcalls/adapter/src/lib.rs index 888dc3468c1..6f36d284348 100644 --- a/rs/https_outcalls/adapter/src/lib.rs +++ b/rs/https_outcalls/adapter/src/lib.rs @@ -13,18 +13,20 @@ mod metrics; pub use cli::Cli; pub use config::{Config, IncomingSource}; -use reqwest::{header::HeaderMap, redirect::Policy, Client, Proxy, Url}; pub use rpc_server::CanisterHttp; use futures::{Future, Stream}; +use hyper::{client::connect::HttpConnector, Client}; +use hyper_rustls::HttpsConnectorBuilder; +use hyper_socks2::SocksConnector; use ic_https_outcalls_service::canister_http_service_server::CanisterHttpServiceServer; -use ic_logger::{info, ReplicaLogger}; +use ic_logger::ReplicaLogger; use ic_metrics::MetricsRegistry; use std::time::Duration; use tokio::io::{AsyncRead, AsyncWrite}; use tonic::transport::{ server::{Connected, Router}, - Server, + Server, Uri, }; use tower::layer::util::Identity; @@ -32,60 +34,36 @@ pub struct AdapterServer(Router); impl AdapterServer { pub fn new(config: Config, logger: ReplicaLogger, metrics: &MetricsRegistry) -> Self { - let timeout = Duration::from_secs(config.http_connect_timeout_secs); - - // TODO: NET-1703 - let socks_client = match config.socks_proxy.parse::() { - Ok(socks_url) => match Proxy::https(socks_url) { - Ok(proxy) => { - let client = Client::builder() - .proxy(proxy) - .use_rustls_tls() - .https_only(true) - .http1_only() - .redirect(Policy::none()) - .referer(false) - .default_headers(HeaderMap::new()) - .connect_timeout(timeout) - .build(); - - if client.is_err() { - info!( - logger, - "Socks Client not created: Reqwest client builder failed: {:?}", client - ); - } - - client.ok() - } - Err(err) => { - info!( - logger, - "Socks Client not created: Failed to create https proxy: {:?}", err - ); - None - } - }, - Err(err) => { - info!( - logger, - "Socks Client not created: Failed to parse socks url: {:?}", err - ); - None - } + // Socks client setup + let mut http_connector = HttpConnector::new(); + http_connector.enforce_http(false); + http_connector + .set_connect_timeout(Some(Duration::from_secs(config.http_connect_timeout_secs))); + // The proxy connnector requires a the URL scheme to be specified. I.e socks5:// + // Config validity check ensures that url includes scheme, host and port. + // Therefore the parse 'Uri' will be in the correct format. I.e socks5://somehost.com:1080 + let proxy_connector = SocksConnector { + proxy_addr: config + .socks_proxy + .parse::() + .expect("Failed to parse socks url."), + auth: None, + connector: http_connector.clone(), }; + let https_connector = HttpsConnectorBuilder::new() + .with_native_roots() + .https_only() + .enable_http1() + .wrap_connector(proxy_connector); + let socks_client = Client::builder().build::<_, hyper::Body>(https_connector); - let https_client = Client::builder() - .use_rustls_tls() - .https_only(true) - .http1_only() - .redirect(Policy::none()) - .referer(false) - .default_headers(HeaderMap::new()) - .connect_timeout(timeout) - .build() - .expect("Failed to create HTTPS client"); - + // Https client setup. + let https_connector = HttpsConnectorBuilder::new() + .with_native_roots() + .https_only() + .enable_http1() + .wrap_connector(http_connector); + let https_client = Client::builder().build::<_, hyper::Body>(https_connector); let canister_http = CanisterHttp::new(https_client, socks_client, logger, metrics); Self( diff --git a/rs/https_outcalls/adapter/src/rpc_server.rs b/rs/https_outcalls/adapter/src/rpc_server.rs index 94f9c4e6d90..82c1525e1e4 100644 --- a/rs/https_outcalls/adapter/src/rpc_server.rs +++ b/rs/https_outcalls/adapter/src/rpc_server.rs @@ -3,18 +3,23 @@ use crate::metrics::{ LABEL_DOWNLOAD, LABEL_HEADER_RECEIVE_SIZE, LABEL_HTTP_METHOD, LABEL_HTTP_SCHEME, LABEL_REQUEST_HEADERS, LABEL_RESPONSE_HEADERS, LABEL_UPLOAD, LABEL_URL_PARSE, }; +use byte_unit::Byte; use core::convert::TryFrom; -use futures::StreamExt; +use http::{header::USER_AGENT, uri::Scheme, HeaderName, HeaderValue, Uri}; +use hyper::{ + client::HttpConnector, + header::{HeaderMap, ToStrError}, + Body, Client, Method, +}; +use hyper_rustls::HttpsConnector; +use hyper_socks2::SocksConnector; +use ic_async_utils::{receive_body_without_timeout, BodyReceiveError}; use ic_https_outcalls_service::{ canister_http_service_server::CanisterHttpService, CanisterHttpSendRequest, CanisterHttpSendResponse, HttpHeader, HttpMethod, }; use ic_logger::{debug, ReplicaLogger}; use ic_metrics::MetricsRegistry; -use reqwest::{ - header::{HeaderMap, HeaderName, HeaderValue, ToStrError, USER_AGENT}, - Client, Method, Url, -}; use std::str::FromStr; use tonic::{Request, Response, Status}; @@ -30,16 +35,16 @@ const USER_AGENT_ADAPTER: &str = "ic/1.0"; /// implements RPC pub struct CanisterHttp { - client: Client, - socks_client: Option, + client: Client>, + socks_client: Client>>, logger: ReplicaLogger, metrics: AdapterMetrics, } impl CanisterHttp { pub fn new( - client: Client, - socks_client: Option, + client: Client>, + socks_client: Client>>, logger: ReplicaLogger, metrics: &MetricsRegistry, ) -> Self { @@ -62,19 +67,7 @@ impl CanisterHttpService for CanisterHttp { let req = request.into_inner(); - if !req.url.is_ascii() { - debug!(self.logger, "URL contains non-ascii characters"); - self.metrics - .request_errors - .with_label_values(&[LABEL_URL_PARSE]) - .inc(); - return Err(Status::new( - tonic::Code::InvalidArgument, - "Failed to parse URL: URL contains non-ascii characters".to_string(), - )); - } - - let url = req.url.parse::().map_err(|err| { + let uri = req.url.parse::().map_err(|err| { debug!(self.logger, "Failed to parse URL: {}", err); self.metrics .request_errors @@ -86,10 +79,10 @@ impl CanisterHttpService for CanisterHttp { ) })?; - if url.scheme() != "https" { + if uri.scheme() != Some(&Scheme::HTTPS) { debug!( self.logger, - "Got request with no or http scheme specified. {}", url + "Got request with no or http scheme specified. {}", uri ); self.metrics .request_errors @@ -144,26 +137,36 @@ impl CanisterHttpService for CanisterHttp { // If we are allowed to use socks and condition described in `should_use_socks_proxy` hold, // we do the requests through the socks proxy. If not we use the default IPv6 route. - let http_resp = match &self.socks_client { - Some(socks_client) if req.socks_proxy_allowed => { - let response = self.client.request(method.clone(), url.clone()).headers(headers.clone()).body(req.body.clone()).send().await; - - match response { + let http_resp = if req.socks_proxy_allowed { + // Http request does not implement clone. So we have to manually construct a clone. + let req_body_clone = req.body.clone(); + let mut http_req = hyper::Request::new(Body::from(req.body)); + *http_req.headers_mut() = headers; + *http_req.method_mut() = method; + *http_req.uri_mut() = uri.clone(); + let mut http_req_clone = hyper::Request::new(Body::from(req_body_clone)); + *http_req_clone.headers_mut() = http_req.headers().clone(); + *http_req_clone.method_mut() = http_req.method().clone(); + *http_req_clone.uri_mut() = http_req.uri().clone(); + + match self.client.request(http_req).await { // If we fail we try with the socks proxy. For destinations that are ipv4 only this should // fail fast because our interface does not have an ipv4 assigned. Err(direct_err) => { self.metrics.requests_socks.inc(); - socks_client.request(method.clone(), url.clone()).headers(headers.clone()).body(req.body).send().await.map_err(|e| { + self.socks_client.request(http_req_clone).await.map_err(|e| { format!("Request failed direct connect {direct_err} and connect through socks {e}") }) } Ok(resp)=> Ok(resp), } - }, - _ => { - self.client.request(method.clone(), url.clone()).headers(headers.clone()).body(req.body).send().await.map_err(|e| format!("Failed to directly connect: {e}")) + } else { + let mut http_req = hyper::Request::new(Body::from(req.body)); + *http_req.headers_mut() = headers; + *http_req.method_mut() = method; + *http_req.uri_mut() = uri.clone(); + self.client.request(http_req).await.map_err(|e| format!("Failed to directly connect: {e}")) } - } .map_err(|err| { debug!(self.logger, "Failed to connect: {}", err); self.metrics @@ -174,12 +177,11 @@ impl CanisterHttpService for CanisterHttp { tonic::Code::Unavailable, format!( "Connecting to {:.50} failed: {}", - url.host().map(|host| host.to_string()).unwrap_or("".to_string()), + uri.host().unwrap_or(""), err, ), ) })?; - self.metrics .network_traffic .with_label_values(&[LABEL_UPLOAD]) @@ -213,65 +215,53 @@ impl CanisterHttpService for CanisterHttp { ) })?; - let available_size = req - .max_response_size_bytes - .checked_sub(headers_size_bytes as u64) - .ok_or_else(|| { - self.metrics - .request_errors - .with_label_values(&[LABEL_HEADER_RECEIVE_SIZE]) - .inc(); - Status::new( - tonic::Code::OutOfRange, - format!( - "Header size exceeds specified response size limit {}", - req.max_response_size_bytes - ), - ) - })? as usize; - - let length = http_resp.content_length().unwrap_or_default() as usize; - - if length > available_size { - self.metrics - .request_errors - .with_label_values(&[LABEL_BODY_RECEIVE_SIZE]) - .inc(); - return Err(Status::new( - tonic::Code::OutOfRange, - "Value of 'Content-length' header exceeds http body size limit.", - )); - } - - let mut stream = http_resp.bytes_stream(); - let mut body_bytes: Vec = Vec::with_capacity(length); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|err| { - debug!(self.logger, "Failed to fetch body: {}", err); - self.metrics - .request_errors - .with_label_values(&[LABEL_BODY_RECEIVE_TIMEOUT]) - .inc(); - Status::new( - tonic::Code::Unavailable, - format!("Failed to fetch body: {}", err), - ) - })?; - let mut chunk = chunk.slice(..).to_vec(); - - if body_bytes.len() + chunk.len() > available_size { - self.metrics - .request_errors - .with_label_values(&[LABEL_BODY_RECEIVE_SIZE]) - .inc(); - return Err(Status::new( - tonic::Code::OutOfRange, - format!("Http body exceeds size limit of {} bytes.", available_size), - )); + // We don't need a timeout here because there is a global timeout on the entire request. + let body_bytes = receive_body_without_timeout( + http_resp.into_body(), + // Account for size of headers. + Byte::from( + req.max_response_size_bytes + .checked_sub(headers_size_bytes as u64) + .ok_or_else(|| { + self.metrics + .request_errors + .with_label_values(&[LABEL_HEADER_RECEIVE_SIZE]) + .inc(); + Status::new( + tonic::Code::OutOfRange, + format!( + "Header size exceeds specified response size limit {}", + req.max_response_size_bytes + ), + ) + })?, + ), + ) + .await + .map_err(|err| { + debug!(self.logger, "Failed to fetch body: {}", err); + match err { + // SysTransient error + BodyReceiveError::Timeout(e) | BodyReceiveError::Unavailable(e) => { + self.metrics + .request_errors + .with_label_values(&[LABEL_BODY_RECEIVE_TIMEOUT]) + .inc(); + Status::new( + tonic::Code::Unavailable, + format!("Failed to fetch body: {}", e), + ) + } + // SysFatal error + BodyReceiveError::TooLarge(e) => { + self.metrics + .request_errors + .with_label_values(&[LABEL_BODY_RECEIVE_SIZE]) + .inc(); + Status::new(tonic::Code::OutOfRange, e) + } } - - body_bytes.append(&mut chunk); - } + })?; self.metrics .network_traffic @@ -280,7 +270,7 @@ impl CanisterHttpService for CanisterHttp { Ok(Response::new(CanisterHttpSendResponse { status, headers, - content: body_bytes, + content: body_bytes.to_vec(), })) } } diff --git a/rs/https_outcalls/adapter/tests/server_test.rs b/rs/https_outcalls/adapter/tests/server_test.rs index 43ca99934b0..21333c6c588 100644 --- a/rs/https_outcalls/adapter/tests/server_test.rs +++ b/rs/https_outcalls/adapter/tests/server_test.rs @@ -3,6 +3,7 @@ // We use `hyper-rustls` which uses Rustls, which supports the SSL_CERT_FILE variable. mod test { use futures::TryFutureExt; + use http::{header::HeaderValue, StatusCode}; use ic_https_outcalls_adapter::{AdapterServer, Config}; use ic_https_outcalls_service::{ canister_http_service_client::CanisterHttpServiceClient, CanisterHttpSendRequest, @@ -20,10 +21,7 @@ mod test { use tower::service_fn; use unix::UnixListenerDrop; use uuid::Uuid; - use warp::{ - http::{header::HeaderValue, Response, StatusCode}, - Filter, - }; + use warp::{http::Response, Filter}; // Selfsigned localhost cert const CERT: &str = " @@ -338,7 +336,7 @@ MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgob29X4H4m2XOkSZE assert!(response .unwrap_err() .message() - .contains(&"Failed to directly connect".to_string())); + .contains(&"deadline has elapsed".to_string())); } #[tokio::test] diff --git a/rs/tests/networking/canister_http_correctness_test.rs b/rs/tests/networking/canister_http_correctness_test.rs index ffeddc27088..526f4261d17 100644 --- a/rs/tests/networking/canister_http_correctness_test.rs +++ b/rs/tests/networking/canister_http_correctness_test.rs @@ -8,7 +8,7 @@ Runbook:: 1. Instantiate an IC with one application subnet with the HTTP feature enabled. 2. Install NNS canisters 3. Install the proxy canister -4. Make an update call to the proxy canister +4. Make an update call to the proxy canister. Success:: 1. Received http response with status 200. @@ -464,7 +464,7 @@ pub fn test(env: TestEnv) { |response| { let err_response = response.clone().unwrap_err(); matches!(err_response.0, RejectionCode::SysTransient) - && err_response.1.contains("Failed to directly connect") + && err_response.1.contains("Connection refused") }, ) .await,