Support specifying IPv6 proxies in CIDR notation

[sleiner]

Previously, it was not possible to use CIDR notation for IPv6 proxies in the trusted_proxies parameter of config.php. This patch adds support for that, mainly by relying on inet_pton and a custom bitwise string comparison function instead of ip2long (which does not work for IPv6 because of its 128 bit address space).

It is worth noting that the bitwise comparison could be implemented a bit more straightforward using GMP, but since Nextcloud does not currently require GMP to be available, this alternative implementation was chosen instead.

The patch fixes issue #32253.

[kesselb]

Hey @sleiner 👋,

thank you very much for sending a pull request 👍

We had a pull request a while ago to add IPv6 CIDR support to trusted proxies. Maybe some of the tests and/or discussion are helpful for this pr: #12535

I am unsure about this pr. The way we implement the logic looks quite complex compared to https://github.com/symfony/http-foundation/blob/master/IpUtils.php. There is only a little chance that his logic (is ipv4 or is ipv6) changes in the near future (e.g. add ipvX).

Your pr looks good to me. Yet I have the same feeling as for the other pr 🙈

I believe we should pull in/copy IpUtils (including tests) from Symfony and adapt our code to it and not reinvent the wheel. Let me request some reviews to get more opinions on this one.

[sleiner]

Hi @kesselb,

I had not seen #12535 before, thanks for pointing me to that! 😊 After taking a brief look at it, here are my thoughts:

• The test cases for the Request class are basically the same as far as I see (except for some some of the concrete IP addresses and the extra constructor parameters introduced in Implementing trusted_proxies CIDR notation capability for IPv6 #12535). IMO, that's a a good sign.
• Compared to Implementing trusted_proxies CIDR notation capability for IPv6 #12535, this implementation is much simpler:
  • No new classes, just two more helper methods
  • No custom-built detection of IPv4 vs. IPv6 addresses, just the use of inet_pton
  • No regexes validating IP addresses, just the use of inet_pton
• Still, there are two places were this patch implements custom logic:
  • Splitting a string possibly containing a CIDR representation up into the net address and the prefix length: This just splits the string by the / character if there is one (same as Symfony's IpUtils).
  • Detecting whether two packed strings have the same prefix with a variable length of bits: Since I did not want to rely on GMP being available, this involves some custom fiddling with bits. Symfony's IpUtils do the same for IPv6, although their implementation differs quite a bit from mine.
• Compared to my implementation, the Symfony one has a few advantages:
  • It has been used in production before.
  • It may perform better: For my implementation, I can say that I have not benchmarked it. In any case, the symfony implementation uses a cache while mine does not. This may be beneficial.

In general, I have no strong attachment to the specific code of this patch, I mainly would like to use the feature ;-) I have never contributed to Nextcloud (or written any PHP, for that matter) before so I had no idea what your policies are concerning pulling in code from external libraries. So just writing the code myself seemed like the quickest way to me. So let me know what you and your colleagues consider the best way forward here 👍🏻

[sleiner]

@kesselb could you please approve running the workflows again? 😅

[sleiner]

@kesselb Can you give an ETA for the reviews?

[skjnldsv]

Code looks good

[come-nc]

@sleiner Hello, in the end we added symfony/http-foundation as a dependency because we needed the same functionnality in other parts of the code and felt it was better to rely on symfony rather than maintain our own copy.
Could you refactor this PR to use Symfony\Component\HttpFoundation\IpUtils instead? (rebase on master first)

I could do that in an other PR but I think your tests can work the same and this is valuable.

[sleiner]

Sure thing 👍🏻 (though it will probably take a few days before I'll get to it)

[sleiner]

@come-nc done 😊

[come-nc]

@sleiner Can you rebase on master to retrigger CI?
It feels like the failures are unrelated.

[sleiner]

@come-nc done 👍🏻

[sleiner]

we still have some failing runs :/

• The failing performance test seems like a general issue with pull requests from forked repositories to me.
• The drone build fails on a timeout (for a notification in the files app), so this is probably a flaky test?

[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

[come-nc]

we still have some failing runs :/

• The failing performance test seems like a general issue with pull requests from forked repositories to me.
• The drone build fails on a timeout (for a notification in the files app), so this is probably a flaky test?

Yeah seems unrelated, I merged it.