Verify HTTP file sources by digest when the server sends no cache-validation headers

[miharp]

Fixes #581.

AI disclosure: this contribution (investigation, code, tests, docs, and this description) was produced with substantial assistance from Claude (Anthropic), per the project's AI usage policy. I've reviewed it and take full responsibility for it.

Summary

A file resource with an http(s) source pointed at a server that sends no Last-Modified, ETag, or checksum header at all — or sends an ETag that's never consulted because checksum => etag wasn't requested — is treated as "changed" on every single run, forever. Puppet::FileServing::HttpMetadata fabricates the current wall-clock time as a fake mtime whenever headers give nothing usable, and since :mtime is the last resort in the checksum fallback chain, the comparison is guaranteed to conclude "changed" independent of whether the remote content actually changed. This fires notify/subscribe (e.g. restarting a service) on every Puppet run against origins like Artifactory behind a caching proxy, or plain raw.githubusercontent.com sources.

See #581 for the full root-cause writeup, real-world reproduction, and a link to a self-contained Docker repro.

Behavior changes at a glance

Rows marked unchanged behave identically before and after this PR. "Before" is current main; all runs assume the remote content did not change unless stated.

Server provides	checksum on the resource	Before (main)	After (this PR)
Nothing usable — no ETag, no Last-Modified (Artifactory behind a caching proxy)	default, or any digest type	rewrite + notify fires every run	body downloaded and hashed once: unchanged content → clean no-op (exit 0); changed content → applied normally
Strong digest-shaped ETag only, no Last-Modified (raw.githubusercontent.com)	default (no checksum => etag)	rewrite + notify fires every run — the ETag is never consulted without opt-in	same as row above
Strong digest-shaped ETag	etag	compared via ETag (#329), but the no-match fallback hardcoded md5 — broken under FIPS	same comparison; fallback now uses Puppet[:digest_algorithm], which is FIPS-safe
Reliable Last-Modified	default	compared via mtime	unchanged
Last-Modified that churns on every request despite unchanged content	default	rewrite + notify every run	unchanged — deliberate boundary, see below
X-Checksum-Sha256/-Sha1/-Md5 or Content-MD5 header	default	compared via the header digest	unchanged
Nothing usable	mtime or ctime	rewrite + notify every run	treated as unchanged, with a warning on every run that a file from this source can never be detected as changed
Nothing usable	none	rewrite + notify every run (the fabricated mtime applied even to an explicit none)	treated as unchanged, silently — "don't verify" is exactly what was asked for
HEAD succeeds with no validators, verification GET then fails	default, or any digest type	n/a (no verification GET existed; the failure surfaced on the forced content fetch)	the run fails (exit 4) — "couldn't verify" is never silently reported as "unchanged"

Note the change is invisible to network cost in the common cases: the unchanged-content case downloads the body once per run either way (before: as the content fetch that rewrote the file; after: as the verification stream that's hashed and discarded). Only the actually-changed case costs one extra request, and replace => false is not an available workaround for any of the fixed rows — it also ignores real remote changes forever, which is precisely what this fix preserves.

What changed

1. lib/puppet/file_serving/http_metadata.rb — no more fabricated Time.now. No usable header now resolves to :none ("unverifiable, assume unchanged" — the same semantics checksum => none already has elsewhere) instead of a checksum guaranteed to differ every run. Adds #verify!, called by the terminus below to override that :none verdict with a real, earned digest.
2. lib/puppet/indirector/file_metadata/http.rb — when metadata resolves to :none and the resource asked for a real digest (the default, or any explicit type other than mtime/ctime/none), downloads the body once and hashes it as it streams by, discarding the bytes. If a rewrite turns out to be needed (content actually changed), the normal content fetch downloads it again — one extra request only in the uncommon changed case, and no open file handles for a long-running agent to accumulate in the common unchanged case. A failed verification GET is a real failure, not "unchanged": a non-success response returns nil (not found, same as every other failure branch in #find — which also preserves next-source fallback for source => [...] arrays), and a raised network error propagates, exactly like the existing HEAD request already behaves. When an explicit checksum => mtime/ctime meets a server with no time header, the terminus now logs a warning that a file from this source can never be detected as changed, rather than degrading silently into never-update mode.
3. lib/puppet/type/file/source.rb — fixes a latent ordering bug in copy_source_value that the new :none path exposed: the content ('{none}') was assigned before the checksum param was updated from the requested type to the metadata's resolved type, so the content munge summed it with the stale type (e.g. mtime), producing a desired value ('{mtime}') that could never match the retrieved '{none}' — rewrite + notify every run, the very bug this PR fixes elsewhere. Found by scenario 8 of the end-to-end Docker repro (below), invisible to the unit suites; a regression spec now pins the ordering. Also brings the source parameter docs in line with actual behavior: they still described the old, buggy Last-Modified-or-nothing fallback as intended, never documented checksum => etag at all (which shipped in Feature: file etag support #329), and now state the fix boundary explicitly.
4. lib/puppet/type/file/checksum.rb — documents checksum => etag (same pre-existing gap), and fixes the pre-existing :etag fallback (also from Feature: file etag support #329) that hardcoded :md5 when no ETag-derived type resolves — which breaks under FIPS. Both that fallback and the terminus's equivalent now use Puppet[:digest_algorithm], which is always FIPS-safe, and — since the local and remote sides of the comparison must agree on an algorithm — fixing one without the other would have made them diverge.

Explicitly not fixed: a server whose Last-Modified header itself changes on every request despite unchanged content (e.g. a dynamic backend behind a caching proxy). That still resolves to :mtime, not :none, so the verification path never triggers — the gate only earns a checksum when there's no header to trust, not when there's an untrustworthy one. Distinguishing the two would mean paying for a full download on every apply for any HTTP source with a Last-Modified header at all, including the well-behaved majority where it's perfectly reliable. This boundary is now documented in the source parameter docs, not just here.

Known minor cost: a resource combining checksum_value with a headerless http(s) source still pays for the verification download even though the checksum_value comparison doesn't use the metadata checksum — the terminus only sees checksum_type in the request options. Rare combination; threading checksum_value through the indirection felt like scope creep for this fix.

Testing

Unit specs updated/added in spec/unit/file_serving/http_metadata_spec.rb, spec/unit/indirector/file_metadata/http_spec.rb, and spec/unit/type/file/source_spec.rb, including coverage for: earning a checksum via GET, the mtime/ctime/none/unspecified opt-outs (asserted by omitting the GET stub, so WebMock fails the example if a request sneaks through), the warning firing for mtime/ctime and staying silent for none/unspecified, a failed verification GET returning nil, a raised network error propagating, the FIPS-safe etag fallback, and a regression spec for the copy_source_value ordering (verified to fail against the unfixed code).

$ bundle exec rspec spec/unit/file_serving/http_metadata_spec.rb \
    spec/unit/indirector/file_metadata/http_spec.rb \
    spec/unit/type/file/source_spec.rb \
    spec/unit/type/file/content_spec.rb
274 examples, 0 failures, 1 pending (pre-existing, unrelated)

$ bundle exec rspec spec/unit/file_serving/ spec/unit/type/file_spec.rb spec/unit/type/file/
1084 examples, 0 failures, 10 pending (all pre-existing, unrelated)

For an end-to-end check beyond mocks, see miharp/openvox#1 (not for merge) — a Docker harness that runs real puppet apply against a hand-rolled HTTP server able to withhold, churn, or fail cache-validation responses on demand, asserting notify behavior and exact --detailed-exitcodes exit codes across eight scenarios, with console output from both an unpatched run (bug reproduces) and this branch (resolved). Scenario 7 proves the failure-propagation property end-to-end: HEAD succeeding with no validators followed by a failing verification GET produces a failed run (exit 4), not a silently clean one. Scenario 8 covers the checksum => mtime opt-out (unchanged + warning) — and caught the copy_source_value ordering bug on its first run, which the 1,000+ unit examples had missed.

In draft while the fix boundary described above (only :none triggers verification, not an untrustworthy-but-present header) and the backwards-compatibility labeling are under discussion.

[miharp]

Should this be labeled backwards-incompatible? The behavior change here means a file resource with an http(s) source and no usable cache-validation headers, previously reported as "changed" (and thus firing notify/subscribe) on every run, will now be treated as unchanged unless content actually differs. That's a real behavior change for anyone relying on the old always-changed side effect, similar to prior bug-fix PRs like #170 that got this label.

[Sharpie]

If I'm reading the change here correctly, it would be backwards incompatible.

The current behavior results in a file resource generating a change event every run, but also in any new file content being downloaded. If this change halts the downloads in addition to the change events, then it is a decisive break in behavior.

[Sharpie]

Also, if this halts both downloads and change events, then that behavior is already accessible today by setting replace => false on the file resource. In that case, my inclination would be to 👎 a change this large if it doesn't add any new functionality.

[miharp]

If this change halts the downloads in addition to the change events, then it is a decisive break in behavior.

Agreed this is backwards incompatible and should be labeled accordingly.

One correction on the mechanics: downloads don't stop, and new content is still applied. With no validators from the server, this branch downloads the full body on every run and hashes it as it streams by. Changed content is still detected, written, and notify still fires, exactly as today. The only thing that stops is the rewrite and notify when the downloaded bytes match what's already on disk. So network cost in the unchanged case is the same as main, one full-body download per run, and the changed case still converges. The PR description now has a before/after table covering each server-header shape, including the rows where nothing changes.

[miharp]

that behavior is already accessible today by setting replace => false on the file resource

replace => false isn't equivalent. It stops managing content entirely once the file exists, so a genuine change on the remote is ignored forever. This change still converges on real changes; it only stops reporting a change when the bytes are identical.

The gap it fills is origins that send no validators at all, like the Artifactory case in #581. There is no ETag for checksum => etag to use, and checksum_value defeats the purpose when the content legitimately changes now and then. Today the options for such a source are a notify storm on every run, or replace => false and silent staleness. This adds the missing third option: converge when content changes, stay quiet when it doesn't.