Ensure that if present the HTTP_X_FORWARDED_FOR IP address is used in…

[seamuslee001]

…stead of the SERVER_ADDR when logging items from the IDS

Overview

This ensures that if present we use the HTTP_X_FORWARDED_FOR address as that is generally more accurate if behind a load balancer than SERVER_ADDR

Before

the SERVER_ADDR is always used even if behind a load balancer

After

The SERVER_ADDR is used only if we can't use HTTP_X_FORWARDED_FOR

https://www.chriswiegman.com/2014/05/getting-correct-ip-address-php

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

(1) Isn't there a bit of a snag with X-Forwarded-For?

• If the httpd is behind a firewall, then do use X-Forwarded-For.
• If the httpd is not behind a firewall, then do not use X-Forwarded-For (even if it's given - because it can be given fraudulently).

For example, in Drupal, settings.php exposes a bunch of reverse_proxy settings -- I think that's because you can't determine the "true" IP without knowing more context about the deployment environment.

(2) OTOH... For purposes of CRM_Core_IDS::log()... maybe we don't need the "true" IP? Logs provide data for future forensics, and IDS::log() is specifically for suspicious requests. If the request is suspicious, then recording both SERVER_ADDR and HTTP_X_FORWARDED_FOR might be better support for those future forensics?

[seamuslee001]

@totten maybe looking at the IDS it uses REMOTE_ADDR instead of SERVER_ADDR (which actually seems to make more sense) and also appends the HTTP_X_FORWARD_FOR if present https://github.com/civicrm/civicrm-packages/blob/master/IDS/Log/Email.php#L167

[totten]

@seamuslee001 oh, yeah, that is a nice pattern. Seems sensible for CRM_Core_IDS::log() to use the same IP pattern as IDS_Log_Email.

[seamuslee001]

@totten have updated now