From f38b9c59213625b245f3fd97f86e51c245872506 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Tue, 1 Oct 2024 15:04:54 -0400 Subject: [PATCH] mobile: Fix bugs in the Apple PAC proxy resolver 1. Makes PAC proxy resolution run in its own thread, so it doesn't interfere with the main Envoy thread. 2. Adds some CFNetwork initialization logic found in Cronet that could account for bugs in CFNetworkExecuteProxyAutoConfigurationURL. 3. Explicitely runs the run loop source and makes it a private scoped run loop. 4. Fixes the weak_ptr logic in the NetworkConfigurationFilter callbacks. Signed-off-by: Ali Beyad --- mobile/library/common/BUILD | 1 + .../http/network_configuration/filter.cc | 9 +- mobile/library/common/internal_engine.cc | 9 ++ mobile/library/common/network/BUILD | 4 + .../network/apple_pac_proxy_resolver.cc | 50 ++++++--- .../common/network/apple_pac_proxy_resolver.h | 6 +- .../common/network/apple_proxy_resolver.cc | 41 +++++-- .../common/network/apple_proxy_resolver.h | 13 ++- .../common/network/proxy_resolver_interface.h | 10 ++ .../network_configuration_filter_test.cc | 1 + mobile/test/common/proxy/BUILD | 2 - .../proxy/test_apple_api_registration.cc | 6 -- .../proxy/test_apple_pac_proxy_resolver.cc | 92 ---------------- .../proxy/test_apple_pac_proxy_resolver.h | 27 ----- .../common/proxy/test_apple_proxy_resolver.cc | 5 - .../common/proxy/test_apple_proxy_resolver.h | 6 -- .../test_apple_proxy_settings_monitor.cc | 10 +- mobile/test/swift/integration/proxying/BUILD | 17 +++ .../HTTPRequestUsingPacProxyTest.swift | 101 ++++++++++++++++++ .../proxying/HTTPRequestUsingProxyTest.swift | 59 ---------- 20 files changed, 238 insertions(+), 231 deletions(-) delete mode 100644 mobile/test/common/proxy/test_apple_pac_proxy_resolver.cc delete mode 100644 mobile/test/common/proxy/test_apple_pac_proxy_resolver.h create mode 100644 mobile/test/swift/integration/proxying/HTTPRequestUsingPacProxyTest.swift diff --git a/mobile/library/common/BUILD b/mobile/library/common/BUILD index e945d1b03751..1990b609dc0d 100644 --- a/mobile/library/common/BUILD +++ b/mobile/library/common/BUILD @@ -31,6 +31,7 @@ envoy_cc_library( "//library/common/http:header_utility_lib", "//library/common/logger:logger_delegate_lib", "//library/common/network:connectivity_manager_lib", + "//library/common/network:proxy_api_lib", "//library/common/stats:utility_lib", "//library/common/types:c_types_lib", "@envoy//envoy/server:lifecycle_notifier_interface", diff --git a/mobile/library/common/extensions/filters/http/network_configuration/filter.cc b/mobile/library/common/extensions/filters/http/network_configuration/filter.cc index 20ae20e09fff..b5c003750e0b 100644 --- a/mobile/library/common/extensions/filters/http/network_configuration/filter.cc +++ b/mobile/library/common/extensions/filters/http/network_configuration/filter.cc @@ -107,15 +107,14 @@ NetworkConfigurationFilter::resolveProxy(Http::RequestHeaderMap& request_headers std::weak_ptr weak_self = weak_from_this(); Network::ProxyResolutionResult proxy_resolution_result = proxy_resolver->resolver->resolveProxy( - target_url, proxy_settings_, - [&weak_self](const std::vector& proxies) { + target_url, proxy_settings_, [weak_self](const std::vector& proxies) { RELEASE_ASSERT( Thread::MainThread::isMainOrTestThread(), "NetworkConfigurationProxy PAC proxy resolver callback not running on main thread."); // This is the callback invoked from the Apple APIs resolving the PAC file URL, which - // gets invoked on the Envoy thread. We keep a weak_ptr to this filter instance - // so that, if the stream is canceled and the filter chain is torn down in the meantime, - // we will fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved + // gets invoked on a separate thread. We keep a weak_ptr to this filter instance so that, + // if the stream is canceled and the filter chain is torn down in the meantime, we will + // fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved // proxies. if (auto filter_ptr = weak_self.lock()) { filter_ptr->onProxyResolutionComplete(Network::ProxySettings::create(proxies)); diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc index ea6c36f4ad91..0f99af112a68 100644 --- a/mobile/library/common/internal_engine.cc +++ b/mobile/library/common/internal_engine.cc @@ -10,6 +10,7 @@ #include "source/common/runtime/runtime_features.h" #include "absl/synchronization/notification.h" +#include "library/common/network/proxy_api.h" #include "library/common/stats/utility.h" namespace Envoy { @@ -131,6 +132,14 @@ envoy_status_t InternalEngine::main(std::shared_ptr opti server_ = main_common->server(); event_dispatcher_ = &server_->dispatcher(); + // If proxy resolution APIs are configured on the Engine, set the main thread's dispatcher + // on the proxy resolver for handling callbacks. + auto* proxy_resolver = static_cast( + Api::External::retrieveApi("envoy_proxy_resolver", /*allow_absent=*/true)); + if (proxy_resolver != nullptr) { + proxy_resolver->resolver->setDispatcher(event_dispatcher_); + } + cv_.notifyAll(); } END_TRY diff --git a/mobile/library/common/network/BUILD b/mobile/library/common/network/BUILD index 2efd61a6d191..f13e2595c36e 100644 --- a/mobile/library/common/network/BUILD +++ b/mobile/library/common/network/BUILD @@ -108,6 +108,7 @@ envoy_cc_library( repository = "@envoy", deps = [ ":proxy_settings_lib", + "@envoy//envoy/event:dispatcher_interface", ], ) @@ -150,6 +151,9 @@ envoy_cc_library( "//library/common/apple:utility_lib", "//library/common/extensions/cert_validator/platform_bridge:c_types_lib", "//library/common/types:c_types_lib", + "@envoy//envoy/event:dispatcher_interface", + "@envoy//source/common/common:thread_impl_lib_posix", + "@envoy//source/common/common:thread_lib", ], "//conditions:default": [], }), diff --git a/mobile/library/common/network/apple_pac_proxy_resolver.cc b/mobile/library/common/network/apple_pac_proxy_resolver.cc index f8d1fda23720..0fd01437a6b2 100644 --- a/mobile/library/common/network/apple_pac_proxy_resolver.cc +++ b/mobile/library/common/network/apple_pac_proxy_resolver.cc @@ -13,13 +13,16 @@ namespace Network { namespace { +CFStringRef pacRunLoopMode() { + static const CFStringRef runloop_mode = CFSTR("envoy.PacProxyResolver"); + return runloop_mode; +} + // Creates a CFURLRef from a C++ string URL. CFURLRef createCFURL(const std::string& url_string) { - auto cf_url_string = - CFStringCreateWithCString(kCFAllocatorDefault, url_string.c_str(), kCFStringEncodingUTF8); - auto cf_url = CFURLCreateWithString(kCFAllocatorDefault, cf_url_string, /*baseURL=*/nullptr); - CFRelease(cf_url_string); - return cf_url; + return CFURLCreateWithBytes(kCFAllocatorDefault, + reinterpret_cast(url_string.c_str()), + url_string.length(), kCFStringEncodingUTF8, nullptr); } } // namespace @@ -31,16 +34,15 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr std::unique_ptr completion_callback( static_cast(ptr)); - std::vector proxies; - if (cf_error != nullptr || cf_proxies == nullptr) { + if (cf_error == nullptr) { ENVOY_BUG(cf_error != nullptr, Apple::toString(CFErrorCopyDescription(cf_error))); - // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS - // system (URLSession) is doing. - (*completion_callback)(proxies); - return; } - for (int i = 0; i < CFArrayGetCount(cf_proxies); i++) { + // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS + // system (URLSession) is doing. + const int num_proxy_entries = cf_proxies != nullptr ? CFArrayGetCount(cf_proxies) : 0; + std::vector proxies; + for (int i = 0; i < num_proxy_entries; i++) { CFDictionaryRef cf_dictionary = static_cast(CFArrayGetValueAtIndex(cf_proxies, i)); CFStringRef cf_proxy_type = @@ -65,6 +67,8 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr } (*completion_callback)(proxies); + + CFRunLoopStop(CFRunLoopGetCurrent()); } CFRunLoopSourceRef @@ -80,13 +84,29 @@ ApplePacProxyResolver::createPacUrlResolverSource(CFURLRef cf_proxy_autoconfigur } void ApplePacProxyResolver::resolveProxies( - const std::string& target_url, const std::string& proxy_autoconfiguration_file_url, + const std::string& proxy_autoconfiguration_file_url, const std::string& target_url, ProxySettingsResolvedCallback proxy_resolution_completed) { CFURLRef cf_target_url = createCFURL(target_url); CFURLRef cf_proxy_autoconfiguration_file_url = createCFURL(proxy_autoconfiguration_file_url); + if (cf_target_url == nullptr || cf_proxy_autoconfiguration_file_url == nullptr) { + return; + } std::unique_ptr completion_callback = std::make_unique(std::move(proxy_resolution_completed)); + + // This dummy call to CFNetworkCopyProxiesForURL initializes some state within CFNetwork that is + // required by CFNetworkExecuteProxyAutoConfigurationURL. + // Chrome/Cronet on Apple does the same: + // https://source.chromium.org/chromium/chromium/src/+/main:net/proxy_resolution/proxy_resolver_apple.cc;l=241-248;drc=f5eeebaebe332e71aadfd5c43ec485cb23f0c7f8 + CFDictionaryRef empty_dictionary = + CFDictionaryCreate(nullptr, nullptr, nullptr, 0, nullptr, nullptr); + CFArrayRef empty_result = CFNetworkCopyProxiesForURL(cf_target_url, empty_dictionary); + CFRelease(empty_dictionary); + if (empty_result) { + CFRelease(empty_result); + } + // According to https://developer.apple.com/documentation/corefoundation/cfstreamclientcontext, // the version must be 0. auto context = std::make_unique( @@ -102,7 +122,9 @@ void ApplePacProxyResolver::resolveProxies( CFRunLoopSourceRef run_loop_source = createPacUrlResolverSource( cf_proxy_autoconfiguration_file_url, cf_target_url, context.release()); - CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, kCFRunLoopDefaultMode); + CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode()); + CFRunLoopRunInMode(pacRunLoopMode(), DBL_MAX, false); + CFRunLoopRemoveSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode()); CFRelease(cf_target_url); CFRelease(cf_proxy_autoconfiguration_file_url); diff --git a/mobile/library/common/network/apple_pac_proxy_resolver.h b/mobile/library/common/network/apple_pac_proxy_resolver.h index 24d349056506..673e00a6b063 100644 --- a/mobile/library/common/network/apple_pac_proxy_resolver.h +++ b/mobile/library/common/network/apple_pac_proxy_resolver.h @@ -27,13 +27,13 @@ class ApplePacProxyResolver { /** * Resolves proxy for a given URL using proxy auto configuration file that's hosted at a given * URL. - * @param target_url A request URL to resolve the proxy for. * @param proxy_autoconfiguration_file_url A URL at which a proxy configuration file is hosted. + * @param target_url A request URL to resolve the proxy for. * @param proxy_resolution_completed A function that's called with result proxies as its * arguments when proxy resolution completes. */ - void resolveProxies(const std::string& target_url, - const std::string& proxy_autoconfiguration_file_url, + void resolveProxies(const std::string& proxy_autoconfiguration_file_url, + const std::string& target_url, ProxySettingsResolvedCallback proxy_resolution_completed); protected: diff --git a/mobile/library/common/network/apple_proxy_resolver.cc b/mobile/library/common/network/apple_proxy_resolver.cc index f0bc2c329b5d..da0d6a2faead 100644 --- a/mobile/library/common/network/apple_proxy_resolver.cc +++ b/mobile/library/common/network/apple_proxy_resolver.cc @@ -1,6 +1,7 @@ #include "library/common/network/apple_proxy_resolver.h" #include "source/common/common/assert.h" +#include "source/common/common/posix/thread_impl.h" namespace Envoy { namespace Network { @@ -8,7 +9,17 @@ namespace Network { AppleProxyResolver::AppleProxyResolver() : proxy_settings_monitor_( std::make_unique(proxySettingsUpdater())), - pac_proxy_resolver_(std::make_unique()) {} + pac_proxy_resolver_(std::make_unique()), + thread_factory_(Thread::PosixThreadFactory::create()) {} + +AppleProxyResolver::~AppleProxyResolver() { + // Wait for the PAC resolution threads to finish. + for (auto& [_, thread] : pac_resolution_threads_) { + if (thread->joinable()) { + thread->join(); + } + } +} SystemProxySettingsReadCallback AppleProxyResolver::proxySettingsUpdater() { return SystemProxySettingsReadCallback( @@ -23,11 +34,14 @@ void AppleProxyResolver::start() { started_ = true; } +void AppleProxyResolver::setDispatcher(Event::Dispatcher* dispatcher) { dispatcher_ = dispatcher; } + ProxyResolutionResult AppleProxyResolver::resolveProxy(const std::string& target_url_string, std::vector& proxies, ProxySettingsResolvedCallback proxy_resolution_completed) { ASSERT(started_, "AppleProxyResolver not started."); + ASSERT(dispatcher_ != nullptr, "Dispatcher not set on the AppleProxyResolver."); std::string pac_file_url; { @@ -55,11 +69,26 @@ AppleProxyResolver::resolveProxy(const std::string& target_url_string, // implement a caching layer with TTLs for PAC file URL resolution within AppleProxyResolver // itself. ASSERT(!pac_file_url.empty(), "PAC file URL must not be empty if PAC is enabled."); - pac_proxy_resolver_->resolveProxies( - pac_file_url, target_url_string, - [proxy_resolution_completed](const std::vector& proxies) { - proxy_resolution_completed(proxies); - }); + Thread::PosixThreadPtr thread = thread_factory_->createThread( + [this, pac_file_url, target_url_string, callback = std::move(proxy_resolution_completed)] { + pac_proxy_resolver_->resolveProxies( + pac_file_url, target_url_string, + [this, callback](const std::vector& proxies) { + auto pac_resolve_thread_id = thread_factory_->currentPthreadId(); + dispatcher_->post([this, callback, proxies, pac_resolve_thread_id] { + auto thread_handle = pac_resolution_threads_.extract(pac_resolve_thread_id); + if (!thread_handle.empty()) { + thread_handle.mapped()->join(); + } + + callback(proxies); + }); + }); + }, + /* options= */ absl::nullopt, /* crash_on_failure=*/false); + Thread::ThreadId thread_id = thread->pthreadId(); + pac_resolution_threads_[thread_id] = std::move(thread); + return ProxyResolutionResult::ResultInProgress; } diff --git a/mobile/library/common/network/apple_proxy_resolver.h b/mobile/library/common/network/apple_proxy_resolver.h index c40a97f138d2..877be6572502 100644 --- a/mobile/library/common/network/apple_proxy_resolver.h +++ b/mobile/library/common/network/apple_proxy_resolver.h @@ -6,6 +6,12 @@ #include #include +#include "envoy/event/dispatcher.h" + +#include "source/common/common/posix/thread_impl.h" +#include "source/common/common/thread.h" + +#include "absl/container/flat_hash_map.h" #include "library/common/network/apple_pac_proxy_resolver.h" #include "library/common/network/apple_system_proxy_settings_monitor.h" #include "library/common/network/proxy_resolver_interface.h" @@ -20,7 +26,7 @@ namespace Network { class AppleProxyResolver : public ProxyResolver { public: AppleProxyResolver(); - virtual ~AppleProxyResolver() = default; + virtual ~AppleProxyResolver(); /** * Starts proxy resolver. It needs to be called prior to any proxy resolution attempt. @@ -38,6 +44,8 @@ class AppleProxyResolver : public ProxyResolver { resolveProxy(const std::string& target_url_string, std::vector& proxies, ProxySettingsResolvedCallback proxy_resolution_completed) override; + virtual void setDispatcher(Event::Dispatcher* dispatcher) override; + /* * Supplies a function that updates this instance's proxy settings. */ @@ -49,6 +57,9 @@ class AppleProxyResolver : public ProxyResolver { std::unique_ptr proxy_settings_monitor_; std::unique_ptr pac_proxy_resolver_; absl::optional proxy_settings_; + std::unique_ptr thread_factory_; + absl::flat_hash_map pac_resolution_threads_; + Event::Dispatcher* dispatcher_; absl::Mutex mutex_; bool started_ = false; }; diff --git a/mobile/library/common/network/proxy_resolver_interface.h b/mobile/library/common/network/proxy_resolver_interface.h index 5abedc9c1b32..afe7a90eb3e6 100644 --- a/mobile/library/common/network/proxy_resolver_interface.h +++ b/mobile/library/common/network/proxy_resolver_interface.h @@ -1,5 +1,7 @@ #pragma once +#include "envoy/event/dispatcher.h" + #include "library/common/network/proxy_settings.h" namespace Envoy { @@ -28,6 +30,14 @@ class ProxyResolver { virtual ProxyResolutionResult resolveProxy(const std::string& target_url_string, std::vector& proxies, ProxySettingsResolvedCallback proxy_resolution_completed) PURE; + + /** + * Sets the Event Dispatcher that a proxy resolver instance can use to post callbacks to. + * Typically, this would be the main Envoy thread's dispatcher (which is also the worker thread + * in Envoy Mobile). + * @param dispatcher The event dispatcher. + */ + virtual void setDispatcher(Event::Dispatcher* dispatcher) PURE; }; } // namespace Network diff --git a/mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc b/mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc index 92f7e5ba8e94..a113e7be4540 100644 --- a/mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc +++ b/mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc @@ -191,6 +191,7 @@ class MockProxyResolver : public Network::ProxyResolver { MOCK_METHOD(Network::ProxyResolutionResult, resolveProxy, (const std::string& target_url_string, std::vector& proxies, Network::ProxySettingsResolvedCallback proxy_resolution_completed)); + MOCK_METHOD(void, setDispatcher, (Event::Dispatcher * dispatcher)); }; class NetworkConfigurationFilterProxyResolverApiTest : public NetworkConfigurationFilterTest { diff --git a/mobile/test/common/proxy/BUILD b/mobile/test/common/proxy/BUILD index 6c05ec3608b8..d721e213c422 100644 --- a/mobile/test/common/proxy/BUILD +++ b/mobile/test/common/proxy/BUILD @@ -9,7 +9,6 @@ envoy_cc_test_library( srcs = select({ "@envoy//bazel:apple": [ "test_apple_api_registration.cc", - "test_apple_pac_proxy_resolver.cc", "test_apple_proxy_resolver.cc", "test_apple_proxy_settings_monitor.cc", ], @@ -18,7 +17,6 @@ envoy_cc_test_library( hdrs = select({ "@envoy//bazel:apple": [ "test_apple_api_registration.h", - "test_apple_pac_proxy_resolver.h", "test_apple_proxy_resolver.h", "test_apple_proxy_settings_monitor.h", ], diff --git a/mobile/test/common/proxy/test_apple_api_registration.cc b/mobile/test/common/proxy/test_apple_api_registration.cc index 5a8ea1ff6f9c..e9ecd449fa88 100644 --- a/mobile/test/common/proxy/test_apple_api_registration.cc +++ b/mobile/test/common/proxy/test_apple_api_registration.cc @@ -1,6 +1,5 @@ #include "test/common/proxy/test_apple_api_registration.h" -#include "test/common/proxy/test_apple_pac_proxy_resolver.h" #include "test/common/proxy/test_apple_proxy_resolver.h" #include "test/common/proxy/test_apple_proxy_settings_monitor.h" @@ -30,11 +29,6 @@ void registerTestAppleProxyResolver(absl::string_view host, int port, const bool test_resolver->setSettingsMonitorForTest( std::make_unique( std::string(host), port, use_pac_resolver, test_resolver->proxySettingsUpdater())); - if (use_pac_resolver) { - // Create a TestApplePacProxyResolver and set the test resolver to use it. - test_resolver->setPacResolverForTest( - std::make_unique(std::string(host), port)); - } // Start the resolver, as we do when registering the envoy_proxy_resolver API. test_resolver->start(); // Create a new test ProxyResolverApi. diff --git a/mobile/test/common/proxy/test_apple_pac_proxy_resolver.cc b/mobile/test/common/proxy/test_apple_pac_proxy_resolver.cc deleted file mode 100644 index 9c46434ce4c7..000000000000 --- a/mobile/test/common/proxy/test_apple_pac_proxy_resolver.cc +++ /dev/null @@ -1,92 +0,0 @@ -#include "test/common/proxy/test_apple_pac_proxy_resolver.h" - -#include -#include - -#include - -#include "library/common/network/proxy_settings.h" - -namespace Envoy { -namespace Network { - -namespace { - -// Contains state that is passed into the run loop callback. -struct RunLoopSourceInfo { - RunLoopSourceInfo(CFStreamClientContext* _context, const std::string& _host, const int _port) - : context(_context), host(_host), port(_port) {} - - CFStreamClientContext* context; - const std::string& host; - const int port; -}; - -void runLoopCallback(void* info) { - // Wrap in unique_ptr so we release the memory at the end of the function execution. - std::unique_ptr run_loop_info(static_cast(info)); - - const void* keys[] = {kCFProxyTypeKey, kCFProxyHostNameKey, kCFProxyPortNumberKey}; - const void* values[] = { - kCFProxyTypeHTTPS, - CFStringCreateWithCString(kCFAllocatorDefault, run_loop_info->host.c_str(), - kCFStringEncodingUTF8), - CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &run_loop_info->port)}; - const int num_pairs = sizeof(keys) / sizeof(CFStringRef); - - CFDictionaryRef settings_dict = CFDictionaryCreate( - kCFAllocatorDefault, static_cast(keys), static_cast(values), - num_pairs, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks); - - const void* objects[] = {settings_dict}; - CFArrayRef proxies = CFArrayCreate(kCFAllocatorDefault, static_cast(objects), - /*numValues=*/1, &kCFTypeArrayCallBacks); - - // Wrap in unique_ptr so we release the memory at the end of the function execution. - std::unique_ptr client_context(run_loop_info->context); - - // Invoke the proxy resolution callback that the ApplePacProxyResolver invokes. - Network::proxyAutoConfigurationResultCallback(/*ptr=*/client_context->info, proxies, - /*cf_error=*/nullptr); - - // Release the memory allocated above. - CFRelease(proxies); - CFRelease(settings_dict); - // We don't want to release the first value, since it's a global constant. - for (int i = 1; i < num_pairs; ++i) { - CFRelease(values[i]); - } -} - -} // namespace - -TestApplePacProxyResolver::TestApplePacProxyResolver(const std::string& host, const int port) - : host_(host), port_(port) {} - -CFRunLoopSourceRef TestApplePacProxyResolver::createPacUrlResolverSource( - CFURLRef /*cf_proxy_autoconfiguration_file_url*/, CFURLRef /*cf_target_url*/, - CFStreamClientContext* context) { - auto run_loop_info = std::make_unique(context, host_, port_); - run_loop_context_ = std::make_unique( - CFRunLoopSourceContext{/*version=*/0, - /*info=*/run_loop_info.release(), - /*retain=*/nullptr, - /*release=*/nullptr, - /*copyDescription=*/nullptr, - /*equal=*/nullptr, - /*hash=*/nullptr, - /*schedule=*/nullptr, - /*cancel=*/nullptr, - /*perform=*/runLoopCallback}); - - auto run_loop_source = - CFRunLoopSourceCreate(kCFAllocatorDefault, /*order=*/0, run_loop_context_.get()); - // There are no network events that get triggered to notify this test run loop source that - // it should process a result, so we have to signal it manually. - CFRunLoopSourceSignal(run_loop_source); - - return run_loop_source; -} - -} // namespace Network -} // namespace Envoy diff --git a/mobile/test/common/proxy/test_apple_pac_proxy_resolver.h b/mobile/test/common/proxy/test_apple_pac_proxy_resolver.h deleted file mode 100644 index d1985c967c03..000000000000 --- a/mobile/test/common/proxy/test_apple_pac_proxy_resolver.h +++ /dev/null @@ -1,27 +0,0 @@ -#pragma once - -#include -#include - -#include "library/common/network/apple_pac_proxy_resolver.h" - -namespace Envoy { -namespace Network { - -class TestApplePacProxyResolver : public Network::ApplePacProxyResolver { -public: - TestApplePacProxyResolver(const std::string& host, const int port); - virtual ~TestApplePacProxyResolver() = default; - -protected: - CFRunLoopSourceRef createPacUrlResolverSource(CFURLRef cf_proxy_autoconfiguration_file_url, - CFURLRef cf_target_url, - CFStreamClientContext* context) override; - - const std::string host_; - const int port_; - std::unique_ptr run_loop_context_; -}; - -} // namespace Network -} // namespace Envoy diff --git a/mobile/test/common/proxy/test_apple_proxy_resolver.cc b/mobile/test/common/proxy/test_apple_proxy_resolver.cc index 283518fef8bc..b2649ad65117 100644 --- a/mobile/test/common/proxy/test_apple_proxy_resolver.cc +++ b/mobile/test/common/proxy/test_apple_proxy_resolver.cc @@ -8,10 +8,5 @@ void TestAppleProxyResolver::setSettingsMonitorForTest( proxy_settings_monitor_ = std::move(monitor); } -void TestAppleProxyResolver::setPacResolverForTest( - std::unique_ptr&& resolver) { - pac_proxy_resolver_ = std::move(resolver); -} - } // namespace Network } // namespace Envoy diff --git a/mobile/test/common/proxy/test_apple_proxy_resolver.h b/mobile/test/common/proxy/test_apple_proxy_resolver.h index f5543685cb2a..96053222da8d 100644 --- a/mobile/test/common/proxy/test_apple_proxy_resolver.h +++ b/mobile/test/common/proxy/test_apple_proxy_resolver.h @@ -15,12 +15,6 @@ class TestAppleProxyResolver : public Network::AppleProxyResolver { */ void setSettingsMonitorForTest(std::unique_ptr&& monitor); - - /** - * Resets the PAC URL resolver to the supplied instance. - * For tests only. - */ - void setPacResolverForTest(std::unique_ptr&& resolver); }; } // namespace Network diff --git a/mobile/test/common/proxy/test_apple_proxy_settings_monitor.cc b/mobile/test/common/proxy/test_apple_proxy_settings_monitor.cc index 7c6ff214dc9d..64dfe90e6698 100644 --- a/mobile/test/common/proxy/test_apple_proxy_settings_monitor.cc +++ b/mobile/test/common/proxy/test_apple_proxy_settings_monitor.cc @@ -3,6 +3,8 @@ #include #include +#include "absl/strings/str_cat.h" + namespace Envoy { namespace Network { @@ -44,13 +46,11 @@ CFDictionaryRef TestAppleSystemProxySettingsMonitor::getSystemProxySettingsWithP const void* keys[] = {kCFNetworkProxiesProxyAutoConfigEnable, kCFNetworkProxiesProxyAutoConfigURLString}; + // Interpret the host + port as the location from which to obtain the PAC file. + const std::string pac_file_url = absl::StrCat("http://", host_, ":", port_); const void* values[] = { CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &one_), - // The PAC file URL doesn't matter, we don't use it in the tests; we just need it to exist to - // exercise the PAC file URL code path in the tests. - CFStringCreateWithCString(kCFAllocatorDefault, - "http://randomproxysettingsserver.com/random.pac", - kCFStringEncodingUTF8)}; + CFStringCreateWithCString(kCFAllocatorDefault, pac_file_url.c_str(), kCFStringEncodingUTF8)}; int num_pairs = sizeof(keys) / sizeof(CFStringRef); diff --git a/mobile/test/swift/integration/proxying/BUILD b/mobile/test/swift/integration/proxying/BUILD index b891bf3b39c7..4733cead3fef 100644 --- a/mobile/test/swift/integration/proxying/BUILD +++ b/mobile/test/swift/integration/proxying/BUILD @@ -21,3 +21,20 @@ envoy_mobile_swift_test( "//test/swift/integration:test_extensions", ], ) + +envoy_mobile_swift_test( + name = "http_request_using_pac_proxy_test", + srcs = [ + "HTTPRequestUsingPacProxyTest.swift", + ], + exec_properties = { + "sandboxNetwork": "standard", + }, + visibility = ["//visibility:public"], + deps = [ + "//library/objective-c:envoy_engine_objc_lib", + "//test/objective-c:envoy_test_api", + "//test/objective-c:envoy_test_server", + "//test/swift/integration:test_extensions", + ], +) diff --git a/mobile/test/swift/integration/proxying/HTTPRequestUsingPacProxyTest.swift b/mobile/test/swift/integration/proxying/HTTPRequestUsingPacProxyTest.swift new file mode 100644 index 000000000000..0e33b0976bd7 --- /dev/null +++ b/mobile/test/swift/integration/proxying/HTTPRequestUsingPacProxyTest.swift @@ -0,0 +1,101 @@ +import Envoy +import EnvoyEngine +import EnvoyTestApi +import EnvoyTestServer +import Foundation +import TestExtensions +import XCTest + +final class HTTPRequestUsingPacProxyTest: XCTestCase { + override static func setUp() { + super.setUp() + register_test_extensions() + } + + override static func tearDown() { + super.tearDown() + // Flush the stdout and stderror to show the print output. + fflush(stdout) + fflush(stderr) + } + + private func executeRequest(engine: Engine, scheme: String, authority: String, path: String) + -> String? { + let responseHeadersExpectation = + self.expectation(description: "Successful response headers received") + let responseTrailersExpectation = + self.expectation(description: "Successful response trailers received") + + let requestHeaders = RequestHeadersBuilder(method: .get, scheme: scheme, + authority: authority, path: path) + .build() + + var responseBuffer = Data() + engine.streamClient() + .newStreamPrototype() + .setOnResponseHeaders { responseHeaders, _, _ in + XCTAssertEqual(200, responseHeaders.httpStatus) + responseHeadersExpectation.fulfill() + } + .setOnResponseData { data, _, _ in + responseBuffer.append(contentsOf: data) + } + .setOnResponseTrailers { _, _ in + responseTrailersExpectation.fulfill() + } + .start() + .sendHeaders(requestHeaders, endStream: true) + + let expectations = [responseHeadersExpectation, responseTrailersExpectation] + XCTAssertEqual(XCTWaiter.wait(for: expectations, timeout: 10), .completed) + + return String(data: responseBuffer, encoding: .utf8) + } + + // TODO(abeyad): Right now, this test doesn't correctly execute + // `CFNetworkExecuteProxyAutoConfigurationURL`, so it doesn't go through the proxy, but it proves + // that PAC proxy resolution callbacks and the threading works correctly. Once the issue with + // testing `CFNetworkExecuteProxyAutoConfigurationURL` is resolved, add a check to verify that + // the request went through the actual configured proxy. + func testHTTPRequestUsingPacProxy() throws { + EnvoyTestServer.startHttpProxyServer() + let proxyPort = EnvoyTestServer.getProxyPort() + EnvoyTestServer.startHttp1PlaintextServer() + let httpPort = EnvoyTestServer.getHttpPort() + + let engineExpectation = self.expectation(description: "Run started engine") + + let engine = EngineBuilder() + .setLogLevel(.info) + .setLogger { _, msg in + print(msg, terminator: "") + } + .setOnEngineRunning { + engineExpectation.fulfill() + } + .respectSystemProxySettings(true) + .build() + + let pacScript = """ + function FindProxyForURL(url, host) { + return "PROXY 127.0.0.1:\(proxyPort)"; + } + """ + EnvoyTestServer.setHeadersAndData("Content-Type", + header_value: "application/x-ns-proxy-autoconfig", + response_body: pacScript) + EnvoyTestApi.registerTestProxyResolver("127.0.0.1", port: httpPort, usePacResolver: true) + + XCTAssertEqual(XCTWaiter.wait(for: [engineExpectation], timeout: 5), .completed) + + let host = "www.gstatic.com" + let path = "/ytlr/txt/licenses_cobalt.txt" + if let respBody = executeRequest(engine: engine, scheme: "http", authority: host, path: path) { + XCTAssertGreaterThanOrEqual(respBody.utf8.count, 5000) + } + + engine.terminate() + EnvoyTestServer.shutdownTestProxyServer() + EnvoyTestServer.shutdownTestHttpServer() + } +} diff --git a/mobile/test/swift/integration/proxying/HTTPRequestUsingProxyTest.swift b/mobile/test/swift/integration/proxying/HTTPRequestUsingProxyTest.swift index 48f5d17a089b..daa302a1bf94 100644 --- a/mobile/test/swift/integration/proxying/HTTPRequestUsingProxyTest.swift +++ b/mobile/test/swift/integration/proxying/HTTPRequestUsingProxyTest.swift @@ -139,65 +139,6 @@ final class HTTPRequestUsingProxyTest: XCTestCase { EnvoyTestServer.shutdownTestProxyServer() } - // https://github.com/envoyproxy/envoy/issues/33014 - func skipped_testHTTPSRequestUsingPacFileUrlResolver() throws { - EnvoyTestServer.startHttpsProxyServer() - let port = EnvoyTestServer.getProxyPort() - - let engineExpectation = self.expectation(description: "Run started engine") - let responseHeadersExpectation = - self.expectation(description: "Successful response headers received") - let responseBodyExpectation = - self.expectation(description: "Successful response trailers received") - - let engine = EngineBuilder() - .setLogLevel(.debug) - .setLogger { _, msg in - print(msg, terminator: "") - } - .setOnEngineRunning { - engineExpectation.fulfill() - } - .respectSystemProxySettings(true) - .build() - - EnvoyTestApi.registerTestProxyResolver("127.0.0.1", port: port, usePacResolver: true) - - XCTAssertEqual(XCTWaiter.wait(for: [engineExpectation], timeout: 5), .completed) - - let requestHeaders = RequestHeadersBuilder(method: .get, scheme: "https", - authority: "cloud.google.com", path: "/") - .build() - - var responseBuffer = Data() - engine.streamClient() - .newStreamPrototype() - .setOnResponseHeaders { responseHeaders, _, _ in - XCTAssertEqual(200, responseHeaders.httpStatus) - responseHeadersExpectation.fulfill() - } - .setOnResponseData { data, endStream, _ in - responseBuffer.append(contentsOf: data) - if endStream { - responseBodyExpectation.fulfill() - } - } - .setOnResponseTrailers { _, _ in - } - .start() - .sendHeaders(requestHeaders, endStream: true) - - let expectations = [responseHeadersExpectation, responseBodyExpectation] - XCTAssertEqual(XCTWaiter.wait(for: expectations, timeout: 10), .completed) - - if let responseBody = String(data: responseBuffer, encoding: .utf8) { - XCTAssertGreaterThanOrEqual(responseBody.utf8.count, 3900) - } - - engine.terminate() - EnvoyTestServer.shutdownTestProxyServer() - } - func testTwoHTTPRequestsUsingProxy() throws { EnvoyTestServer.startHttpProxyServer() let port = EnvoyTestServer.getProxyPort()