Stop treating HTTP file sources with no cache headers as always-changed - #582
Open
miharp wants to merge 1 commit into
Open
Stop treating HTTP file sources with no cache headers as always-changed#582miharp wants to merge 1 commit into
miharp wants to merge 1 commit into
Conversation
miharp
force-pushed
the
fix/http-file-source-checksum-verification
branch
4 times, most recently
from
August 3, 2026 16:14
7cf7fc2 to
bea2b72
Compare
Puppet::FileServing::HttpMetadata fabricated Time.now as a fake mtime whenever an HTTP(S) file source's HEAD response gave no Last-Modified, ETag, or checksum header. Since mtime is always the last resort in the checksum fallback chain, this made such a source look "changed" on every single compile, forever, regardless of checksum type requested, because the fabricated timestamp trivially compares as newer than whatever's already on disk. Any file resource pointed at an origin that sends no validators (e.g. Artifactory behind Cloudflare, per puppetlabs/puppet#9553) rewrites the file and fires notify/subscribe on every run. Fall back to :none (unverifiable, assume unchanged) instead. To avoid trading that false positive for silently never detecting a real remote change, the http file_metadata terminus now earns a real checksum when the caller asked for one: if headers give nothing usable and the requested checksum type is a real digest (the default, or any explicit type other than mtime/ctime/none), it downloads the body once and hashes it as it streams by. This does not fix a server whose Last-Modified header itself changes on every request despite unchanged content -- that still resolves to :mtime, not :none, so the verification path never triggers. There's no way to tell a lying header from a truthful one without also earning a checksum whenever any header is present, which would erase the point of HEAD-based metadata for the common case. Confirmed this boundary holds with a Docker-based puppet apply run against a hand-rolled HTTP server that can withhold or churn headers on demand. Also brings the source/checksum parameter docs in line with this and with checksum => etag (OpenVoxProject#329), which predates this fix but was never documented: they still described the old, buggy Last-Modified-or- nothing behavior as intended, and never mentioned etag at all. Three correctness issues in an earlier version of this change, caught in review: - A failed or errored verification GET returned the unverifiable :none metadata rather than surfacing the failure, so a transient network problem while trying to earn a checksum would silently look like "no changes" instead of a failed run. A non-success response now returns nil (not found, same as every other failure branch in this method); a raised error (network, TLS, etc.) is no longer rescued and propagates like the existing HEAD request already does. - The earned checksum was cached in a Tempfile on the metadata object so a subsequent rewrite could reuse it instead of downloading twice. But that Tempfile was only ever closed when a rewrite actually happened; the far more common unchanged case left it open until GC, which a long-running agent process (not the one-shot puppet apply the Docker harness exercises) could accumulate across many catalog runs. Dropped the caching: the body is hashed and discarded in place, and a rewrite -- when the (uncommon) case of real content change needs one -- downloads it again, exactly as before this change. - checksum => etag with no resolvable ETag fell back to a hardcoded :md5, breaking under FIPS the same way the existing, older fallback in Puppet::Type::File::Checksum#digest_algorithm already did (added in OpenVoxProject#329, pre-dating this fix). Both now fall back to Puppet[:digest_algorithm], which is always FIPS-safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Michael Harp <mike@mikeharp.com>
miharp
force-pushed
the
fix/http-file-source-checksum-verification
branch
from
August 4, 2026 10:29
bea2b72 to
deb51b5
Compare
Author
|
Should this be labeled |
miharp
marked this pull request as ready for review
August 4, 2026 10:51
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.
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
fileresource with anhttp(s)sourcepointed at a server that sends noLast-Modified,ETag, or checksum header at all — or sends anETagthat's never consulted becausechecksum => etagwasn't requested — is treated as "changed" on every single run, forever.Puppet::FileServing::HttpMetadatafabricates the current wall-clock time as a fake mtime whenever headers give nothing usable, and since:mtimeis the last resort in the checksum fallback chain, the comparison is guaranteed to conclude "changed" independent of whether the remote content actually changed. This firesnotify/subscribe(e.g. restarting a service) on every Puppet run against origins like Artifactory behind a caching proxy, or plainraw.githubusercontent.comsources.See #581 for the full root-cause writeup, real-world reproduction, and a link to a self-contained Docker repro.
What changed
lib/puppet/file_serving/http_metadata.rb— no more fabricatedTime.now. No usable header now resolves to:none("unverifiable, assume unchanged" — the same semanticschecksum => nonealready has elsewhere) instead of a checksum guaranteed to differ every run. Adds#verify!, called by the terminus below to override that:noneverdict with a real, earned digest.lib/puppet/indirector/file_metadata/http.rb— when metadata resolves to:noneand the resource asked for a real digest (the default, or any explicit type other thanmtime/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 forsource => [...]arrays), and a raised network error propagates, exactly like the existing HEAD request already behaves.lib/puppet/type/file/source.rb— docs only. Brings thesourceparameter docs in line with actual behavior: they still described the old, buggyLast-Modified-or-nothing fallback as intended, and never documentedchecksum => etagat all (which shipped in Feature: file etag support #329).lib/puppet/type/file/checksum.rb— documentschecksum => etag(same pre-existing gap), and fixes the pre-existing:etagfallback (also from Feature: file etag support #329) that hardcoded:md5when no ETag-derived type resolves — which breaks under FIPS. Both that fallback and the terminus's equivalent now usePuppet[: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-Modifiedheader 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 aLast-Modifiedheader at all, including the well-behaved majority where it's perfectly reliable.Known minor cost: a resource combining
checksum_valuewith a headerlesshttp(s)source still pays for the verification download even though thechecksum_valuecomparison doesn't use the metadata checksum — the terminus only seeschecksum_typein the request options. Rare combination; threadingchecksum_valuethrough the indirection felt like scope creep for this fix.Testing
Unit specs updated/added in
spec/unit/file_serving/http_metadata_spec.rbandspec/unit/indirector/file_metadata/http_spec.rb, including coverage for: earning a checksum via GET, themtime/ctime/none/unspecified opt-outs (asserted by omitting the GET stub, so WebMock fails the example if a request sneaks through), a failed verification GET returning nil, a raised network error propagating, and the FIPS-safeetagfallback.For an end-to-end check beyond mocks, see miharp/openvox#1 (not for merge) — a Docker harness that runs real
puppet applyagainst a hand-rolled HTTP server able to withhold, churn, or fail cache-validation responses on demand, asserting both notify behavior and exact--detailed-exitcodesexit codes, with console output from both an unpatched run (bug reproduces) and this branch (resolved). Its 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.Opening as draft to get early feedback on the fix boundary described above (only
:nonetriggers verification, not an untrustworthy-but-present header) before final review.