[somethingwithproof]

Description

The Nagios host import decides whether a Nagios host already exists in Cacti by comparing addresses. It built its map of Cacti devices with gethostbyname(), which only returns IPv4, and compared the imported address as plain text. A device that only has an IPv6 address never matched, so the import tried to add it a second time. A device with both kinds was only known by its first IPv4 address, and the same IPv6 address written two ways (2001:DB8::1 and 2001:db8:0:0:0:0:0:1) did not match itself.

This change keeps the same cache file and the same lookup flow and makes the address handling correct for IPv6:

• A host name is looked up for A and AAAA records with dns_get_record(). If the resolver returns no records, the system lookup that earlier versions used (gethostbyname()) is tried, so names that resolved before still resolve.
• An IPv4 or IPv6 literal, including one in brackets or with a scope id, is used as it is, without a lookup.
• Every address is stored and compared in one canonical text form (inet_pton() then inet_ntop()): 2001:DB8::1, 2001:db8:0:0:0:0:0:1 and [2001:db8::1] are the same address, and an IPv4-mapped IPv6 address (::ffff:192.0.2.10) is the IPv4 address.
• All addresses of a device are kept, not one. The address being imported is resolved the same way, so a Nagios address that is a host name also matches.
• The cache file keeps its layout ($cacheKey and $hosts). It gains $cacheVersion = 2. A file written by an earlier version has no version and is rebuilt once, because it only holds the first IPv4 address of each device. An earlier version reading a new file ignores the extra variable.

checkHostExists() also called NpcCactiController::getHostnames() statically, which is an Error on PHP 8 (the plugin's floor is 8.2), so the method could not run at all. It now calls the method on an instance.

Related Issue

No open issue.

Motivation and Context

Dual-stack and IPv6-only monitoring is common. With the old map the import duplicated every Cacti device that was not reachable through an IPv4 name, and it could not recognise an IPv6 address that Nagios and Cacti wrote differently.

Other places that handle addresses were checked and need no change: npc_hosts.address is varchar(128) and holds a full IPv6 address (at most 45 characters); the controllers only select and return the column; js/npc.js shows it as text; there is no IPv4 pattern anywhere in the plugin; cli.php and perfdata.php print it with a fixed column width of 30, which is cosmetic for the longest IPv6 text and was left alone because those files cannot be covered by the unit tests. The import passes the address to cli/add_device.php --ip=, which accepts IPv6 in Cacti.

Scope and risk

• Scope: controllers/sync.php (address handling, checkHostExists()), the unit bootstrap, phpunit.xml (measure controllers/sync.php), tests, CHANGELOG.
• Compatibility: backward compatible. The cache path, layout and key are unchanged; an old file is rebuilt once. The method signatures are unchanged. Two new public helpers (normalizeAddress(), resolveAddresses()) and two overridable lookups (dnsRecords(), legacyLookup()) are added.
• Behaviour change to be aware of: when two Cacti devices share an address, the later device wins, as before. When a name has several records, all of them map to the device.
• The cache still lives in /tmp/npc_address_cache.php and is still read with include. That is a separate problem and is not changed here; the hardening PR (Harden the NPC controllers and fix review findings #54) replaces it. Both PRs change checkHostExists(), so the second one merged needs a rebase of that method.
• Not a security change.

Rollback

Revert the merge commit. A cache file written by this version has a different set of keys; delete /tmp/npc_address_cache.php after a revert, or let the next import rebuild it.

How Has This Been Tested?

tests/Unit/SyncAddressMapTest.php (56 cases). DNS is replaced by closures, so no test sends a query and every result is deterministic.

• Canonical form: IPv4, IPv4 with spaces, IPv6 compressed, upper case, fully expanded, partly expanded, in brackets, with a scope id, loopback, unspecified, IPv4-mapped (dotted and hex); host name, empty, null, octet out of range, leading-zero octet, bad hex digit, too many groups, brackets around a name, only brackets.
• Lookups: a literal needs no lookup; a name with A and AAAA records returns each address once in canonical form; A only; AAAA only; records without a usable address are ignored; no records falls back to the system lookup; a name that does not resolve and an empty name return nothing without a query; the record types passed to DNS.
• Map: devices with both kinds, IPv6 only, an IPv4 literal host name, a bracketed IPv6 literal host name, an unresolvable device, a device found by the system lookup; the imported address as IPv4, IPv6 in four spellings, a host name, an unknown address, an unresolvable name, empty; the last of two devices sharing an address wins.
• Cache: the file layout and version marker, a current file is read without asking Cacti or DNS, a file with another key or without a version is rebuilt, and a file written by an earlier version of the same key still answers IPv4 lookups through the new reader.

Executed locally (PHP 8.4, Pest through a Docker image with Xdebug, MariaDB 11.8.9 in a throwaway container) in the order of the CI workflow on two Cacti trees, a 1.2.x tree (1.2.32) and a develop tree (1.3.0):

Core ref	Install	php -l	Pest	Patch coverage	i18n and manifest
1.2.32	plugin_manage --install --enable clean	clean	154 passed, 1 skipped	100.00 percent (41 of 41)	pass
develop 1.3.0	clean	clean	154 passed, 1 skipped	100.00 percent (41 of 41)	pass

The skipped test is a root-user permission check. All 56 new tests fail against the unchanged controllers/sync.php. The Pest and PHPUnit versions come from the develop tree's lock file; a fresh composer install for 1.2.x and PHP 8.2 and 8.3 were not run locally. The tests use no eval, no http:// literal, no SQL and no network, and write only to temporary files that they remove.

Performance

Building the map now asks for A and AAAA records of each Cacti device with one dns_get_record() call instead of one gethostbyname() call, which is one extra query per device name when the resolver answers. The map is built once per cache key, not per import request. I did not measure it: the local setup has no resolver whose latency would mean anything. Reading the cache is unchanged apart from normalising the imported address, which is a few string operations.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation. (CHANGELOG)
• I have updated the documentation repository accordingly.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud