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

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

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

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

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
 

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

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