From 98d6415264c1f211f50ee7a0b336103a488c3608 Mon Sep 17 00:00:00 2001 From: Mohammed Abdellatif Date: Fri, 25 Oct 2019 19:21:32 +0000 Subject: [PATCH] Add support for GuzzleHTTP 'no' proxy The custom config allows to setup a proxy URI that is passed to GuzzleHTTP client as request options. Guzzle has the option to receive an array of proxies for each URI scheme as well as 'no' key value pair to provide a list of host names that should not be proxied to. Guzzle would automatically populate these options with HTTPS_PROXY and NO_PROXY environment variables. However, when providing a 'proxy' request option, default values will be overriden and it is required to explicitly provide the 'no' value if needed. More info: http://docs.guzzlephp.org/en/stable/request-options.html#proxy This commit will add support for a new config 'proxyexclude', which takes a list of host names to be excluded. It will also provide 'proxy' request option as an array instead of a string to Guzzle, and populate 'http' and 'https' URI schemes with proxy URI, and 'no' with 'proxyexclude' list. Also, if no 'proxy' is configured, it will leave out 'proxy' request option, so it won't override Guzzle default values. Sample config file includes a hint on how to explicitly sync 'proxyexclude' with NO_PROXY, and a note about default values. Signed-off-by: Mohammed Abdellatif --- config/config.sample.php | 16 +++ lib/private/Http/Client/Client.php | 41 ++++++-- tests/lib/Http/Client/ClientTest.php | 150 +++++++++++++++++++++++++-- 3 files changed, 189 insertions(+), 18 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 2894bc5debcd2..ea91bb83bb40b 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -520,6 +520,12 @@ /** * The URL of your proxy server, for example ``proxy.example.com:8081``. * + * Note: Guzzle (the http library used by Nextcloud) is reading the environment + * variables HTTP_PROXY (only for cli request), HTTPS_PROXY, and NO_PROXY by default. + * + * If you configure proxy with Nextcloud any default configuration by Guzzle + * is overwritten. Make sure to set ``proxyexclude`` accordingly if necessary. + * * Defaults to ``''`` (empty string) */ 'proxy' => '', @@ -532,6 +538,16 @@ */ 'proxyuserpwd' => '', +/** + * List of host names that should not be proxied to. + * For example: ``['.mit.edu', 'foo.com']``. + * + * Hint: Use something like ``explode(',', getenv('NO_PROXY'))`` to sync this + * value with the global NO_PROXY option. + * + * Defaults to empty array. + */ +'proxyexclude' => [], /** * Deleted Items (trash bin) diff --git a/lib/private/Http/Client/Client.php b/lib/private/Http/Client/Client.php index c52d90ff8ecd8..657bfb8d536b3 100644 --- a/lib/private/Http/Client/Client.php +++ b/lib/private/Http/Client/Client.php @@ -65,12 +65,20 @@ public function __construct( } private function buildRequestOptions(array $options): array { + $proxy = $this->getProxyUri(); + $defaults = [ - RequestOptions::PROXY => $this->getProxyUri(), RequestOptions::VERIFY => $this->getCertBundle(), RequestOptions::TIMEOUT => 30, ]; + // Only add RequestOptions::PROXY if Nextcloud is explicitly + // configured to use a proxy. This is needed in order not to override + // Guzzle default values. + if($proxy !== null) { + $defaults[RequestOptions::PROXY] = $proxy; + } + $options = array_merge($defaults, $options); if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) { @@ -96,11 +104,21 @@ private function getCertBundle(): string { } /** - * Get the proxy URI + * Returns a null or an associative array specifiying the proxy URI for + * 'http' and 'https' schemes, in addition to a 'no' key value pair + * providing a list of host names that should not be proxied to. + * + * @return array|null + * + * The return array looks like: + * [ + * 'http' => 'username:password@proxy.example.com', + * 'https' => 'username:password@proxy.example.com', + * 'no' => ['foo.com', 'bar.com'] + * ] * - * @return string|null */ - private function getProxyUri(): ?string { + private function getProxyUri(): ?array { $proxyHost = $this->config->getSystemValue('proxy', ''); if ($proxyHost === '' || $proxyHost === null) { @@ -108,12 +126,21 @@ private function getProxyUri(): ?string { } $proxyUserPwd = $this->config->getSystemValue('proxyuserpwd', ''); + if ($proxyUserPwd !== '' && $proxyUserPwd !== null) { + $proxyHost = $proxyUserPwd . '@' . $proxyHost; + } + + $proxy = [ + 'http' => $proxyHost, + 'https' => $proxyHost, + ]; - if ($proxyUserPwd === '' || $proxyUserPwd === null) { - return $proxyHost; + $proxyExclude = $this->config->getSystemValue('proxyexclude', []); + if ($proxyExclude !== [] && $proxyExclude !== null) { + $proxy['no'] = $proxyExclude; } - return $proxyUserPwd . '@' . $proxyHost; + return $proxy; } /** diff --git a/tests/lib/Http/Client/ClientTest.php b/tests/lib/Http/Client/ClientTest.php index 73cfb0bb6ca20..2f9e70a8bb9ea 100644 --- a/tests/lib/Http/Client/ClientTest.php +++ b/tests/lib/Http/Client/ClientTest.php @@ -63,7 +63,15 @@ public function testGetProxyUriProxyHostEmptyPassword(): void { ->method('getSystemValue') ->with('proxyuserpwd', null) ->willReturn(null); - $this->assertSame('foo', self::invokePrivate($this->client, 'getProxyUri')); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with('proxyexclude', []) + ->willReturn([]); + $this->assertEquals([ + 'http' => 'foo', + 'https' => 'foo' + ], self::invokePrivate($this->client, 'getProxyUri')); } public function testGetProxyUriProxyHostWithPassword(): void { @@ -87,7 +95,58 @@ public function testGetProxyUriProxyHostWithPassword(): void { }) ) ->willReturn('username:password'); - $this->assertSame('username:password@foo', self::invokePrivate($this->client, 'getProxyUri')); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with( + $this->equalTo('proxyexclude'), + $this->callback(function ($input) { + return $input === []; + }) + ) + ->willReturn([]); + $this->assertEquals([ + 'http' => 'username:password@foo', + 'https' => 'username:password@foo' + ], self::invokePrivate($this->client, 'getProxyUri')); + } + + public function testGetProxyUriProxyHostWithPasswordAndExclude(): void { + $this->config + ->expects($this->at(0)) + ->method('getSystemValue') + ->with( + $this->equalTo('proxy'), + $this->callback(function ($input) { + return $input === ''; + }) + ) + ->willReturn('foo'); + $this->config + ->expects($this->at(1)) + ->method('getSystemValue') + ->with( + $this->equalTo('proxyuserpwd'), + $this->callback(function ($input) { + return $input === ''; + }) + ) + ->willReturn('username:password'); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with( + $this->equalTo('proxyexclude'), + $this->callback(function ($input) { + return $input === []; + }) + ) + ->willReturn(['bar']); + $this->assertEquals([ + 'http' => 'username:password@foo', + 'https' => 'username:password@foo', + 'no' => ['bar'] + ], self::invokePrivate($this->client, 'getProxyUri')); } private function setUpDefaultRequestOptions(): void { @@ -101,6 +160,11 @@ private function setUpDefaultRequestOptions(): void { ->method('getSystemValue') ->with('proxyuserpwd', null) ->willReturn(null); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with('proxyexclude', []) + ->willReturn([]); $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -109,7 +173,10 @@ private function setUpDefaultRequestOptions(): void { $this->defaultRequestOptions = [ 'verify' => '/my/path.crt', - 'proxy' => 'foo', + 'proxy' => [ + 'http' => 'foo', + 'https' => 'foo' + ], 'headers' => [ 'User-Agent' => 'Nextcloud Server Crawler', ], @@ -131,7 +198,10 @@ public function testGetWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -154,7 +224,10 @@ public function testPostWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -177,7 +250,10 @@ public function testPutWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -200,7 +276,10 @@ public function testDeleteWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -223,7 +302,10 @@ public function testOptionsWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -246,7 +328,10 @@ public function testHeadWithOptions(): void { $options = array_merge($this->defaultRequestOptions, [ 'verify' => false, - 'proxy' => 'bar', + 'proxy' => [ + 'http' => 'bar', + 'https' => 'bar' + ], ]); $this->guzzleClient->method('request') @@ -268,7 +353,6 @@ public function testSetDefaultOptionsWithNotInstalled(): void { $this->assertEquals([ 'verify' => \OC::$SERVERROOT . '/resources/config/ca-bundle.crt', - 'proxy' => null, 'headers' => [ 'User-Agent' => 'Nextcloud Server Crawler' ], @@ -287,6 +371,11 @@ public function testSetDefaultOptionsWithProxy(): void { ->method('getSystemValue') ->with('proxyuserpwd', null) ->willReturn(null); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with('proxyexclude', []) + ->willReturn([]); $this->certificateManager ->expects($this->once()) ->method('getAbsoluteBundlePath') @@ -295,7 +384,46 @@ public function testSetDefaultOptionsWithProxy(): void { $this->assertEquals([ 'verify' => '/my/path.crt', - 'proxy' => 'foo', + 'proxy' => [ + 'http' => 'foo', + 'https' => 'foo' + ], + 'headers' => [ + 'User-Agent' => 'Nextcloud Server Crawler' + ], + 'timeout' => 30, + ], self::invokePrivate($this->client, 'buildRequestOptions', [[]])); + } + + public function testSetDefaultOptionsWithProxyAndExclude(): void { + $this->config + ->expects($this->at(0)) + ->method('getSystemValue') + ->with('proxy', null) + ->willReturn('foo'); + $this->config + ->expects($this->at(1)) + ->method('getSystemValue') + ->with('proxyuserpwd', null) + ->willReturn(null); + $this->config + ->expects($this->at(2)) + ->method('getSystemValue') + ->with('proxyexclude', []) + ->willReturn(['bar']); + $this->certificateManager + ->expects($this->once()) + ->method('getAbsoluteBundlePath') + ->with(null) + ->willReturn('/my/path.crt'); + + $this->assertEquals([ + 'verify' => '/my/path.crt', + 'proxy' => [ + 'http' => 'foo', + 'https' => 'foo', + 'no' => ['bar'] + ], 'headers' => [ 'User-Agent' => 'Nextcloud Server Crawler' ],