Skip to content

Query NTP over IPv6 and tidy panel leftovers (stacked on #432) - #434

Open
somethingwithproof wants to merge 6 commits into
fix/hardening-review-fixesfrom
fix/ntp-ipv6-and-panel-leftovers
Open

somethingwithproof wants to merge 6 commits into
fix/hardening-review-fixesfrom
fix/ntp-ipv6-and-panel-leftovers

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Title

Fix NTP checks for IPv6 servers and tidy four leftovers in the panel code

Description

Stacked on #432: the base branch is fix/hardening-review-fixes and this PR must merge after #432. Once #432 is merged, retarget it to develop.

Four small fixes, one commit each, with tests, plus one commit that adjusts #432's end-to-end tests (the first favourite no longer needs a seeded panel, and the deleted-user poller row is now skipped and removed instead of logged).

NTP over IPv6. ntp_time() opened an AF_INET socket, so an IPv6 server failed even though the settings check accepts it, and a name that only has an AAAA record could not be used. It now:

  • uses an IPv4 or IPv6 address as it is, and accepts an IPv6 address in brackets (the settings check lets [2001:db8::1] through as a domain name)
  • resolves a name with dns_get_record() for AAAA and A records, and asks gethostbyname() as well, so a name that only exists in the hosts file still works
  • tries the first IPv6 address, then the first IPv4 address, each with the existing five second timeout
  • keeps the return contract: an integer Unix timestamp, or a string starting with error:
  • talks UDP through PHP streams instead of the sockets extension, so the check no longer needs that extension

ntp_time() gained optional parameters (port, timeout, resolver) so the tests can run it against a UDP server on the loopback address and replace the DNS lookup. Callers are unchanged.

Old panel registration functions. intropage_add_panel() wrote file, has_detail and refresh_interval into plugin_intropage_panel_definition and added a column to plugin_intropage_user_auth, none of which exist any more, and intropage_remove_panel() ran ALTER TABLE ... DROP ?, which cannot bind an identifier. Nothing in the repository calls them and panellib/README.md describes register_<name>() as the way to add panels. The names stay for any plugin that still calls them. Both now log a deprecation warning with cacti_log() and return false (the declared return type changes from string to bool).

Favourite graphs. The priority of a new favourite was MAX(priority) + 1 over the user's panels, which is NULL for a user without panel rows, so the insert failed. It is now COALESCE(MAX(priority), 0) + 1.

Deleted users. The poller selected every due panel row and ran the panel code for it. For a row whose user no longer exists, Cacti logs AUTH ERROR: Invalid Cacti User ID ... is being used in a permission that does not exist on every cycle. The poller query now leaves out rows of users that are not in user_auth (system rows with user 0 and rows without an owner are kept), and each cycle deletes dashboard links and panel data rows of such users, at most 500 per table per cycle. The delete does nothing when user_auth has no rows, in case the user table cannot be read. The user_remove hook already does this when the plugin is active at the time a user is deleted; this covers the rows it missed.

Reviewed and left alone: the other panel code that handles addresses or names (panellib/*) only displays values from the database or from the syslog plugin, or uses cacti_gethostinfo() with DNS_AAAA already included for the DNS check. The one IPv4-sized value is the syslog panels cutting the device address to 37 characters for display; an IPv6 address can be 39, so the last characters of a long one are not shown.

Related Issue

No tracking issue. These come from a code review of develop.

Motivation and Context

IPv6-only NTP servers and pool names with AAAA records are common. The other three are leftovers from a review of the panel code: a public function that cannot work, a first-use failure, and a log line every minute for deleted users.

How Has This Been Tested?

Unit tests (Pest, the repository's stub bootstrap; tests/bootstrap-unit.php is the same file as on the hardening branch so the two can merge in either order): 109 passed, 1 skipped, 1 warning (the existing rmdir notice). tests/bin/patch-coverage.php against origin/develop: 100.00 percent (60/60 changed lines; poller_intropage.php is on its allowlist and its two changed statements call functions that are covered). check-i18n-pot.php, validate-manifest.php and php -l on every PHP file pass.

NTP tests: IPv4 literal, IPv6 literal (full, compressed, loopback), IPv6 in brackets with and without spaces, a name with only an A record, only an AAAA record and both (IPv6 tried first, only the first of each family used), a name that does not resolve, a server that does not answer (one second timeout, checked to take at least 0.9 s), a short answer, something that is not an address, and a fallback from an IPv6 address with no listener to an IPv4 server. The network tests talk to a one-shot UDP server that a child PHP process starts on 127.0.0.1 or ::1; the IPv6 tests skip themselves where ::1 cannot be bound. They passed here, so they ran.

Run in the order of the workflow on both cores, PHP 8.3, MariaDB 11.8.9 in Docker: install and enable with plugin_manage, full Pest run, poller.php --force, scan of cacti.log.

Core Pest Patch coverage Poller FATAL:|ERROR:|WARNING: in cacti.log
1.2.x (1.2.32) 109 passed, 1 skipped, 1 warning 100.00 percent exit 0, SYSTEM STATS and INTROPAGE STATS logged none
develop (1.3.0 dev) 109 passed, 1 skipped, 1 warning 100.00 percent exit 0, same none

Real database check of the new SQL, on both cores: with a panel row and a dashboard link for user 424242 who does not exist, a row for user 1, and the system rows present, intropage_poller_due_panels() returned no row for 424242 and one for user 1; after intropage_purge_orphan_rows() the rows and the link of 424242 were gone and the row of user 1 and all 13 system rows were still there. Running poller_intropage.php with the orphan row logged Invalid Cacti User ID 424242 once on origin/develop and not at all on this branch.

Not tested: a real NTP server on the internet or over a real IPv6 route (the tests use the loopback), PHP 8.0 to 8.2 and 8.4, the workflow itself, and the settings page. The change from five seconds to up to ten seconds per attempt (two addresses) means the panel can take up to 30 seconds in the poller when a server does not answer on either family, instead of 15.

Rollback

Revert the merge commit. No schema or data migration. Rows deleted by the orphan cleanup belong to users who are not in user_auth and cannot be restored from the plugin tables.

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.
  • I have updated the documentation repository accordingly.

The check opened an IPv4 socket, so IPv6 servers and names that only have AAAA records failed.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
MAX(priority) is NULL for a user without panels, so the first favourite could not be stored.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
They wrote to columns that no longer exist and ran ALTER TABLE with a bound identifier. They now log a warning and return false.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Cacti logged an authentication error for each such row on every cycle.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… the end-to-end tests

The favourite test no longer seeds a panel first, and the deleted user row is now skipped and removed instead of logged.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

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.

1 participant