Add capability of specifying "trusted_proxies" entries in CIDR notation (IPv4) #12036

olivermg · 2018-10-25T09:58:24Z

This enhances the handling of "trusted_proxies" config param. It adds the capability to define IPv4 ranges in CIDR notation (e.g. 192.168.0.0/24) instead of just single IP addresses as before.

Fixes #6550

olivermg · 2018-10-25T11:11:50Z

I may be wrong, but I don't think the check failed because of this PR's changes:

https://drone.nextcloud.com/nextcloud/server/11837/284

rullzer · 2018-10-25T15:40:40Z

Cool stuff.

Could you add ipv6 support as well. As that will now fail hard I think

kesselb · 2018-10-25T17:32:00Z

Good work! If you need some inspiration how to solve it with ipv6 you could look at https://github.com/symfony/http-foundation/blob/3.4/IpUtils.php

olivermg · 2018-10-25T17:57:31Z

Could you add ipv6 support as well. As that will now fail hard I think

Sure, I'll look into that, although I don't see a reason why it should fail right now, as the regex at
https://github.com/nextcloud/server/pull/12036/files#diff-3e4cca42108ce6a5b5592a58e1dae481R606
just won't match IPv6 addresses. Thus it would compare the unaltered values of $remoteAddress and $trustedProxy which should be the exact same behaviour as before.

Nevertheless, I'll do two things:

add a handful of tests for IPv6 addresses to RequestTest.php to validate that it doesn't currently fail for IPv6 addresses
look into whether I can implement the same functionality regarding CIDR notation for IPv6 addresses

olivermg · 2018-10-26T12:37:23Z

Could you add ipv6 support as well. As that will now fail hard I think

Okay, I think I see now. You are probably referring to that it would fail when using CIDR notation for IPv6 addresses. That's correct of course. I'm aware that the current/initial implementation only works for IPv4. I should probably have made that clear in the PR subject. My bad.

However, I'm working on adding IPv6 too, so I'll add it to this PR as soon as it's done.

lib/private/AppFramework/Http/Request.php

rullzer · 2018-10-29T19:19:55Z

@olivermg ah I see now that ipv6 proxies would have just worked just not with CIDR notation.

We could also get this is soon and then work on ipv6 CIDR later?
Or are you close to finishing ipv6?

olivermg · 2018-10-29T20:03:12Z

@olivermg ah I see now that ipv6 proxies would have just worked just not with CIDR notation.

Yeah, that is what I meant earlier. The behaviour for IPv6 addresses should not change at all with this PR (i.e. it should not introduce a regression).

We could also get this is soon and then work on ipv6 CIDR later?
Or are you close to finishing ipv6?

Sure, just go ahead and merge if that's ok for you. I'll open another PR for IPv6.

I'm working on it but it'll take a bit more effort as I'm introducing new classes for that, in order to not pollute the Request class too much with all that IP/netmask calculations stuff.

rullzer

Could you squash your commits into 1?

rullzer · 2018-10-29T21:31:58Z

tests/lib/AppFramework/Http/RequestTest.php

+
+	/*
+	 * will work after having implemented IPv6 CIDR
+	public function testGetRemoteAddressIPv6WithMatchingCidrTrustedRemote() {


Could you remove this for now?

rullzer · 2018-10-29T21:32:07Z

tests/lib/AppFramework/Http/RequestTest.php

+	}
+
+	/*
+	 * will work after having implemented IPv6 CIDR


Signed-off-by: Oliver Wegner <[email protected]>

olivermg · 2018-10-30T08:23:51Z

Sure, done.

MorrisJobke

Code makes sense, tested and works 👍

MorrisJobke · 2018-10-30T08:44:57Z

@olivermg Thanks for your contribution - feel free to have a look at other small issues in our issue tracker: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

And welcome to the community 🎉

olivermg · 2018-10-30T09:32:35Z

@olivermg Thanks for your contribution - feel free to have a look at other small issues in our issue tracker: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

And welcome to the community 🎉

Thanks a lot :-)

I'm glad I could contribute something hopefully useful.

MorrisJobke · 2018-10-30T09:49:04Z

CI failure is unrelated -> merging.

kesselb · 2018-10-30T15:03:37Z

@olivermg

server/config/config.sample.php

Lines 1499 to 1506 in 3eb5ac9

    
           /** 
        
            * List of trusted proxy servers 
        
            * 
        
            * If you configure these also consider setting `forwarded_for_headers` which 
        
            * otherwise defaults to `HTTP_X_FORWARDED_FOR` (the `X-Forwarded-For` header). 
        
            * Defaults to an empty array. 
        
            */ 
        
           'trusted_proxies' => array('203.0.113.45', '198.51.100.128'),

when you have time you could add some notes about cidr as comment. @MorrisJobke has a magic tool to generate this file https://github.com/nextcloud/documentation/blob/master/admin_manual/configuration_server/config_sample_php_parameters.rst from config.sample.php 🚀

olivermg · 2018-10-30T15:27:12Z

Yes, I'll add some samples/documentation for CIDR to that config.sample.php. Great idea 😃 .

olivermg · 2018-10-30T16:41:45Z

Regarding the documentation in config.sample.php, I've opened PR #12145

MorrisJobke · 2018-10-31T23:55:02Z

Its also not that magic. It’s a script that runs daily and updates the documentation automatically;)

rullzer reviewed Oct 26, 2018

View reviewed changes

lib/private/AppFramework/Http/Request.php Outdated Show resolved Hide resolved

olivermg force-pushed the master branch from 80d3216 to 17f2c8b Compare October 26, 2018 14:12

rullzer approved these changes Oct 29, 2018

View reviewed changes

rullzer added enhancement 3. to review Waiting for reviews labels Oct 29, 2018

rullzer modified the milestones: Nextcloud 16, Nextcloud 15 Oct 29, 2018

rullzer requested review from ChristophWurst, MorrisJobke and kesselb October 29, 2018 21:32

Adding handling of CIDR notation to trusted_proxies for IPv4

401ca28

Signed-off-by: Oliver Wegner <[email protected]>

olivermg force-pushed the master branch from 17af0be to 401ca28 Compare October 30, 2018 08:17

MorrisJobke approved these changes Oct 30, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 30, 2018

olivermg changed the title ~~Add capability of specifying "trusted_proxies" entries in CIDR notation~~ Add capability of specifying "trusted_proxies" entries in CIDR notation (IPv4) Oct 30, 2018

MorrisJobke merged commit dccfe4b into nextcloud:master Oct 30, 2018

olivermg mentioned this pull request Oct 30, 2018

Extending documentation in config.sample.php for trusted_proxies #12145

Merged

kesselb mentioned this pull request Nov 6, 2018

wrong ip stored for bruteforce behind a loadbalancer #8087

Closed

olivermg mentioned this pull request Nov 19, 2018

Implementing trusted_proxies CIDR notation capability for IPv6 #12535

Closed

kukukk mentioned this pull request May 26, 2019

Theming broken in Nextcloud 15.0.7 #15121

Closed

wiswedel mentioned this pull request Jun 11, 2019

Proxy notation hints nextcloud/documentation#1477

Merged

Uh oh!

Add capability of specifying "trusted_proxies" entries in CIDR notation (IPv4) #12036

Add capability of specifying "trusted_proxies" entries in CIDR notation (IPv4) #12036

Uh oh!

Conversation

olivermg commented Oct 25, 2018

Uh oh!

olivermg commented Oct 25, 2018

Uh oh!

rullzer commented Oct 25, 2018

Uh oh!

kesselb commented Oct 25, 2018

Uh oh!

olivermg commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivermg commented Oct 26, 2018

Uh oh!

Uh oh!

rullzer commented Oct 29, 2018

Uh oh!

olivermg commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

rullzer Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

olivermg commented Oct 30, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Oct 30, 2018

Uh oh!

olivermg commented Oct 30, 2018

Uh oh!

MorrisJobke commented Oct 30, 2018

Uh oh!

kesselb commented Oct 30, 2018

Uh oh!

olivermg commented Oct 30, 2018

Uh oh!

olivermg commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MorrisJobke commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olivermg commented Oct 25, 2018 •

edited

Loading

olivermg commented Oct 29, 2018 •

edited

Loading

olivermg commented Oct 30, 2018 •

edited

Loading