Repository navigation
Query NTP over IPv6 and tidy panel leftovers (stacked on #432) - #434
Open
somethingwithproof wants to merge 6 commits into
Open
somethingwithproof wants to merge 6 commits into
somethingwithproof wants to merge 6 commits into
Conversation
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>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



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-fixesand this PR must merge after #432. Once #432 is merged, retarget it todevelop.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 anAF_INETsocket, 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:[2001:db8::1]through as a domain name)dns_get_record()for AAAA and A records, and asksgethostbyname()as well, so a name that only exists in the hosts file still workserror: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()wrotefile,has_detailandrefresh_intervalintoplugin_intropage_panel_definitionand added a column toplugin_intropage_user_auth, none of which exist any more, andintropage_remove_panel()ranALTER TABLE ... DROP ?, which cannot bind an identifier. Nothing in the repository calls them andpanellib/README.mddescribesregister_<name>()as the way to add panels. The names stay for any plugin that still calls them. Both now log a deprecation warning withcacti_log()and returnfalse(the declared return type changes fromstringtobool).Favourite graphs. The priority of a new favourite was
MAX(priority) + 1over the user's panels, which is NULL for a user without panel rows, so the insert failed. It is nowCOALESCE(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 existon every cycle. The poller query now leaves out rows of users that are not inuser_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 whenuser_authhas no rows, in case the user table cannot be read. Theuser_removehook 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 usescacti_gethostinfo()withDNS_AAAAalready 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.phpis the same file as on the hardening branch so the two can merge in either order): 109 passed, 1 skipped, 1 warning (the existingrmdirnotice).tests/bin/patch-coverage.phpagainst 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.phpandphp -lon 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.1or::1; the IPv6 tests skip themselves where::1cannot 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.FATAL:|ERROR:|WARNING:in cacti.logReal 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; afterintropage_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. Runningpoller_intropage.phpwith the orphan row loggedInvalid Cacti User ID 424242once 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_authand cannot be restored from the plugin tables.Types of changes
Checklist