Skip to content
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.

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

Fix display of remote ip in php log #292

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

ne20002
Copy link
Collaborator

@ne20002 ne20002 commented Feb 10, 2025

Based on #289 I create this PR to collect the changes needed to ensure that the original requesters ip is used for logging, etc.

This is what is to be done now:

  • @m33m33 Looking in Respect Forwarded-For headers friendica#11680 I believe logging the correct ip should be possible already if the Friendica configuration is set accordingly. We need to figure out how to add this correctly to the docker configuration. We may also need to add another optional env variable with a proper default for this.

  • For the Apache image the mod_remoteip needs to be adjusted.

  • We need to document how this is working

Co-authored-by: @m33m33

@m33m33
Copy link

m33m33 commented Feb 12, 2025

Looking at code comited during #289 it seems that the idea is to use X-Forwarded-For header when a trusted_proxies populate this header.
See here and there.

It will be perfect for a bare metal deployment, and this is probably what @nupplaphil had in mind.

But... that won't do with dockerized deployments, as reverse proxies will have some random, changing IP.

Options that comes to mind:

  • hint admins to add the whole friendica pod/network/whatever subnet in trusted_proxies. Problem is it won't work out of the box, I don't like that.
  • set it to docker's default network subnet 172.16.0.0/16. It's a private subnet, supposed to be tied to friendica docker internal network, shouldn't expose to much risks,

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

I'm not sure how the best way for the logging of pgp access log is. I'm currently looking at $SERVER(REMOTE_ADDR).

When looking in php info of my 2024.12-fpm it shows the correct value for the client.
It is not recommended to use X-Forwarded-For directly as it can be spoofed.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

So, why not set the trusted_proxies default value to RFC1918 private networks by default, and hint the admin to restrict that if they feel like it.

It would work out of the box, minimized the development effort, and it matches the current apache's mod_remoteip configuration, which have me thinking that it was the original idea.

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

The apache image without fpm does not have problems with logging. Or am I'm missing something?
It works with mod_remoteip when properly configured. Setting the private networks as trusted proxy is the way it's usually done.
Options to improve this would be to add an optional env variable to the container to be set by the admin.
But for unusual cases an admin can always use its own config file and mount it into the container.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

Yes, X-Forwarded-For can be spoofed. However, admins do control their own reverse proxy, we can hint them to actively clear X-Forwarded-For headers received from the Internet, and add it on their own trusted reverse proxy.

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

For the php access log I'm struggling with how to do it properly. Just using the Forwarded-For header is no option.
It might be spoofed or even worse... there wouldn't be any validation on that.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

The apache image without fpm does not have problems with logging. Or am I'm missing something? It works with mod_remoteip when properly configured. Setting the private networks as trusted proxy is the way it's usually done. Options to improve this would be to add an optional env variable to the container to be set by the admin. But for unusual cases an admin can always use its own config file and mount it into the container.

Currently apache image doesn't log real client IP. To my understanding, everything is ready to have it working, if trusted_proxy is set (currently blank). Or at least it seems to be the idea from priveious PR about that issue. Relying on X-Real-IP header instead.

With that said, neither are bullet proof, at some point admins should do their chores and satinize headers from untrusted networks (internet, IPS proxies), and rely on what they actually own and control: the front facing reverse proxy in friendica docker's pod.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

I'm not sure how the best way for the logging of pgp access log is. I'm currently looking at $SERVER(REMOTE_ADDR).

pgp ?

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

Yes, X-Forwarded-For can be spoofed. However, admins do control their own reverse proxy, we can hint them to actively clear X-Forwarded-For headers received from the Internet, and add it on their own trusted reverse proxy.

I wouldn't assume that Friendca admins are experienced Linux admins. There are hobby users using docker with copied docker-compose files out there because it's easy and they don't know how to properly install software or what else is needed for a professional setup ( reverse proxy, WAF, ids, ...).

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

I'm not sure how the best way for the logging of pgp access log is. I'm currently looking at $SERVER(REMOTE_ADDR).

pgp ?

Php. Autoknacker

@m33m33
Copy link

m33m33 commented Feb 12, 2025

For the php access log I'm struggling with how to do it properly. Just using the Forwarded-For header is no option. It might be spoofed or even worse... there wouldn't be any validation on that.

Sorry I fail to see how it could hurt to use X-Forwarded-For or X-Real-IP in logs and logs only, like in apache access_log LogFormat and fpm access.format, with precautions on the trusted reverse proxy as I suggested up there.

Is it possible that you're taking that particular case too far by using it deep in friendica's php code ? in places that, yes, it must not be trusted ?

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

I'm pretty sure Apache image will log with the changes above. And it already logged correctly when the reverse proxy set X-Real-Ip.
As you said, it all is based on what is configured on the reverse proxy.
Still, as far as I see

  • we have a solution for the apache image.
  • there is a solution in the Friendica code (but lacking ipv6)
  • plain php access log is the open issue.
    But I need more digging into it. What I've found $SERVER(REMOTE_ADDR) is usually recommended but seem to have its own drawbacks.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

Thanks for the clarification, I think I understand now why you are digging into php code, for the "plain php access log" to me it means "php-fpm access format"

That's probably me being more admin that dev, but I wouldn't add more complexity to the code, and rather use php-fpm's access format to get X-Real-IP like in apache images for instance, and write that in logs.

@ne20002
Copy link
Collaborator Author

ne20002 commented Feb 12, 2025

As a dev I'm looking for the correct solution rather than the first coming in mind.
A http header is kind of an input and should not be used without proper validation. So I take a closer look on the matter.

I want a simple but secure solution that will work out of the box and does what is expected. This is not about printing X-Forward-For but getting the correct remote address and use it (for logging). And for different scenarios as otherwise the next user will complain soon that it is not working for him.

@m33m33
Copy link

m33m33 commented Feb 12, 2025

That's the correct way to do it indeed.

Until then I will rely on X-* sanitized headers on my instance, because logging the local ip of the reverse proxy is a no go for me.

In the end of the day if friendica app show the real client IP in logs I'm happy with it.

@ne20002 ne20002 force-pushed the feat/forwarded-for-header branch from 4b7b2dd to f4ff69c Compare February 21, 2025 15:04
@ne20002 ne20002 changed the title Fix handling of X-Forwarded-For header Fix display of remote ip in php log Feb 21, 2025
@ne20002 ne20002 force-pushed the feat/forwarded-for-header branch from f4ff69c to 1cc0200 Compare February 21, 2025 15:07
@ne20002 ne20002 marked this pull request as ready for review February 21, 2025 15:14
@ne20002 ne20002 requested a review from nupplaphil February 21, 2025 15:15
Copy link
Collaborator

@MrPetovan MrPetovan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@MrPetovan MrPetovan merged commit ae32b83 into friendica:stable Feb 21, 2025
19 checks passed
@ne20002 ne20002 deleted the feat/forwarded-for-header branch February 21, 2025 18:04
@ne20002 ne20002 restored the feat/forwarded-for-header branch February 21, 2025 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants