diff --git a/lib/puppet/file_serving/http_metadata.rb b/lib/puppet/file_serving/http_metadata.rb index d5c287c337..5c89a71d22 100644 --- a/lib/puppet/file_serving/http_metadata.rb +++ b/lib/puppet/file_serving/http_metadata.rb @@ -13,8 +13,11 @@ def initialize(http_response, path = '/dev/null') # hash available checksums for eventual collection @checksums = {} - # use a default mtime in case there is no usable HTTP header - @checksums[:mtime] = "{mtime}#{Time.now}" + # No usable HTTP header means we have no way to tell whether the + # remote content changed. Fall back to :none (always considered in + # sync) rather than fabricating "now" as an mtime, which would make + # every compile look like a change forever. + @checksums[:none] = '{none}' # RFC-1864, deprecated in HTTP/1.1 due to partial responses checksum = http_response['content-md5'] @@ -63,7 +66,7 @@ def initialize(http_response, path = '/dev/null') def collect # Prefer the checksum_type from the indirector request options # but fall back to the alternative otherwise - [@checksum_type, :sha256, :sha1, :md5, :mtime].each do |type| + [@checksum_type, :sha256, :sha1, :md5, :mtime, :none].each do |type| if type == :etag if @checksums[:etag] @checksum = @checksums[:etag] @@ -83,4 +86,12 @@ def collect break if @checksum end end + + # Called by the http terminus when it had to download the whole body to + # compute a checksum, because no header gave us one. Overrides whatever + # :none fallback #collect landed on with the real, earned digest. + def verify!(checksum_type, checksum) + @checksum_type = checksum_type + @checksum = "{#{checksum_type}}#{checksum}" + end end diff --git a/lib/puppet/indirector/file_metadata/http.rb b/lib/puppet/indirector/file_metadata/http.rb index b50fd6fa59..640eb468f7 100644 --- a/lib/puppet/indirector/file_metadata/http.rb +++ b/lib/puppet/indirector/file_metadata/http.rb @@ -3,12 +3,14 @@ require_relative '../../../puppet/file_serving/http_metadata' require_relative '../../../puppet/indirector/generic_http' require_relative '../../../puppet/indirector/file_metadata' +require_relative '../../../puppet/util/checksums' require 'net/http' class Puppet::Indirector::FileMetadata::Http < Puppet::Indirector::GenericHttp desc "Retrieve file metadata from a remote HTTP server." include Puppet::FileServing::TerminusHelper + include Puppet::Util::Checksums def find(request) checksum_type = request.options[:checksum_type] @@ -17,14 +19,14 @@ def find(request) client = Puppet.runtime[:http] head = client.head(uri, options: { include_system_store: true }) - return create_httpmetadata(head, checksum_type) if head.success? + return verify(client, uri, checksum_type, create_httpmetadata(head, checksum_type)) if head.success? case head.code when 403, 405 # AMZ presigned URL and puppetserver may return 403 # instead of 405. Fallback to partial get get = partial_get(client, uri) - return create_httpmetadata(get, checksum_type) if get.success? + return verify(client, uri, checksum_type, create_httpmetadata(get, checksum_type)) if get.success? end nil @@ -46,4 +48,55 @@ def create_httpmetadata(http_request, checksum_type) metadata.collect metadata end + + # Headers gave us nothing usable. If the caller wants real content + # verification (i.e. didn't explicitly ask for mtime/ctime/none), earn a + # checksum by downloading the body once here and hashing it as it + # streams by, without keeping the bytes around: if a rewrite turns out + # to be needed, the normal content fetch downloads it again. That costs + # one extra request only when the content has actually changed, and + # avoids holding an open tempfile for the far more common unchanged + # case, which a long-running agent would otherwise accumulate across + # many catalog runs. + # + # A failed or errored GET here is a real failure, not "unchanged": a + # non-success response returns nil, exactly like the HEAD request above + # already does on failure, and any raised error (network, TLS, etc.) + # propagates rather than being swallowed -- silently treating "we + # couldn't verify" as "unchanged" would hide the failure entirely, + # whereas before this method existed, that same failure would have + # surfaced when the always-different fabricated mtime forced a content + # fetch anyway. + def verify(client, uri, checksum_type, metadata) + return metadata if metadata.checksum_type != :none + + # mtime/ctime normally track changes, but with no time header from the + # server there is nothing to compare against, so the file can never be + # detected as changed -- say so instead of degrading silently. An + # explicit :none (or no requested type at all) already means + # "don't verify", so those stay quiet. + if checksum_type == :mtime || checksum_type == :ctime + Puppet.warning(_("Source %{uri} supplied no usable HTTP validation headers; with checksum => %{type} the file will never be detected as changed. Use a content digest checksum type to detect changes from this source.") % { uri: uri, type: checksum_type }) + return metadata + end + return metadata if checksum_type.nil? || checksum_type == :none + + # :etag means "verify with whatever digest the server hands us"; with + # no header to hand us one, fall back to the agent's configured + # digest algorithm, which -- unlike a hardcoded md5 -- is guaranteed + # usable under FIPS. + digest_type = checksum_type == :etag ? Puppet[:digest_algorithm].to_sym : checksum_type + + checksum = nil + client.get(uri, options: { include_system_store: true }) do |response| + return nil unless response.success? + + checksum = send("#{digest_type}_stream") do |sum| + response.read_body { |chunk| sum << chunk } + end + end + + metadata.verify!(digest_type, checksum) + metadata + end end diff --git a/lib/puppet/type/file/checksum.rb b/lib/puppet/type/file/checksum.rb index e8fa999bb8..a02861f231 100644 --- a/lib/puppet/type/file/checksum.rb +++ b/lib/puppet/type/file/checksum.rb @@ -10,7 +10,12 @@ # The default is defined in Puppet.default_digest_algorithm desc "The checksum type to use when determining whether to replace a file's contents. - The default checksum type is sha256." + The default checksum type is sha256. + + Set this to `etag` for `http(s)` sources to prefer a strong `ETag` + response header (when the server sends one that looks like a real + digest) over the `Last-Modified` header. See the `source` attribute + for the full order of preference `http(s)` sources use." # The values are defined in Puppet::Util::Checksums.known_checksum_types newvalues(:sha256, :sha256lite, :md5, :md5lite, :sha1, :sha1lite, :sha512, :sha384, :sha224, :mtime, :ctime, :none, :etag) @@ -59,6 +64,8 @@ def digest_algorithm return resolved end - :md5 + # No resolvable ETag-derived type to match (e.g. no source at all). + # Puppet[:digest_algorithm] is always FIPS-safe, unlike a hardcoded md5. + Puppet[:digest_algorithm].to_sym end end diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb index 97ed758e30..1cad1796f0 100644 --- a/lib/puppet/type/file/source.rb +++ b/lib/puppet/type/file/source.rb @@ -51,12 +51,29 @@ module Puppet parameter. If the `checksum_value` parameter is not specified for `puppet` and `file` sources, OpenVox computes a checksum based on its `Puppet[:digest_algorithm]`. For `http(s)` sources, OpenVox uses the - first HTTP header it recognizes out of the following list: - `X-Checksum-Sha256`, `X-Checksum-Sha1`, `X-Checksum-Md5` or `Content-MD5`. - If the server response does not include one of these headers, OpenVox - defaults to using the `Last-Modified` header. OpenVox updates the local - file if the header is newer than the modified time (mtime) of the local - file. + first usable signal out of the following, in order: + + * An `X-Checksum-Sha256`, `X-Checksum-Sha1`, `X-Checksum-Md5`, or + `Content-MD5` header. + * If `checksum => etag` is set, a strong `ETag` header whose value is a + bare 32, 40, or 64 character hex string (an md5, sha1, or sha256 + digest, respectively). Weak ETags (`W/"..."`) and ETags that aren't + recognizable digests are ignored. + * A `Last-Modified` header. OpenVox updates the local file if the + header is newer than the modified time (mtime) of the local file. + A `Last-Modified` header is always trusted when present: if a server + regenerates it on every request even though the content is unchanged, + the file is treated as changed on every run. In that case, arrange + for the server to send a checksum header or a usable `ETag` instead. + * If none of the above are present, OpenVox does not guess from a + fabricated timestamp. If `checksum` requests a real digest (the + default, or any explicit type other than `mtime`, `ctime`, or `none`), + OpenVox downloads the file once to compute one directly, and fails + the resource rather than assuming it is unchanged if that download + itself fails. If `checksum` is `mtime`, `ctime`, or `none`, OpenVox + treats the file as unchanged; because `mtime` and `ctime` normally + track changes, OpenVox also logs a warning that a file from such a + source can never be detected as changed. _HTTP_ URIs can include a user information component so that OpenVox can retrieve file metadata and content from HTTP servers that require HTTP Basic @@ -262,12 +279,17 @@ def copy_source_value(metadata_method) value = metadata.send(metadata_method) # Force the mode value in file resources to be a string containing octal. value = value.to_s(8) if param_name == :mode && value.is_a?(Numeric) - resource[param_name] = value if metadata_method == :checksum - # If copying checksum, also copy checksum_type + # If copying checksum, also copy checksum_type -- and do so before + # assigning the content, whose munge sums any value that isn't a + # recognizable checksum with the *current* checksum type. Metadata + # that resolved to :none yields the bare '{none}', which checksum? + # does not recognize; summing it with a stale requested type (e.g. + # mtime) would produce a desired value that can never match. resource[:checksum] = metadata.checksum_type end + resource[param_name] = value end end diff --git a/spec/fixtures/vcr/cassettes/Puppet_Type_File/when_sourcing/from_http/using_mtime/should_fetch_if_no_header_specified.yml b/spec/fixtures/vcr/cassettes/Puppet_Type_File/when_sourcing/from_http/using_mtime/should_not_fetch_if_no_header_specified.yml similarity index 100% rename from spec/fixtures/vcr/cassettes/Puppet_Type_File/when_sourcing/from_http/using_mtime/should_fetch_if_no_header_specified.yml rename to spec/fixtures/vcr/cassettes/Puppet_Type_File/when_sourcing/from_http/using_mtime/should_not_fetch_if_no_header_specified.yml diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb index bf0b47f866..1e0dbc6850 100644 --- a/spec/integration/type/file_spec.rb +++ b/spec/integration/type/file_spec.rb @@ -1373,17 +1373,19 @@ def build_path(dir) expect(File.read(httppath)).to eq "Content via HTTP\n" end - # The fixture has neither last-modified nor content-checksum headers. - # Such upstream ressources are treated as "really fresh" and get - # downloaded during every run. - it "should fetch if no header specified" do + # The fixture has neither last-modified nor content-checksum headers + # (its ETag is not a recognizable digest). With checksum => mtime + # there is nothing to compare against, so the file is treated as + # unchanged and a warning is logged. + it "should not fetch if no header specified" do File.open(httppath, "wb") { |f| f.puts "Content originally on disk\n" } # make sure the mtime is not "right now", lest we get a race FileUtils.touch httppath, mtime: Time.parse("Sun, 22 Mar 2015 22:57:43 GMT") catalog.add_resource resource catalog.apply expect(Puppet::FileSystem.exist?(httppath)).to be_truthy - expect(File.read(httppath)).to eq "Content via HTTP\n" + expect(File.read(httppath)).to eq "Content originally on disk\n" + expect(@logs.map(&:message)).to include(a_string_matching(/checksum => mtime the file will never be detected as changed/)) end it "should fetch if mtime is older on disk" do diff --git a/spec/unit/file_serving/http_metadata_spec.rb b/spec/unit/file_serving/http_metadata_spec.rb index b47a260e3c..788938ccc8 100644 --- a/spec/unit/file_serving/http_metadata_spec.rb +++ b/spec/unit/file_serving/http_metadata_spec.rb @@ -34,20 +34,15 @@ http_response['X-Checksum-Md5'] = 'c58989e9740a748de4f5054286faf99b' metadata = described_class.new(http_response) metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end context "with no Last-Modified or Content-MD5 header from the server" do - it "should use :mtime as the checksum type, based on current time" do - # Stringifying Time.now does some rounding; do so here so we don't end up with a time - # that's greater than the stringified version returned by collect. - time = Time.parse(Time.now.to_s) + it "should use :none as the checksum type, rather than fabricating a changing mtime" do metadata = described_class.new(http_response) metadata.collect - expect( metadata.checksum_type ).to eq :mtime - checksum = metadata.checksum - expect( checksum[0...7] ).to eq '{mtime}' - expect( Time.parse(checksum[7..-1]) ).to be >= time + expect( metadata.checksum_type ).to eq :none + expect( metadata.checksum ).to eq '{none}' end end @@ -118,11 +113,11 @@ context "without checksum => etag" do let(:md5) { "f5ffec8d8d16b43d5e9ac6ad4330c445" } - it "does not auto-activate ETag and falls back to mtime" do + it "does not auto-activate ETag and falls back to :none" do http_response.add_field('ETag', %("#{md5}")) metadata = described_class.new(http_response) metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end end @@ -175,22 +170,22 @@ end context "that is a weak ETag" do - it "ignores the ETag and falls back to mtime" do + it "ignores the ETag and falls back to :none" do http_response.add_field('ETag', 'W/"f5ffec8d8d16b43d5e9ac6ad4330c445"') metadata = described_class.new(http_response) metadata.checksum_type = :etag metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end end context "that is not a recognizable hash" do - it "ignores the ETag and falls back to mtime" do + it "ignores the ETag and falls back to :none" do http_response.add_field('ETag', '"5e8c5-27a-3e8b8840"') metadata = described_class.new(http_response) metadata.checksum_type = :etag metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end end @@ -230,16 +225,31 @@ metadata = described_class.new(http_response) metadata.checksum_type = :etag metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end it "falls back to other checksums when no ETag is present" do metadata = described_class.new(http_response) metadata.checksum_type = :etag metadata.collect - expect( metadata.checksum_type ).to eq :mtime + expect( metadata.checksum_type ).to eq :none end end end end + + describe "#verify!" do + let(:http_response) { Net::HTTPOK.new(1.0, '200', 'OK') } + + it "overrides the :none fallback with an earned checksum" do + metadata = described_class.new(http_response) + metadata.collect + expect( metadata.checksum_type ).to eq :none + + metadata.verify!(:sha256, 'abc123') + + expect( metadata.checksum_type ).to eq :sha256 + expect( metadata.checksum ).to eq '{sha256}abc123' + end + end end diff --git a/spec/unit/indirector/file_metadata/http_spec.rb b/spec/unit/indirector/file_metadata/http_spec.rb index 12f17ee913..c2d12955b7 100644 --- a/spec/unit/indirector/file_metadata/http_spec.rb +++ b/spec/unit/indirector/file_metadata/http_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require 'digest' require 'puppet/indirector/file_metadata' require 'puppet/indirector/file_metadata/http' @@ -38,7 +39,7 @@ expect(result.path).to eq('/dev/null') expect(result.relative_path).to be_nil expect(result.destination).to be_nil - expect(result.checksum).to match(%r{mtime}) + expect(result.checksum).to eq('{none}') expect(result.owner).to be_nil expect(result.group).to be_nil expect(result.mode).to be_nil @@ -86,7 +87,7 @@ .to_return(status: 200, headers: DEFAULT_HEADERS.merge("ETag" => %("#{etag_md5}"))) result = model.indirection.find(key) - expect(result.checksum_type).to eq(:mtime) + expect(result.checksum_type).to eq(:none) end it "uses ETag as md5 when checksum_type is etag" do @@ -113,14 +114,17 @@ expect(result.checksum).to eq("{sha256}#{sha256}") end - it "ignores weak ETags even with checksum_type => etag" do + it "ignores weak ETags even with checksum_type => etag, and earns a checksum via GET instead" do + body = "some file content" stub_request(:head, key) .to_return(status: 200, headers: DEFAULT_HEADERS.merge( "ETag" => 'W/"f5ffec8d8d16b43d5e9ac6ad4330c445"' )) + stub_request(:get, key).to_return(status: 200, body: body) result = model.indirection.find(key, checksum_type: :etag) - expect(result.checksum_type).to eq(:mtime) + expect(result.checksum_type).to eq(Puppet[:digest_algorithm].to_sym) + expect(result.checksum).to eq("{#{Puppet[:digest_algorithm]}}#{Digest::SHA256.hexdigest(body)}") end it "leniently parses base64" do @@ -227,6 +231,99 @@ end end + context "when no header can provide a checksum" do + it "earns one via GET when a real digest was requested" do + body = "some file content" + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + stub_request(:get, key).to_return(status: 200, body: body) + + result = model.indirection.find(key, checksum_type: :sha256) + expect(result.checksum_type).to eq(:sha256) + expect(result.checksum).to eq("{sha256}#{Digest::SHA256.hexdigest(body)}") + end + + it "does not fetch the body when checksum_type is mtime, but warns that changes can never be detected" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + # No :get stub: a network call here would fail the example. + + expect(Puppet).to receive(:warning).with(/checksum => mtime the file will never be detected as changed/) + result = model.indirection.find(key, checksum_type: :mtime) + expect(result.checksum_type).to eq(:none) + end + + it "does not fetch the body when checksum_type is ctime, but warns that changes can never be detected" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + + expect(Puppet).to receive(:warning).with(/checksum => ctime the file will never be detected as changed/) + result = model.indirection.find(key, checksum_type: :ctime) + expect(result.checksum_type).to eq(:none) + end + + it "does not fetch the body or warn when checksum_type is none" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + + expect(Puppet).not_to receive(:warning) + result = model.indirection.find(key, checksum_type: :none) + expect(result.checksum_type).to eq(:none) + end + + it "does not fetch the body or warn when no checksum_type was requested at all" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + + expect(Puppet).not_to receive(:warning) + result = model.indirection.find(key) + expect(result.checksum_type).to eq(:none) + end + + it "treats a failed verification GET as not found, rather than as unchanged" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + stub_request(:get, key).to_return(status: 500) + + expect(model.indirection.find(key, checksum_type: :sha256)).to be_nil + end + + it "propagates a network error during the verification GET, rather than treating it as unchanged" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + stub_request(:get, key).to_raise(Errno::ECONNREFUSED) + + # Match Puppet's own wrapper text, not the strerror for ECONNREFUSED, + # which differs between platforms (Windows says "No connection could + # be made because the target machine actively refused it."). + expect { + model.indirection.find(key, checksum_type: :sha256) + }.to raise_error(Puppet::HTTP::HTTPError, %r{Request to https://example\.com/path/to/file failed}) + end + + it "falls back to Puppet[:digest_algorithm] (not a hardcoded md5) when checksum_type is etag and nothing resolves" do + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + body = "some file content" + stub_request(:get, key).to_return(status: 200, body: body) + + result = model.indirection.find(key, checksum_type: :etag) + expect(result.checksum_type).to eq(Puppet[:digest_algorithm].to_sym) + expect(result.checksum).to eq("{#{Puppet[:digest_algorithm]}}#{Digest::SHA256.hexdigest(body)}") + end + + it "does not fall back to md5 for checksum_type etag under FIPS" do + allow(Puppet::Util::Platform).to receive(:fips_enabled?).and_return(true) + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS) + stub_request(:get, key).to_return(status: 200, body: "some file content") + + result = model.indirection.find(key, checksum_type: :etag) + expect(result.checksum_type).not_to eq(:md5) + end + + it "does not double-earn a checksum when a header already provided one" do + # A real header-derived checksum should short-circuit before any GET, + # regardless of what was requested. + stub_request(:head, key).to_return(status: 200, headers: DEFAULT_HEADERS.merge(last_modified)) + # No :get stub: a network call here would fail the example. + + result = model.indirection.find(key, checksum_type: :sha256) + expect(result.checksum_type).to eq(:mtime) + end + end + context "when searching" do it "raises an error" do expect { diff --git a/spec/unit/type/file/source_spec.rb b/spec/unit/type/file/source_spec.rb index a8c21bb9f6..c7e18a000a 100644 --- a/spec/unit/type/file/source_spec.rb +++ b/spec/unit/type/file/source_spec.rb @@ -252,6 +252,17 @@ allow(Puppet::Util::Platform).to receive(:windows?).and_return(false) end + it "copies the checksum type before the content, so metadata that resolved to :none munges to '{none}' rather than being summed with the stale requested type" do + @resource[:checksum] = :mtime + allow(@metadata).to receive(:checksum).and_return("{none}") + allow(@metadata).to receive(:checksum_type).and_return(:none) + + @source.copy_source_values + + expect(@resource[:checksum]).to eq(:none) + expect(@resource[:content]).to eq("{none}") + end + context "when source_permissions is `use`" do before :each do @resource[:source_permissions] = "use"