feat(lib): Improve remote address handling and overwritecondaddr checks

[aschieweck]

Summary

This PR has two related improvements:

1. Include support for the Forward Header format as alternative to the X-Foraward-For Headers when checking for the remote address.
2. Check the overwritecondaddr regex from the config against the remote address AND the trusted proxy chain.

I ran into a similar issue as danielwbn in #6914. My remote address is always the IP of the client, which is great and shows that my reverse proxy is working, but I need to set specific overrides depending on the proxy route the request has taken.

The isOverwriteCondition() method was using the direct call to the server['REMOTE_ADDR']) instead of the getRemoteAddress() method. As both methods would need to interact with the trusted proxies and the headers, I refactored this part out into a common method. This can now also handle both styles of forward headers used by reverse proxies.

TODO

• Tests for the override
• Check contributor fluff (first PR here, I'll have to check what I'm missing)

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[aschieweck]

Hi @kesselb, any thoughts on the idea or the current state of this PR?

[kesselb]

Thanks for your pull request 👍

I'm unsure if we should have two default forwarded_for_headers.

What is your use case for overwritecondaddr?

[aschieweck]

The X-Forwared-For is still the most common style, but as the X indicates, it was experimental and is now replaced in the standard by the Forwared header. I think it's a good idea to support both in Nextcloud. I don't have a strong opinion which one, or maybe both, should be the default. This PR has both as default, has both header types are part of the standard. But this can be overwritten in the server config, should it be required. Happy to change it to what makes the most sense to you!

My use case for allowing proxies in the overwritecondaddr is the use of Cloudflare tunnels. I have the local tunnel endpoint in a docker container and I would like to change some things if the traffic is coming from this source. At the moment, overwritecondaddr is only checking the remote address reported to Nextcloud, which in the case of Cloudflare tunnels is the real IP address of the user. I might be able to write a regex if the remote address is not from the local range, but this feels more like a hack. Checking against the proxy is just cleaner.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[aschieweck]

Just merged the current state of the master branch into this one. I hope this should fix most of the checks, as they failed on some setup procedures, i.e. running composer. I haven't touched composer things in this PR, so I'm a bit lost while this wouldn't work.

[kesselb]

Just merged the current state of the master branch into this one. I hope this should fix most of the checks, as they failed on some setup procedures, i.e. running composer. I haven't touched composer things in this PR, so I'm a bit lost while this wouldn't work.

Yep, some of our CI have issues with forks.

It looks like a reasonable change. We need the ip addresse for the brute force detection, and thus it makes sense to also check the reverse proxy for the overwritecondaddr. We only take trusted proxies into account.

I would like to change some things if the traffic is coming from this source.

What would you like to change? I'm just wondering because most of the use cases for overwritecondaddr are usually look like hack. Do you set a different hostname if the request is coming through cf?

[aschieweck]

Yep, some of our CI have issues with forks.

OK, I had a look at the failing checks, and I'd say the failures are due to the CI & fork thing. Nothing related to this PR. But please let me know if there is a check, I should have a closer look at

We only take trusted proxies into account.

This PR is only looking at the remote address report and the chain of trusted proxies. Let's say we have a request with the following header information: Client - Proxy 1 - Trusted Proxy 1 - Proxy 2 - Trusted Proxy 2 - Trusted Proxy 3 - Nextcloud. Then getRemoteAddressAndProxyChain would return the Proxy 2 as remote address. As everything before Trusted Proxy 2 could be spoofed. This is similar to how it was before. But we know also get the chain of Trusted Proxy 2, Trusted Proxy 3 as chain for other checks.

Do you set a different hostname if the request is coming through cf?

That's exactly it. The cleanest way would be to have separate domain and path settings based on the upstream proxy. But that sound like a lot of work for a very niche problem. I'm happy with some jankiness and understand that this isn't a common (or even supported) use case.

[aschieweck]

Hello @nickvergessen & @Altahrim! Do you guys have any feedback on this?