Harden remote-agent FCRDNS and basic auth identity

[somethingwithproof]

Fix helper wiring, IPv6 normalization, and untrusted forwarded-user authentication.

[somethingwithproof]

@copilot review

[Copilot]

Pull request overview

This PR hardens Cacti’s remote agent authorization and basic-auth identity handling by centralizing FCRDNS forward-confirmation logic, improving IPv6 address comparison, and removing trust in a client-supplied forwarded-user header.

Changes:

• Refactors remote_agent.php to use a shared remote_agent_fcrdns_confirmed() helper for forward-confirmation decisions.
• Normalizes IPv6 comparisons by matching addresses via inet_pton() (binary) rather than string representation.
• Removes HTTP_X_FORWARDED_USER as an authentication username source and adds regression tests for both behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/Unit/RemoteAgentFcrdnsTest.php	Adds coverage ensuring IPv6 forms compare equal via binary matching.
tests/Unit/AuthSystemCorrectnessTest.php	Adds a regression assertion that basic auth no longer references HTTP_X_FORWARDED_USER.
remote_agent.php	Replaces inline forward-record matching with the shared helper to harden FCRDNS behavior.
lib/auth.php	Removes forwarded-user header usage and updates FCRDNS helper to compare IPs by binary value.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/auth.php:275

• get_basic_auth_username() still accepts HTTP_* variables (e.g., HTTP_PHP_AUTH_USER / HTTP_REMOTE_USER / HTTP_REDIRECT_REMOTE_USER). These map to client-supplied request headers in typical CGI/FastCGI setups, so a client can spoof them and potentially bypass authentication. If the intent is to avoid trusting client-controlled identity headers, drop the HTTP_* branches and keep only PHP_AUTH_USER / REMOTE_USER / REDIRECT_REMOTE_USER.

	} elseif (isset($_SERVER['HTTP_REDIRECT_REMOTE_USER'])) {
		$raw      = is_array($_SERVER['HTTP_REDIRECT_REMOTE_USER']) ? ($_SERVER['HTTP_REDIRECT_REMOTE_USER'][0] ?? '') : $_SERVER['HTTP_REDIRECT_REMOTE_USER'];
		$username = str_replace('\\', '\\\\', $raw);
	} else {
		$username = false;

[TheWitness]

Before we remove this HTTP_X_FORWARDED_USER and the other potential exploits, we should instead add a trusted proxies to the config.php as an array of proxy servers that are allowed to append this, and then if the client is validated, then allow them to pass instead of removing as we have done a few times now. There is a good writeup/method that we can use to authenticate the client in a more trustworthy way, in other words, making sure the redirect came from an approved proxy.

[TheWitness]

This small snippet of code provides an example. Then, we should evaluate each of the basic methods against the trusted proxies.

function isTrustedProxy() {
    $trustedProxies = ['127.0.0.1', '192.168.1.100']; 

    if (isset($_SERVER['REMOTE_ADDR']) && in_array($_SERVER['REMOTE_ADDR'], $trustedProxies)) {
        if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
            // The header can be a comma-separated list (e.g., "client, proxy1, proxy2")
            $list = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
            
            // The first IP is the original client IP
            $potentialIp = trim($list[0]);

            // 2. Validate that the extracted string is an actual IP
            if (filter_var($potentialIp, FILTER_VALIDATE_IP)) {
                return true;
            }
        }
    }

    return false;
}

[somethingwithproof]

Agreed, that's the better call. A configurable trusted-proxies list with each basic-auth method validated against the source, then honoring the forwarded identity, keeps the proxy support intact without the spoofing risk that stripping the headers was working around. Thanks for the example, it's a cleaner design than what I had.

[somethingwithproof]

A couple of findings while implementing this:

• The Copilot note that remote_agent.php will fatal on remote_agent_fcrdns_confirmed() is a false positive. remote_agent.php requires include/global.php, which require_onces lib/auth.php (global.php:696) before the helper is ever called, so it is in scope at runtime.
• Implementing your trusted-proxies approach rather than stripping the headers: a $trusted_proxies list in config.php.dist (default empty) plus is_trusted_proxy() checking REMOTE_ADDR against it. get_basic_auth_username() now only honors the client-supplied forwarded headers (X-Forwarded-User, PHP-Auth-User, Remote-User, Redirect-Remote-User) when the request comes from a trusted proxy. The server-set REMOTE_USER/REDIRECT_REMOTE_USER paths are left alone so existing external auth keeps working, and an empty list means forwarded identity is ignored.
• Also switched the poller-hostname forward-lookup to reuse remote_agent_fcrdns_confirmed() so its IPv6 comparison is normalized the same way as the client PTR path (was a raw string compare).

[somethingwithproof]

Done, pushed as c2617aa. Added $trusted_proxies with is_trusted_proxy() and gated the client-supplied forwarded headers behind it, server-set REMOTE_USER is untouched. Also switched the poller forward-lookup to the FCRDNS helper for IPv6.

[somethingwithproof]

Heads up, this PR also greens a test already on develop: AuthSystemCorrectnessTest › remote agent authorization fails closed on fcrdns mismatch asserts that remote_agent.php calls remote_agent_fcrdns_confirmed($client_addr, $forward_records). The helper is defined in lib/auth.php on develop but not yet called from remote_agent.php there, so that test fails on develop today; this PR adds the call. (It isn't showing up as a red check because the PHP test job currently reports success even with failing tests in the log, worth a separate look.)

[TheWitness]

Okay, much better now.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/Unit/AuthSystemCorrectnessTest.php:108

• This new correctness test will fail because lib/auth.php now intentionally contains HTTP_X_FORWARDED_USER (it’s just gated behind is_trusted_proxy()). The assertion should validate that the header is only honored when $trusted is true, rather than asserting the string is absent.

test('basic authentication does not trust a client-supplied forwarded user header', function () use ($root) {
	$auth = file_get_contents($root . '/lib/auth.php');

	expect($auth)->not->toContain('HTTP_X_FORWARDED_USER');
});

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/Unit/AuthSystemCorrectnessTest.php:108

• This test will now fail because lib/auth.php intentionally contains HTTP_X_FORWARDED_USER handling (it’s just gated behind is_trusted_proxy()). Instead of asserting the string is absent, assert that any forwarded-user header is only honored when $trusted is true (and optionally that the ungated form is absent).

test('basic authentication does not trust a client-supplied forwarded user header', function () use ($root) {
	$auth = file_get_contents($root . '/lib/auth.php');

	expect($auth)->not->toContain('HTTP_X_FORWARDED_USER');
});

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/Unit/AuthSystemCorrectnessTest.php:108

• This new correctness test will fail because lib/auth.php now does reference HTTP_X_FORWARDED_USER (behind a trusted-proxy gate). Instead of asserting the string is absent, assert that it is only honored when $trusted is true (or drop this test since TrustedProxyAuthTest already covers the behavior).

test('basic authentication does not trust a client-supplied forwarded user header', function () use ($root) {
	$auth = file_get_contents($root . '/lib/auth.php');

	expect($auth)->not->toContain('HTTP_X_FORWARDED_USER');
});

[somethingwithproof]

Thanks. One refinement I can add if you want it: is_trusted_proxy() does an exact string compare on REMOTE_ADDR, so an IPv6 proxy written as ::1 in config wouldn't match a request arriving as 0:0:0:0:0:0:0:1 (or an IPv4-mapped ::ffff:...). I can normalize both sides with inet_pton the same way the FCRDNS path does. Happy to add it or leave exact-match as-is.

[somethingwithproof]

Added the inet_pton normalization I mentioned as a follow-up in #7363, since this one already merged. Same representation-independent matching the FCRDNS path uses.