Repository navigation
issue#109: optional per-device config diff in the backup email - #200
TheWitness merged 13 commits into
Conversation
c1840f5 to
311a314
Compare
311a314 to
72a5d43
Compare
somethingwithproof
left a comment
There was a problem hiding this comment.
Blocker: the Thold recipient list setting never appears. The code checks for plugin_thold_notification_lists, but Thold creates plugin_notification_lists (see includes/database.php in plugin_thold). db_table_exists() is always false, so the setting never renders and plugin_routerconfigs_notification_recipients() never adds the list's addresses. This option also isn't part of #109; it would be easier to review in its own PR, or at least listed in the description.
Suggestion: emailed diffs will carry secrets. A changed enable secret, snmp-server community, TACACS/RADIUS key, user password hash or crypto key all show up in the diff. The diff then goes to everyone on the email, including a notification list. The feature is off by default, but the setting's description should warn about this, and lines matching password, secret, community or key should be redacted by default (for example with built-in exclude patterns, or by masking the value).
Nit: plugin_routerconfigs_diff_exclude_lines() escapes / with str_replace(), so a pattern that already contains \/ becomes \\/ and is silently dropped as invalid. Also, the setting description says patterns are matched "against whole lines", but they aren't anchored, so they match any part of a line.
Nit: the db_table_exists() check at the top level of arrays.php runs a query every time the file is included.
SonarCloud duplicated-lines failure (5.4%): all 28 lines are in tests/Unit/RouterconfigsDiffEmailTest.php. The two-backups test and the binary-backup test repeat the same setup: temp directory, new.cfg/old.cfg contents and the db_fetch_assoc_prepared override. Moving that setup into one helper should clear the gate. For example:
function rc_diff_backups(string $new, string $old): string {
$dir = sys_get_temp_dir() . '/rc-diff-' . uniqid();
mkdir($dir, 0777, true);
file_put_contents($dir . '/new.cfg', $new);
file_put_contents($dir . '/old.cfg', $old);
$GLOBALS['__stub_overrides']['db_fetch_assoc_prepared'] = fn ($sql, $params) => [
['directory' => $dir, 'filename' => 'new.cfg'],
['directory' => $dir, 'filename' => 'old.cfg'],
];
return $dir;
}The cleanup (unlink/rmdir) could move into an afterEach(), which would also remove the leftover temp files when an assertion fails partway through.
…Email tests Moves the repeated temp directory, backup file and db_fetch_assoc_prepared stub setup into two helpers, and removes temp directories in afterEach() so a failed assertion no longer leaves them behind. Clears the SonarCloud duplicated-lines gate on this PR. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Pushed 3b09ff6 to clear the SonarCloud duplicated-lines gate. It only touches
The test cases and assertions are unchanged. SonarCloud passes on this head. The plugin CI only runs for PRs targeting The review points above still stand. |
…sue-109-diff-email Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…in emailed diffs The Thold Notification List option checked for plugin_thold_notification_lists, but Thold creates plugin_notification_lists (with an emails column, see plugin_thold includes/database.php). The table never existed, so the option never appeared and its addresses were never added. The setting, recipient lookup and tests now use plugin_notification_lists. The table check also ran at the top level of includes/arrays.php, a query on every include. routerconfigs_config_settings() now adds the option when it builds the Settings tab, with the same condition. Secret values in the emailed diff are now hidden by default: passwords and secrets of every type (0, 5, 7, 8, 9), SNMP communities and SNMPv3 auth/priv keys, TACACS+/RADIUS keys, ISAKMP/IKEv2 pre-shared keys, OSPF, BGP, HSRP, VRRP and IS-IS authentication, PPP and wireless keys, and certificate or key hex rows, following the line classes Oxidized's IOS and NX-OS models redact. A generic fallback hides the value after a password, secret, community or key keyword, so an unrecognized line fails closed. The diff is redacted rather than the configurations, so a changed secret still appears as a -/+ pair without either value. Coverage is Cisco-centric, which the setting description says. Sending secrets needs the new 'Include Secrets in Diffs' option, which is off by default and also off when unset, and the 'Include Config Diffs' description warns that the diff goes out in clear text. Lines that change on every backup are always left out of the comparison: '! Last configuration change', '! NVRAM config last updated', 'ntp clock-period' and 'Current configuration :' (Oxidized IOS model), and NX-OS '!Time:' and '!Running configuration last done at:'. Exclude patterns are compiled by plugin_routerconfigs_regex() from #130, so a pattern containing '\/' is no longer double escaped and dropped. The description now says patterns match anywhere in a line unless anchored with ^ or $. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Review found the redaction fail-open in four ways: it hid only the first token of a value, so 'snmp-server community my secret RO' leaked the rest; it matched keywords only as whole words, missing tokens such as wpa-psk or set passwd; it handled each line alone, so multi-line secrets went out; and a keyword line in an unknown shape passed through. plugin_routerconfigs_redact_lines() now replaces everything after the first token containing passw, secret, psk, key, passphrase, community or md5 (plus an optional type number or ENC), keeping the line and anything after a quoted value that closes on the same line. It hides every line of a PEM block, a Cisco certificate or key-string block up to 'quit', and a quoted value continued on later lines, and hides any row of hex words or long base64 run on its own, so a block that starts outside a diff hunk is still covered. Keyword-less secrets (SNMP trap host communities, SNMPv3 keys, HSRP/VRRP text authentication) keep their own patterns. Each diff hunk is redacted as one sequence. This over-redacts some harmless lines, such as 'description key account'; the tests record that as accepted, and the setting description says redaction fails closed. Test values are placeholders. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Hiding secrets in the diff output could not work reliably: a hunk can start in the middle of a certificate or quoted key, and the old and new sides shared one block state. Each full configuration is now redacted from its first line before the exclude patterns and the comparison run, so blocks are always tracked from where they open and context lines are already clean. A change confined to a hidden value no longer shows up. The per-configuration redactor also: - re-checks the rest of a line after a block closes, and checks block opener lines like any other line - opens Cisco certificate blocks only on 'certificate [ca|self-signed] <serial>' and bare 'key-string', and hides every line until 'quit', -----END or the closing quote, hiding to the end of the configuration when the block never ends - ends the kept part at the keyword word and an optional '=' or ':', so password=..., secret:... and key-string=... are hidden, and adds pwd; a type number or ENC stays only as a separate word - skips backslash-escaped quotes when looking for a closing quote - hides text authentication values in general (NHRP, bare 'authentication text'), leaving aaa method lists and the IOS 802.1X interface commands alone - lets a PEM marker that comes before a keyword win, so the kept part never includes a PEM body The tests run a realistic sample configuration through the redactor and the emailed diff, and check that no placeholder value is left on the old side, the new side or in the context lines. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…g values Diffing the redacted configurations hid real changes: a changed password, or a crafted unterminated quote, redacted to the same text on both sides, so no hunk and no notice. And every value-parsing rule so far had a syntax that slipped past it. The comparison now runs on the raw lines (after the volatile and exclude filters), so every real change gives a hunk. Each whole configuration is redacted once from its first line, aligned with the raw lines, and the hunks print the redacted form of each line. A replaced line whose redacted form is unchanged is marked '(changed)', so a password change shows up as '-username <secret hidden>' and '+username <secret hidden> (changed)'. The hunks are rendered from jfcherng's grouped opcodes, or from Horde's edits grouped the same way on older Cacti releases. Redaction no longer parses values. A line that contains a secret-like word anywhere (passw, passphrase, pwd, secret, psk, key, community, md5, authentication, credential, token, private), a -----BEGIN marker or the start of a quoted value, and any snmp-server host or user line, is reduced to its indentation and leading word. Every line of a PEM block, a certificate block under 'crypto pki (or ca) certificate chain', a bare key-string block under 'crypto key pubkey-chain', or a quoted value left open by an odd number of unescaped quotes is hidden until it closes. A block that never closes hides the rest of the configuration, and the Email gets a note saying on which line it opened. Harmless lines that mention a key are hidden too, which is accepted and described in the setting. The tests cover every earlier bypass (key=value, key:value, JSON pairs, quoted keywords, escaped and unterminated quotes, values with spaces, standby/vrrp/nhrp/ospf authentication, certificate, key-string and PEM blocks, a hunk that starts mid-block) through the emailed diff, a diff without secrets, the unclosed-block note on either side, Horde grouping against jfcherng's, and a realistic configuration with no placeholder left anywhere. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sue-109-diff-email locales/po/cacti.pot regenerated. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ines With the comparison on the raw lines, hiding too much can no longer hide a change, so redaction errs fully toward hiding: - certificate and key-string blocks open wherever those lines appear, not only under their usual parent - a hex or base64 row is hidden even outside a block - single-quoted multi-line values are tracked like double-quoted ones - the state after each line is worked out from the whole line, so a line that closes one block and opens another (-----END X----- -----BEGIN Y-----, or a closing double quote followed by an opening single quote) keeps the next block hidden, and a terminator line is shown only when it is a bare quit or -----END line that leaves no block open The note for a block that never closes is kept. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Resolved by @somethingwithproof
|
Pushed for the review, after merging #199 (1529385, 1d42339):
Tests: every earlier bypass (key=value, key:value, JSON, quoted keywords, escaped and unterminated quotes, spaced values, standby/vrrp/nhrp/ospf authentication, cert, key-string and PEM blocks, a hunk that starts mid-block, blocks closing and reopening on one line) runs through the emailed diff with placeholder values, asserting a hunk and no placeholder. Further tests cover a diff without secrets, the unclosed-block note on both sides, Horde grouping against jfcherng, and a realistic configuration with nothing left anywhere. 284 passed locally, and the new cases fail on the previous heads. SonarCloud and the PHP 8.2-8.6 integration jobs pass on 86046b0. Profiling (median, 1% of lines changed, 20 exclude patterns):
The diff library dominates and grows faster than linear. Redaction is 3/30/121 ms. Running the excludes through To revisit:
|
…sue-109-diff-email Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues can expose SNMPv3 credentials in emails, send notifications from disabled lists, and skip delivery to Bcc-only recipients.
3 open issues
What changed in this PR
Adds optional per-device configuration diffs to RouterConfigs backup emails, building on the existing backup summary and diff tooling to address issue #109.
Changes:
- Compares each device’s two latest backups, with volatile-line filtering, binary detection, configurable context, and default secret redaction.
- Adds email-diff settings and optional Thold notification-list recipients.
- Adds unit coverage and updates translation strings and the changelog.
| File | Description |
|---|---|
tests/Unit/RouterconfigsDiffEmailTest.php |
Tests diff generation, redaction, file containment, and recipient resolution. |
tests/Unit/RouterconfigsConfigTest.php |
Tests conditional availability of the Thold setting. |
setup.php |
Registers the notification-list setting when Thold’s table exists. |
locales/po/cacti.pot |
Adds translation entries for settings and diff messages. |
includes/functions.php |
Implements email diffs, filtering, redaction, and recipient resolution. |
includes/arrays.php |
Defines diff and notification-list settings. |
CHANGELOG.md |
Records the configuration-diff email feature. |
Review effort: Balanced
…user rows Skip a notification list that is disabled, send its Bcc recipients through mailer(), and hide HPE 'snmpv3 user' lines in emailed diffs. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Automated validity review of @somethingwithproof's commits on this PR Reviewed and validated: full Pest suite passes (289 tests, 100% patch coverage), all integration legs (PHP 8.2–8.6) and SonarCloud are green, PHP lint clean, i18n/manifest gates pass, version correct (1.11). The fixes are solid — in particular correcting the Thold table name from the nonexistent Two notes for maintainer awareness (neither fails CI on this branch):
|
e4b1ad2
|
Reconciliation done. Merged the head of #130 ( Validated after the merge: full Pest suite passes (291 tests, 100% patch coverage), PHP lint clean, i18n and manifest gates pass, DST-robust ( |
…e-109-diff-email # Conflicts: # locales/po/cacti.pot
|
|
Conflicts resolved. The base branch (#130) advanced — #197 and #198 merged to Merged the current head of #130 into this branch (merge commit GitHub now reports this PR as MERGEABLE. Validated after the merge:
The remaining |





Description
Final PR in the stacked chain (#133 → #117 → #130 → #109), built on top of the #130 branch. It optionally appends a per-device unified configuration diff to the backup Email so a reviewer can see exactly what changed on each device after a run.
Backup-file reads are path-validated (the same containment check the Compare page uses) so a crafted directory/filename cannot escape the backup directory.
Related Issue
#109
Motivation and Context
Operators asked (in 2019, and again in 2023) for a daily emailed diff of each device's configuration, with the ability to exclude lines that always change (timestamps, serials) and to avoid diffing binary blobs — exactly the concerns raised in the issue thread. This builds that on top of the existing nightly summary Email and diff tooling.
How Has This Been Tested?
Ran the plugin's Pest suite from a Cacti checkout with the plugin as a real
plugins/routerconfigsdirectory (the stacked diff covers #133 + #117 + #130 + #109):pest --configuration phpunit.xml→ 190 passed (1145 assertions). Addedtests/Unit/RouterconfigsDiffEmailTest.phpcovering text/binary detection, exclude-pattern parsing and line filtering (including a malformed pattern and a clamped diff context), unified-diff rendering (identical vs changed), safe backup-file reading (missing keys, traversal, and a symlink that escapes its directory), the two-most-recent-backups diff (changed / fewer-than-two / binary / unreadable), and the recipient resolution with and without a Thold list.php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100→ 100.00% (288/288 lines).php tests/bin/check-i18n-pot.php origin/develop→ OK (locales/po/cacti.potregenerated; per-language.po/.moleft to Weblate).php -lclean on every changed file.php tests/bin/validate-manifest.php→ matches.The Email-assembly code that stitches the diffs into the message lives inside
plugin_routerconfigs_download()'s existing@codeCoverageIgnoreregion (it needs a live poller run); the pure helpers it calls are all unit-tested. The Horde Text_Diff fallback is coverage-ignored because the CI Cacti (1.2.32+) always provides jfcherng, so that branch is unreachable there.Types of changes
Checklist: