security: fetch the manifest more safely

In order to fetch vulnerabilities, the security scanner first has to
gather the layers to be inspected. In our case, we fetch this
information from the image manifest as given by the Registry.

This commit adds some code so the fetching of this manifest is safer.
For example, it will not freak out if a timeout was reached when
requesting the manifest from the registry.

Fixes #1743

Signed-off-by: Miquel Sabaté Solà <[email protected]>

Author: mssola

lib/portus/background/security_scanning.rb

@@ -42,9 +42,11 @@ def execute!
           # progress.
           tag.update_vulnerabilities(scanned: Tag.statuses[:scan_working])
 
-          # Fetch vulnerabilities.
+          # Fetch vulnerabilities. If there was an error and nil was returned,
+          # simply skip this iteration.
           sec = ::Portus::Security.new(tag.repository.full_name, tag.name)
           vulns = sec.vulnerabilities
+          next unless vulns
 
           # And now update the tag with the vulnerabilities.
           dig = update_tag(tag, vulns)

lib/portus/http_helpers.rb

@@ -92,7 +92,7 @@ def handle_error(response, params = {})
         str += "\n"
       end
 
-      raise NotFoundError, str
+      raise ::Portus::Errors::NotFoundError, str
     end
 
     private

lib/portus/registry_client.rb

@@ -52,8 +52,13 @@ def reachable?
     #   - The manifest digest.
     #   - The manifest itself as a ruby hash.
     #
-    # It will raise either a ManifestNotFoundError or a RuntimeError if
-    # something goes wrong.
+    # Three different exceptions might be raised:
+    #
+    #   - ::Portus::RequestError: there was a request error with the registry
+    #     (e.g. a timeout).
+    #   - ::Portus::Errors::NotFoundError: the given manifest was not found.
+    #   - ::Portus::RegistryClient::ManifestError: there was an unknown problem
+    #     with the manifest.
     def manifest(repository, tag = "latest")
       res = safe_request("#{repository}/manifests/#{tag}", "get")
 

lib/portus/security.rb

@@ -43,10 +43,11 @@ def vulnerabilities
 
       # First get all the layers composing the given image.
       client = Registry.get.client
-      manifest = client.manifest(@repo, @tag)
+      layers = fetch_layers(client)
+      return unless layers
 
       params = {
-        layers:       manifest.last["layers"].map { |l| l["digest"] },
+        layers:       layers.map { |l| l["digest"] },
         token:        client.token,
         registry_url: client.base_url
       }
@@ -58,5 +59,16 @@ def vulnerabilities
       end
       res
     end
+
+    # Returns an array with the layers as given in the manifest. If an error
+    # occured then nil will be returned and the error will be logged.
+    def fetch_layers(rc)
+      ary = rc.manifest(@repo, @tag)
+      ary.last["layers"]
+    rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+           ::Portus::RegistryClient::ManifestError => e
+      Rails.logger.info e.to_s
+      nil
+    end
   end
 end

spec/lib/portus/security/clair_spec.rb

@@ -161,4 +161,28 @@ def expect_cve_match(cves, given, expected)
 
     expect(res[:clair]).to be_empty
   end
+
+  it "returns nil when a timeout occurred when fetching the manifest" do
+    allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest)
+      .and_raise(::Portus::RequestError, exception: Net::ReadTimeout, message: "something")
+
+    clair = ::Portus::Security.new("coreos/dex", "unrelated")
+    expect(clair.vulnerabilities).to be_nil
+  end
+
+  it "returns nil when the manifest does not exist anymore on the registry" do
+    allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest)
+      .and_raise(::Portus::Errors::NotFoundError, "something")
+
+    clair = ::Portus::Security.new("coreos/dex", "unrelated")
+    expect(clair.vulnerabilities).to be_nil
+  end
+
+  it "returns nil when there was something wrong with the manifest" do
+    allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest)
+      .and_raise(::Portus::RegistryClient::ManifestError, "something")
+
+    clair = ::Portus::Security.new("coreos/dex", "unrelated")
+    expect(clair.vulnerabilities).to be_nil
+  end
 end