TheWitnessandThomas Vincent 629971cc73 Harden RRDtool proxy request handling (#20) ...

* Harden RRDtool proxy request handling

* Address PR review feedback: encrypt oversized-request errors, fix env inheritance, spacing, and symlink test

Addresses review comments on PR #17:
- lib/client.php: encrypt the 'Request too large' error frame once the
  client is authenticated, matching the existing encrypted-error pattern
- lib/client.php: build the removespikes child-process environment from
  getenv() instead of $_ENV so the full current environment (e.g. TEMP on
  Windows) is inherited, with RRDP_RRDTOOL_PATH merged on top
- lib/client.php: add the missing space in the RRDTOOL_PIPE_UNAVAILABLE
  error frame so it matches the other 'ERROR: ...' responses
- tests/run.php: check the symlink() fixture return value and skip the
  symlink-escape assertion (instead of producing a false positive) when
  symlink() is unavailable

* Upgrade phpseclib 3.x to phpseclib/phpseclib ^4.0

- composer.json/composer.lock: require phpseclib/phpseclib ^4.0 (was ^3.0)
- Update phpseclib3\\... namespace usages to phpseclib4\\... across
  rrdtool-proxy.php, lib/client.php, lib/functions.php, lib/wizard.php,
  and tests/run.php
- lib/functions.php: phpseclib4 removed the Random class, so
  Random::string(32) is replaced with random_bytes(32)
- Fix lib/__master__.php and lib/replicator.php, which PR #17 never
  migrated off the old pre-Composer phpseclib API (new \\phpseclib\\Crypt\\RSA(),
  ->loadKey(), ->getPublicKeyFingerprint() -- none of which exist in
  phpseclib 3.x or 4.x, so these MSR/replicator peer-authentication code
  paths would fatal at runtime). Both now use
  RSA::loadPublicKey()->getFingerprint('md5') wrapped in try/catch,
  matching the pattern already used in lib/client.php, plus a
  constant-time hash_equals() fingerprint comparison
- tests/run.php: relabel the phpseclib round-trip check from 'phpseclib 3'
  to 'phpseclib 4'

* Bump minimum PHP version to 8.2 and add CHANGELOG.md

- composer.json: require php ^8.2 (was ^8.1)
- include/global.php: bump RRDP_PHP_VERSION_REQUIRED to 80200 to match
- README.md: update minimum/recommended PHP version to 8.2
- composer.lock: refreshed for the new php platform constraint
- Add CHANGELOG.md tracking changes prefixed security:/issue:/feature:

* Add PHPDoc blocks to all functions

Adds a PHPDoc block above every function in rrdtool-proxy.php,
lib/functions.php, lib/client.php, lib/__master__.php, lib/replicator.php,
lib/wizard.php, cli/removespikes.php, tests/run.php, and
.phpstan-bootstrap.php, each with a brief description and inferred
@param/@return types. Function signatures themselves are unchanged; PHPStan
will flag any inferred types that turn out to be inaccurate.

* Add .github/copilot-instructions.md

Generated from README.md and this session's learnings: project architecture
(daemon entrypoint, shared lib helpers, client/master/replicator roles,
wizard, removespikes CLI), build/lint/test commands (composer scripts,
php tests/run.php, Windows platform-req caveats), and conventions (tabs,
GPL header block, PHPDoc-only typing, phpseclib4 gotchas, CHANGELOG entry
prefixes).

Also fix .gitignore: the '.git*' pattern was unintentionally also matching
and ignoring the .github/ directory; added a '!.github/' exception so files
under .github/ (like this one) are tracked.

* Add .gitattributes enforcing LF line endings; normalize existing CRLF files

- Add .gitattributes with '* text=auto eol=lf' so Git normalizes line
  endings to LF on checkout/commit regardless of local core.autocrlf
  settings.
- .gitignore: add '!.gitattributes' alongside the existing '!.github/'
  exception, since '.git*' was also unintentionally ignoring .gitattributes
  itself.
- Normalize the two tracked files that had CRLF line endings
  (.github/copilot-instructions.md, CHANGELOG.md) to LF; no other tracked
  files were affected.

* Manual PSR coding-style adjustments

Manual formatting/coding-style cleanup in cli/removespikes.php,
lib/__master__.php, lib/client.php, and lib/replicator.php.

* Address automated review feedback: symlink confinement, key-pair validation, and misc fixes

Addresses 10 unresolved copilot-pull-request-reviewer comments on PR #20:

- lib/functions.php: rewrite rrdp_resolve_path_within() to walk the path one
  segment at a time (instead of only checking the nearest existing ancestor
  via file_exists()), so a symlink anywhere along a not-yet-fully-existing
  path is resolved and confined even if the symlink itself is dangling.
- lib/functions.php: add rrdp_resolve_command_paths(), which resolves every
  RRD file path operand referenced by an RRDtool command (leading bare file
  arguments and DEF:/SDEF: clauses) against the RRA root and rewrites the
  command to use the canonical resolved paths, closing the symlink-escape
  gap left by the lexical-only rrdp_command_has_unsafe_path() check.
- lib/client.php: wire rrdp_resolve_command_paths() into the RRDtool command
  dispatch path, rejecting/rewriting unsafe operands before they reach the
  RRDtool pipe.
- lib/client.php: enforce the request-size limit on each completed
  transaction and the trailing unfinished remainder, instead of on the
  aggregate receive buffer before splitting on _EOT_, so multiple legitimate
  transactions in one read no longer trip a false 'Request too large'.
- rrdtool-proxy.php: validate at startup that the loaded public/private key
  files actually form a matching pair (comparing derived fingerprints)
  instead of only deriving a fingerprint from the public key, failing
  startup instead of accepting requests that will all fail to decrypt.
- lib/__master__.php, lib/replicator.php: resolve peer-supplied FULLSCAN
  folder/file names (and RRDLAST/RRDDUMP request filenames) through
  rrdp_resolve_rra_path() before using them in mkdir()/touch()/
  file_put_contents()/RRDtool restore-dump-last-flushcached operations,
  instead of concatenating raw peer-supplied text; reject and skip entries
  that fail confinement.
- lib/replicator.php, lib/__master__.php: guard rrdtool_pipe_init() failures
  in the main replication loop the same way lib/client.php already does,
  instead of indexing into a false return value and never retrying.
- cli/removespikes.php: make createRRDFileFromXML() return whether the
  RRDtool restore actually succeeded, and exit non-zero when it fails
  instead of silently exiting 0.
- .phpstan.neon: restore level 6 (was lowered to 2, contradicting
  .github/copilot-instructions.md) and add .phpstan-baseline.neon to
  capture the 42 pre-existing findings surfaced at level 6 without hiding
  future regressions.
- tests/run.php: add the standard Cacti Group GPL header block required for
  every PHP file.

---------

Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>

2026-09-19 09:04:05 -04:00

..

removespikes.php

Harden RRDtool proxy request handling (#20)

2026-09-19 09:04:05 -04:00