Fix DnsPinMiddleware resolve pinning bug

[nullspoon]

Fix #27582
Libcurl expects the value of the CURLOPT_RESOLVE configurations to be an
array of strings, those strings containing a comma delimited list of
resolved IPs for each host:port combination.

The original code here does create that array with the host:port:ip
combination, but multiple ips for a single host:port result in
additional array entries, rather than adding them to the end of the
string with a comma. Per the libcurl docs, the CURLOPT_RESOLVE array
entries should match the syntax host:port:address[,address].

This creates a function-scoped associative array which uses host:port
as the key (which are supposed to be unique and this ensures that), and
the value is an array containing IP strings (ipv4 or ipv6). Once the
associative array is populated, it is then set to the CURLOPT_RESOLVE
array, imploding the ip arrays using a comma delimiter so the array
syntax matches the expected by libcurl.

Note that this reorders the "foreach ip" and "foreach port" loops.
Rather than looping over ips then ports, we now loop over ports then
ips, since ports are part of the unique host:port map, and multiple ips
can exist therein.

Signed-off-by: Aaron Ball [email protected]

[PVince81]

@nullspoon mind pointing at the place where you saw that the $options used inside a handler is a numeric non-associative array ?

the docs at https://docs.guzzlephp.org/en/stable/handlers-and-middleware.html aren't very clear about this and just say "array of request options"

this PR would result in an options array looking like this:

[
   ['curl' => 
      [CURLOPT_RESOLVE => ['entry1', 'entry2', ...]
   ],
]

which means an options array could potentially end up having multiple sets of request options from a single handler, like

[
   ['curl' => 
      [CURLOPT_RESOLVE => ['entry1', 'entry2', ...]
   ],
   ['curl' =>
      [CURLOPT_SOMETHING =>...],
   ],
]

this feels a bit strange which is why I wanted to double check the docs

[nullspoon]

whoops! You're right. Sorry about that. I took a second look through all the docs and bugs I found during research and noticed they mentioned their faq section, which didn't come up during my initial digging. See https://docs.guzzlephp.org/en/stable/faq.html#how-can-i-add-custom-curl-options

cURL offers a huge number of customizable options. While Guzzle normalizes many of these options across different handlers, there are times when you need to set custom cURL options. This can be accomplished by passing an associative array of cURL settings in the curl key of a request.

I'll refactor shortly. :)

[nullspoon]

Actually, upon further testing here, I think I might have found a red herring. Something else appears to be causing the bug. I iterrated over the $options['curl'][CURLOPT_RESOLVE] array with the original code and it indeed was populated correctly. My fault for missing that. I'll get back to troubleshooting this tomorrow when it's not so late at night.

[PVince81]

@nullspoon thanks a lot for your efforts

I still wonder why people in the original ticket have reported that this PR helped fix the issue for them ?

[nullspoon]

@nullspoon thanks a lot for your efforts

I've been using Nextcloud a long time now and and very happy to contribute back to the project if I can!

I still wonder why people in the original ticket have reported that this PR helped fix the issue for them ?

The reason why my first comment on the bug fixes things is because it removes the call to the addDnsPinning method, which is almost certainly where the issue arises. I think the reason why this PR in its current bad state also "fixes" things is because it incorrectly stores these values and so effectively unsets (well, never sets) the CURLOPT_RESOLVE configuration, so dns is never pinned, thus it cannot fail.

That said, I woke up this morning and about 10 minutes in I realized what was wrong here. In short, the way the original code is storing dns entries is overriding itself I think. The syntax defined by the libcurl docs is comma-delimited ips per host:port combination, not one ip per host:port, and thus multiple duplicate host:port entries. I'll have an actual fix in for this later today (work calls first!). 😄

[nullspoon]

Hokay! I have the fix in and tested on my server with success (actual success this time I think) 😄

Since the method of fixing is so different, I'm going to force push over my old branch so it only has the actual fix this time around.

[PVince81]

wouldn't it be much shorter to:

1. separate the local address checker to its own loop: just iterate over target IPs and check them

2. separate the ports loop and do as here in the red block: $curlEntry = $hostName . ':' . $port . ':' . implode(',', $targetIps);

at least this would require less gymnastics, unless I missed some important detail ?

[kesselb]

Index: lib/private/Http/Client/DnsPinMiddleware.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/Http/Client/DnsPinMiddleware.php b/lib/private/Http/Client/DnsPinMiddleware.php
--- a/lib/private/Http/Client/DnsPinMiddleware.php	(revision 5f8246be971f9e4fb9174dcc2066e853eb9f683e)
+++ b/lib/private/Http/Client/DnsPinMiddleware.php	(date 1625477696112)
@@ -120,14 +120,15 @@
 				}
 
 				$targetIps = $this->dnsResolve($hostName, 0);
+				$targetIps = array_unique($targetIps);
 
 				foreach ($targetIps as $ip) {
 					$this->localAddressChecker->ThrowIfLocalIp($ip);
+				}
 
-					foreach ($ports as $port) {
-						$curlEntry = $hostName . ':' . $port . ':' . $ip;
-						$options['curl'][CURLOPT_RESOLVE][] = $curlEntry;
-					}
+				foreach ($ports as $port) {
+					$curlEntry = $hostName . ':' . $port . ':' . implode(',', $targetIps);
+					$options['curl'][CURLOPT_RESOLVE][] = $curlEntry;
 				}
 
 				return $handler($request, $options);

Works for me ;) The array_unique is to filter out duplicate ip addresses. For example www.startpage.com is a cname for startpage.com. dns_get_record('www.startpage.com', DNS_A); will return the ip address even if there is no "a" record.

[LukasReschke]

✅ Requesting a website that requests a local resource via CNAME fails

\OC::$server->getHTTPClientService()->newClient()->get('http://localredirect.statuscode.ch');

✅ Requesting a website that A records a local address fails

\OC::$server->getHTTPClientService()->newClient()->get('http://localhost.statuscode.ch');

The generated CURLOPT_RESOLVE also looks reasonable in my testing.

[0]=> string(50) "google.de:80:142.251.37.3,2a00:1450:4016:80b::2003"
[1]=> string(51) "google.de:443:142.251.37.3,2a00:1450:4016:80b::2003"

[LukasReschke]

/backport to stable22

[LukasReschke]

Failing tests look unrelated.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22