security scanning: Updated tags by digest

Vulnerabilities are shared across tags with the same digest. Therefore,
the update should be digest-wise.

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

app/models/tag.rb

@@ -100,6 +100,17 @@ def fetch_vulnerabilities
     vulnerabilities if scanned == Tag.statuses[:scan_done]
   end
 
+  # Updates the columns related to vulnerabilities with the given
+  # attributes. This will apply to only this tag, or all tags sharing the same
+  # digest (depending on whether the digest is known).
+  def update_vulnerabilities(attrs = {})
+    if digest.blank?
+      update_columns(attrs)
+    else
+      Tag.where(digest: digest).update_all(attrs)
+    end
+  end
+
   protected
 
   # Fetch the digest for this tag. Usually the digest should already be

lib/portus/background/security_scanning.rb

@@ -9,40 +9,66 @@ def work?
         ::Portus::Security.enabled? && Tag.exists?(scanned: Tag.statuses[:scan_none])
       end
 
+      # execute! updates the vulnerabilities of all tags which have not been
+      # scanned yet. Note that this is done digest-wise, so tags which have
+      # already been scanned might be updated as a side-effect of a tag with the
+      # same digest that had not been scanned until then.
       def execute!
+        digests = []
+
         Tag.where(scanned: Tag.statuses[:scan_none]).find_each do |tag|
+          # This may happen when pushing multiple images with the same digest at
+          # once, while a previous one has already been scanned on a previous
+          # iteration.
+          next if tag.digest.present? && digests.include?(tag.digest)
+
           # Mark as work in progress. This is important in case there is a push in
           # progress.
-          tag.update_columns(scanned: Tag.statuses[:scan_working])
+          tag.update_vulnerabilities(scanned: Tag.statuses[:scan_working])
 
           # Fetch vulnerabilities.
           sec = ::Portus::Security.new(tag.repository.full_name, tag.name)
           vulns = sec.vulnerabilities
 
-          # Now it's time to update the columns and mark the scanning as done. That
-          # being said, it may have happened that during the scanning process a push
-          # or a delete action has been performed against this tag. For this reason,
-          # in a transaction we will reload it and check if any of these conditions
-          # changed. If not, then we will proceed with the change.
-          ActiveRecord::Base.transaction do
-            # If the tag no longer exists, then we need to raise a Rollback
-            # exception to leave early and cleanly from the transaction.
-            begin
-              tag = tag.reload
-            rescue ActiveRecord::RecordNotFound
-              raise ActiveRecord::Rollback
-            end
-
-            if tag&.scanned != Tag.statuses[:scan_none]
-              tag.update_columns(vulnerabilities: vulns, scanned: Tag.statuses[:scan_done])
-            end
-          end
+          # And now update the tag with the vulnerabilities.
+          dig = update_tag(tag, vulns)
+          digests << dig if dig
         end
       end
 
       def to_s
         "Security scanning"
       end
+
+      protected
+
+      # update_tag will mark the scanning as done for the given tag and assign
+      # to it the given vulnerabilities. This action will also affect tags with
+      # the same digest. This is done in a transaction, while taking into
+      # consideration possible changes on the given tag that may have happened
+      # meanwhile.
+      #
+      # It returns the affected digest.
+      def update_tag(tag, vulns)
+        digest = nil
+
+        ActiveRecord::Base.transaction do
+          # If the tag no longer exists, then we need to raise a Rollback
+          # exception to leave early and cleanly from the transaction.
+          begin
+            tag = tag.reload
+          rescue ActiveRecord::RecordNotFound
+            raise ActiveRecord::Rollback
+          end
+
+          if tag&.scanned != Tag.statuses[:scan_none]
+            tag.update_vulnerabilities(vulnerabilities: vulns, scanned: Tag.statuses[:scan_done])
+            digest = tag.digest
+          end
+        end
+
+        digest
+      end
     end
   end
 end

spec/controllers/tags_controller_spec.rb

@@ -39,7 +39,7 @@
       enable_security_vulns_module!
     end
 
-    it "assigns the requested tag as @tag", focus: true do
+    it "assigns the requested tag as @tag" do
       get :show, { id: tag.to_param }, valid_session
       expect(assigns(:tag)).to eq(tag)
       expect(response.status).to eq 200

spec/lib/portus/background/security_scanning_spec.rb

@@ -37,7 +37,7 @@
     it "properly saves the vulnerabilities" do
       VCR.turn_on!
 
-      tag = create(:tag, name: "tag", repository: repository, digest: "1", author: admin)
+      tag = create(:tag, name: "tag", repository: repository, author: admin)
 
       VCR.use_cassette("background/clair", record: :none) do
         subject.execute!
@@ -81,6 +81,28 @@
       subject.execute!
       expect { tag.reload }.to raise_error(ActiveRecord::RecordNotFound)
     end
+
+    it "updates all tags with the same digest" do
+      tag   = create(:tag, name: "tag", repository: repository, digest: "1", author: admin)
+      tag1  = create(:tag, name: "tag1", repository: repository, digest: "1", author: admin)
+      count = 0
+
+      allow_any_instance_of(::Portus::Security).to receive(:vulnerabilities) do
+        count += 1
+
+        tag.reload
+        tag1.reload
+        expect(tag.scanned).to eq Tag.statuses[:scan_working]
+        expect(tag1.scanned).to eq Tag.statuses[:scan_working]
+        ["something"]
+      end
+
+      subject.execute!
+
+      expect(count).to eq 1
+      expect(Tag.all.all? { |t| t.scanned == Tag.statuses[:scan_done] }).to be_truthy
+      expect(Tag.all.all? { |t| t.vulnerabilities == ["something"] }).to be_truthy
+    end
   end
 
   describe "#to_s" do