Annotate successful images with details #41

enoodle · 2017-07-27T11:19:52Z

Will allow annotating images that are compliant and also with more details as stated here: https://github.com/adellape/openshift-docs/blob/master/security/container_content.adoc#container-content-scanning

This is in continuation of ManageIQ/manageiq#15031

enoodle · 2017-08-07T12:37:14Z

@moolitayer Please review

enoodle · 2017-08-07T12:38:44Z

@aweiteka Please Review

enoodle · 2017-08-07T12:49:41Z

About the ARF file: I tried to trim it as much a possible but it is a complicated file structure and I couldn't find a simple way to write an empty file. If you have any suggestions please share.

moolitayer · 2017-08-07T13:02:23Z

spec/factories/container_image.rb

@@ -0,0 +1,5 @@
+FactoryGirl.define do
+  factory :openshift_container_image, :class => "ManageIQ::Providers::Openshift::ContainerManager::ContainerImage" do


👍 for factory

moolitayer · 2017-08-07T13:29:18Z

spec/factories/openscap_result.rb

@@ -0,0 +1,4 @@
+FactoryGirl.define do
+  factory :openscap_result
+  factory :openscap_rule_result


can you move openscap_rule_result to its own file please

moolitayer · 2017-08-07T13:43:43Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+  end
+
+  def openscap_summary
+    failed_rules = openscap_rule_results.where(:result => "fail")


openscap_rule_results.where(:result => "fail").group(:status).count { "Low" => 8, "High" => 15 }

moolitayer · 2017-08-07T13:44:29Z

@enoodle do we have a contact from OpenShift or from OpenSCAP that was in the loop of this and can review the annotations?

moolitayer · 2017-08-07T13:50:02Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+      {
+        :label         => label,
+        :severityIndex => index,
+        :score         => failed_rules.select { |rule| severities.include?(rule.severity) }.count


I think this is called data in https://github.com/adellape/openshift-docs/blob/master/security/container_content.adoc#container-content-scanning no?

It was changed since I first started working on this apparently. 😮

enoodle · 2017-08-08T09:19:32Z

@enoodle do we have a contact from OpenShift or from OpenSCAP that was in the loop of this and can review the annotations?

AFAIK @aweiteka is our contact on this issue ManageIQ/manageiq#15013

enoodle · 2017-08-08T12:32:17Z

@moolitayer I fixed your comments, ptal

moolitayer · 2017-08-08T13:56:44Z

@zakiva @cben @zeari @aweiteka can you review this?

cben · 2017-08-09T10:26:24Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+  end
+
+  def security_quality_annotation(compliant)
+    {"quality.images.openshift.io/vulnerability.manageiq" => {


https://docs.openshift.org/latest/security/container_content.html#security-image-metadata
lists 5 "acceptable values" for "providerId", which doesn't include "manageiq". I'm not sure they really intend this to be a closed list, but if yes perhaps this should be quality.images.openshift.io/vulnerability.openscap?

@aweiteka ^^ is the list of allowed providerId intentionally close-ended? Can we use manageiq?

@cben maybe we need to use some "global" function that will return the name of this tool, "manageiq" or "cfme" ? but I didn't know where to one and using "cfme" here seemed wrong.

I didn't mean "manageiq" vs "cloudforms", 👍 for fixed "vulnerability.manageiq" (if we really wanted, we could change :name).
I just meant that the spec as written "allows" only 5 provider ids:
openscap redhatcatalog redhatinsights blackduck jfrog
The whole point of having the spec is allowing new products to integrate so I doubt that's the intent...

I see in https://github.com/openshift/openshift-docs/pull/4461/files#diff-7f8d0575b67fa1bc3861ae9345fde2e2L98 @aweiteka changed allowed provider id redhatcloudforms to openscap, so I suppose he wants this to be vulnerability.openscap .

cben · 2017-08-09T10:35:44Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+      :name        => "ManageIQ",
+      :timestamp   => Time.now.utc.to_i,
+      :description => "OpenSCAP Score",
+      :reference   => "",


reference URL is "required". Do we have anything to put here?

URL of information source and/or more details. Required so user may validate the data.

I wanted to find a cool way like url_to_entity to make this but couldn't.

looks like Rails.application.routes.url_helpers.url_for(:controller => 'dashboard', :action => 'show', :only_path => true) .. but we should probably fix this at some point not to generate urls outside UI.

(I'm thinking urls can change, in the db, we need something that won't.)

cben · 2017-08-09T10:51:34Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+  end
+
+  def annotate_deny_execution(causing_policy)
+    annotate_image({
      "security.manageiq.org/failed-policy" => causing_policy,
      "images.openshift.io/deny-execution"  => "true"


[out of scope for this PR]
This ties making the scan details annotation with making allow/deny-execution annotation.
Until now the way user decided whether to make deny-execution annotations is by using or not the action on a policy reacting to scan complete event.
Now it might become useful to make policy annotation without denying execution, or have extra conditions for denying.
Also, conceivably one might want to deny/allow in other policies that don't have an OpenSCAP scan at hand.
=> in future, it may be worth splitting this, making scan details annotation immediately on scan (or as independent action?), and having actions that make only deny/allow annotation...

cben · 2017-08-09T11:29:18Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+  end
+
+  def annotate_deny_execution(causing_policy)
+    annotate_image({
      "security.manageiq.org/failed-policy" => causing_policy,
      "images.openshift.io/deny-execution"  => "true"


IMO when setting deny-execution we should clear allow-execution and vice versa.
See my comment ManageIQ/manageiq#15013 (comment)

@cben we discussed this on the original PR too : ManageIQ/manageiq#15031 (comment)

what about setting the other annotation to the negative value? It seems to convey more information?

cben

Looks great 👍

Have you seen this info appearing in Openshift UI, or is it a theoretical integration?

cben · 2017-08-09T11:43:08Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+      {
+        :label         => label,
+        :severityIndex => index,
+        :data          => failed_rules.select { |sev| severities.include?(sev) }.values.sum


took me a while to understand how this works. perhaps severities.sum { |sev| failed_rules.fetch(sev, 0) } would be clearer, perhaps not, your call.

cben · 2017-08-09T11:44:19Z

Forgot one question: what happens when you use this action in another policy, on an image that's never been scanned? My only concern is that it won't crash. I think it won't, openscap_rule_results.where(... will return 0 results.

moolitayer · 2017-08-09T12:46:40Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+  def annotate_allow_execution(causing_policy)
+    annotate_image({
+      "security.manageiq.org/successful-policy" => causing_policy,
+      "images.openshift.io/allow-execution"     => "true"


btw @enoodle why do we use two different annotations:

images.openshift.io/allow-execution

images.openshift.io/deny-execution,

"images.openshift.io/deny-execution" => "false" according to ManageIQ/manageiq#15013 (comment) (and the whole issue really)

enoodle · 2017-08-09T17:43:02Z

Forgot one question: what happens when you use this action in another policy, on an image that's never been scanned? My only concern is that it won't crash. I think it won't, openscap_rule_results.where(... will return 0 results.

@cben We might want in the future to annotate all the images that were not scanned yet until they are scanned (probably with non compliant)

How would you suggest to fail here ?

converting severities from openscap convention (high, medium, low, info, unknown) to Openshift specifications.

enoodle · 2017-08-13T14:20:50Z

@moolitayer @cben I addressed most of your earlier comments with the last refactoring (left as a separate commit to be squashed later). PTAL

cben

LGTM except one point

cben · 2017-08-13T16:11:10Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+      :reference   => Rails.application.routes.url_helpers.url_for(:controller => "container_image",
+                                                                   :action     => "openscap_html",
+                                                                   :id         => id,
+                                                                   :only_path  => true),


The annotation will live in Openshift, so a relative URL isn't very useful.
Would :only_path => false give an absolute URL?

@cben Not easily:
ArgumentError: Missing host to link to! Please provide the :host parameter, set default_url_options[:host], or set :only_path to true when I give :host => MiqServer.my_server.hostdname, but I don't see how to get the port number yet. any ideas?

enoodle · 2017-08-16T09:55:37Z

@cben I removed the reference and now it is back to being empty.

miq-bot · 2017-08-16T09:58:20Z

Checked commits enoodle/manageiq-providers-openshift@3af3d23~...cff66ad with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 2 offenses detected

app/models/manageiq/providers/openshift/container_manager/container_image.rb

❗ - Line 15, Col 7 - Style/WordArray - Use %w or %W for an array of words.
❗ - Line 38, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

enoodle · 2017-09-24T11:49:55Z

@aweiteka Can you take a look at this? Your approval will help merge this faster.

moolitayer

LGTM 👍

moolitayer · 2017-10-25T12:56:45Z

@aweiteka Please take a look

aweiteka · 2017-10-25T13:07:52Z

LGTM. Thanks.

simon3z · 2017-10-25T16:35:56Z

@enoodle @moolitayer IIRC we also have BZ RFE to reference here.

moolitayer · 2017-10-26T06:21:26Z

@enoodle ready for merge, is this safe to go in before ManageIQ/manageiq#15031 ? (seems to be the case)

enoodle · 2017-10-26T06:30:18Z

@moolitayer Yes, it will not crush but annotating will not work until the other patch is merged too. It is right for the other way too, they both should be merged together

enoodle mentioned this pull request Jul 27, 2017

Container ssa annotate success ManageIQ/manageiq#15031

Merged

enoodle changed the title ~~annotate successful images with details~~ [WIP] annotate successful images with details Jul 27, 2017

miq-bot added the wip label Jul 27, 2017

enoodle force-pushed the ssa_annotation_of_successful_images branch from 3339929 to 0ba9fb6 Compare August 7, 2017 12:20

enoodle changed the title ~~[WIP] annotate successful images with details~~ Annotate successful images with details Aug 7, 2017

miq-bot removed the wip label Aug 7, 2017

enoodle force-pushed the ssa_annotation_of_successful_images branch from 0ba9fb6 to 208c14e Compare August 7, 2017 12:36

moolitayer reviewed Aug 7, 2017

View reviewed changes

enoodle force-pushed the ssa_annotation_of_successful_images branch 2 times, most recently from 4ac3531 to 7fba3f5 Compare August 8, 2017 12:31

cben reviewed Aug 9, 2017

View reviewed changes

moolitayer reviewed Aug 9, 2017

View reviewed changes

enoodle changed the title ~~Annotate successful images with details~~ [WIP] Annotate successful images with details Aug 9, 2017

miq-bot added the wip label Aug 9, 2017

enoodle changed the title ~~[WIP] Annotate successful images with details~~ Annotate successful images with details Aug 10, 2017

miq-bot removed the wip label Aug 10, 2017

enoodle force-pushed the ssa_annotation_of_successful_images branch from e63aeb6 to d56b929 Compare August 10, 2017 12:35

Erez Freiberger added 3 commits August 13, 2017 15:31

annotate successful images with details

3af3d23

converting severities from openscap convention (high, medium, low, info, unknown) to Openshift specifications.

adding a test

e117a79

move to one annotating function

cff66ad

cben reviewed Aug 13, 2017

View reviewed changes

enoodle force-pushed the ssa_annotation_of_successful_images branch from d56b929 to cff66ad Compare August 16, 2017 09:54

cben approved these changes Aug 16, 2017

View reviewed changes

moolitayer approved these changes Oct 25, 2017

View reviewed changes

simon3z assigned moolitayer Oct 25, 2017

simon3z added the enhancement label Oct 25, 2017

moolitayer added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 29, 2017

moolitayer merged commit ec109a2 into ManageIQ:master Oct 29, 2017

		@@ -0,0 +1,5 @@
		FactoryGirl.define do
		factory :openshift_container_image, :class => "ManageIQ::Providers::Openshift::ContainerManager::ContainerImage" do

Annotate successful images with details #41

Annotate successful images with details #41

Conversation

enoodle commented Jul 27, 2017

enoodle commented Aug 7, 2017

enoodle commented Aug 7, 2017

enoodle commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Aug 8, 2017

enoodle commented Aug 8, 2017

moolitayer commented Aug 8, 2017

cben Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Aug 9, 2017

enoodle commented Aug 13, 2017

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Aug 16, 2017

miq-bot commented Aug 16, 2017

enoodle commented Sep 24, 2017

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Oct 25, 2017

aweiteka commented Oct 25, 2017

simon3z commented Oct 25, 2017

moolitayer commented Oct 26, 2017

enoodle commented Oct 26, 2017

moolitayer commented Aug 7, 2017 •

edited

Loading

cben Aug 9, 2017 •

edited

Loading

cben Aug 9, 2017 •

edited

Loading