New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementing trusted_proxies CIDR notation capability for IPv6 #12535

Closed

olivermg wants to merge 31 commits into master from feature/6550/trusted-proxies-ipv6-cidr

Member

olivermg commented Nov 19, 2018

This PR aims to enhance the capabilities for specifying IP addresses in CIDR notation in the trusted_proxies config param (refer to issue #6550 and PR #12036 for more info):

implement CIDR notation for IPv6
introduce an abstraction layer for representation of IP addresses/ranges and CIDR notation handling, so other classes than just Request might benefit in the future
update config.sample.php to reflect the new capabilities

olivermg added 21 commits

October 31, 2018 22:21


          Adding IpAddress handling utility classes in OC\Net namespace

4dd4379

- IIpAddress
- AbstractIpAddress
- IpAddressV4
- IpAddressV6
- IpAddressFactory

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          Working on IpAddress classes for CIDR

5efc12f

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          Merge branch 'master' of github.com:nextcloud/server into feature/655…

54c3694

…0/trusted-proxies-ipv6-cidr


          Adding test for IpAddressFactory

9c98e43

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding tests to IpAddressV4Test.php

fc7b744

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          Merge branch 'master' of github.com:nextcloud/server into feature/655…

9674d3b

…0/trusted-proxies-ipv6-cidr


          adding tests for IpAddressV4

7b9f5bc

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          renaming tests

587025a

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding tests for IpAddressV6

8d35b0e

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding tests for localhost

5fa3396

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          moving common methods from IPAddressV[46] to AbstractIpAddress

af610cd

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding author to Request.php

a5503ad

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          removing empty setUp & tearDown methods from tests

cf554e0

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          shrinking IIpAddress interface to sensible minimum

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          Merge branch 'master' of github.com:nextcloud/server into feature/655…

b26e862

…0/trusted-proxies-ipv6-cidr


          remove matchesTrustedProxy function from Request.php

b4a5d8f

as this is now being done by classes in OC\Net

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding license headers to new source files

02c6ed1

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          updating config.sample.php to reflect IPv6 CIDR notation

ad76224

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding function header comments

361871d

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          adding license file headers to test files

efc02c6

Signed-off-by: Oliver Wegner <void1976@gmail.com>


          Merge branch 'master' of github.com:nextcloud/server into feature/655…

29488ec

…0/trusted-proxies-ipv6-cidr

Contributor

kesselb commented Nov 19, 2018

@olivermg, thank you for your great contribution 👍

kesselb added the 3. to review label

kesselb added this to the Nextcloud 16 milestone

kesselb added the enhancement label

kesselb reviewed

View reviewed changes

lib/private/Net/AbstractIpAddress.php Outdated Show resolved Hide resolved

kesselb reviewed

View reviewed changes

lib/private/Net/IIpAddress.php Outdated Show resolved Hide resolved

kesselb requested review from rullzer, MorrisJobke and nickvergessen

November 19, 2018 17:09


          introduce IIpAddressFactory and move IpAddressFactory to OC namespace

f82f4fb

Signed-off-by: Oliver Wegner <void1976@gmail.com>

olivermg force-pushed the feature/6550/trusted-proxies-ipv6-cidr branch from 8f73159 to f82f4fb Compare

November 25, 2018 11:03

olivermg added 4 commits

November 27, 2018 17:26


          Merge branch 'master' of github.com:olivermg/server into feature/6550…

733b416

…/trusted-proxies-ipv6-cidr


          Merge branch 'master' of github.com:nextcloud/server into feature/655…

638d876

…0/trusted-proxies-ipv6-cidr


          inject IIpAddressFactory into Request

bc413ea


          check if an IIpAddressFactory exists in Request

a8f7ea4

ChristophWurst reviewed

View reviewed changes

lib/private/AppFramework/Http/Request.php

@@ @@ -136,12 +140,14 @@ public function __construct(array $vars= [], @@
               								ISecureRandom $secureRandom = null,
               								IConfig $config,
               								CsrfTokenManager $csrfTokenManager = null,
-              								string $stream = 'php://input') {
+              								string $stream = 'php://input',
+              								IIpAddressFactory $ipAddressFactory = null) {

Member

ChristophWurst Nov 27, 2018

Any particular reason why this is nullable?

Member Author

olivermg Nov 27, 2018

Not from an application logic point of view and I really don't like it this way. However, making it a mandatory argument will force me to touch quite a few lines of code elsewhere (most in tests though).

$ egrep -rn 'new ([\\[:alnum:]]+\\)?Request[^[:alnum:]]' apps tests lib contribute ocs* ocm-provider settings resources themes config core | grep -v Sabre | wc -l
96

I had initially been happy that there were only two locations in the code that really instantiate Request, but I had not found all of them in production code (e.g. in base.php) and had also not taken the tests into account.

It'll take some time for me to put that additional effort into this.

Member

ChristophWurst Nov 28, 2018

It'll take some time for me to put that additional effort into this.

Unfortunately this is the correct way to do this. Otherwise this is bound to fail someone in the future that assumes the argument is optional.

kesselb mentioned this pull request

WIP: Make trusted_proxies configuration IPv6 compatible #13379

Closed

johanfleury reviewed

View reviewed changes

lib/private/Net/IpAddressV4.php

+              	 * @return string
+              	 */
+              	protected function getCidrRegex(): string {
+              		return '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';

johanfleury Jan 5, 2019

This regex matches invalid IPv4 subnets: https://regex101.com/r/tjtE95/1 (same for IPv6 in the IpAddressV6 class).

As IP address are numbers, I think you could use numerical comparison and avoid relying on regexes (that will most certainly be incorrect in a way or another). I'm not really into PHP so I'm not sure what's the best way to do that (to be honest it bugs me that this is not part of the standard library).

Also, I think it would be useful to warn the administrator when they put a wrong value in the configuration (thus using another "parsing" method).

This was referenced Mar 4, 2019

16.0.0 Alpha 1 #14501

Merged

16.0.0 Beta 1 #14573

Merged

MorrisJobke modified the milestones: Nextcloud 16, Nextcloud 17

MorrisJobke mentioned this pull request

17.0.0 Beta 1 #16408

Merged

28 tasks

Contributor

kesselb commented Jul 15, 2019

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).

@ChristophWurst what should we do? 🤔
@MorrisJobke looks like Nextcloud 18 to me 🙈

MorrisJobke modified the milestones: Nextcloud 17, Nextcloud 18

Member

ChristophWurst commented Oct 24, 2019

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).

I can second that.

rullzer modified the milestones: Nextcloud 18, Nextcloud 19

This was referenced Apr 4, 2020

19 beta 2 #20303

Merged

19 beta 3 #20386

Merged

Member

ChristophWurst commented Apr 15, 2020

Due to lack of activity I will close this PR for now.

ChristophWurst closed this

ChristophWurst removed this from the Nextcloud 19 milestone

ChristophWurst added 0. Needs triage and removed 2. developing labels

MorrisJobke deleted the feature/6550/trusted-proxies-ipv6-cidr branch

April 15, 2020 11:00

kesselb mentioned this pull request

Support specifying IPv6 proxies in CIDR notation #32615

Merged

come-nc mentioned this pull request

Add symfony/http-foundation dependency nextcloud/3rdparty#1102

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

johanfleury johanfleury left review comments

kesselb kesselb left review comments

ChristophWurst ChristophWurst requested changes

rullzer Awaiting requested review from rullzer

MorrisJobke Awaiting requested review from MorrisJobke

nickvergessen Awaiting requested review from nickvergessen

Labels

0. Needs triage enhancement