Accept short profile IDs #107

jan-cerny · 2017-07-03T11:47:13Z

Recently we implemented short profile IDs in OpenSCAP. However
oscapd-evaluate checks if the given profile ID is present
in SCAP Security Guide Datastream. We need to apply
suffix matching on the list of SSG profiles. Otherwise
we will get an error that the profile was not found.

mpreisler · 2017-07-06T14:38:04Z

This definitely needs a comment. Looking at the line and not being familiar with the short ID feature there is no way you can figure out what it is doing...

mpreisler · 2017-07-06T14:39:08Z

Doesn't this break the rest of the API? Like creating and defining tasks and all that? Short profile ID won't work when generating HTML guides.

I am leaning NACK on this.

jan-cerny · 2017-07-11T13:08:19Z

This is in "oscapd-evalaute", not in "oscapd-cli".

This condition checks if the ID provided by user is a suffix of some profile ID in SSG for the target platform. I depends on underlying tool eg. oscap or oscap-ssh if it can process the short ID.

The case that I solve by this patch is: I have oscap that supports shortened IDs installed, so I want to use shortened IDs in oscapd-evaluate scan as well, but I can't, because oscapd-evaluate scan parses SSG datastream, it can't find that exact ID there, so it terminates.

@mpreisler Could you explain connection of this with creating and defining tasks, please?
Do we need a more complex solution for some reason?

mpreisler · 2017-07-11T17:43:38Z

IMO if we add some paradigm to one part of the API we should add it everywhere. Profile ID meaning different things in different parts of the API leads to confusion. Furthermore, both oscapd-cli and oscapd-evaluate use EvaluationSpec, so I don't see why this should be limited to oscapd-evaluate.

jan-cerny · 2017-07-13T09:28:15Z

@mpreisler OK, that sounds reasonable. So I think I will rework the fix so that it completes the shortened ID to the full ID by longest suffix match, preferably before EvaluationSpec is created. That way the shortened ID won't be stored in EvaluationSpec, so it won't introduce any paradigm in any API.

jan-cerny · 2017-07-13T09:47:29Z

I have reworked it so that it doesn't work with short profile IDs in the API, it completes them instead.

mpreisler · 2017-07-13T14:00:32Z

This doesn't solve much and goes contrary to what I said. As a user I expect to be able to use short IDs in all parts of openscap daemon or nowhere.

…

On Jul 13, 2017 05:47, "Jan Černý" ***@***.***> wrote: I have reworked it so that it doesn't work with short profile IDs in the API, it completes them instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#107 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAt-ARgHa_fsx_o1xKjSEmRU6P6sFXcEks5sNeexgaJpZM4OMLwX> .

jan-cerny · 2017-07-14T09:03:43Z

@mpreisler Then, please could you explain how do you imagine short IDs in oscapd-evaluate? I don't understand at all what do you want.

I still think it would be beneficial to support short IDs in oscapd-evaluate, because that will enable short IDs in atomic scan, which will improve user experience of atomic scan, because users will not have to remember and type looong IDs.

If it isn't possible to enable short IDs in oscapd-evaluate, please feel free to close this pull request.

jan-cerny · 2017-07-25T07:58:54Z

@mpreisler Any idea?

mpreisler · 2017-07-25T20:07:32Z

@mpreisler Any idea?

As I said, if it were me I would do this in a way that adds short profile IDs to both oscapd-evaluate (all subcommands) and oscapd-cli (all subcommands). Perhaps others can chime in.

jan-cerny · 2017-07-27T07:58:54Z

We discussed this yesterday. I will look into this problem again. I will try to find a solution that works across whole Deamon in all use-cases. It could be done in EvalauationSpec, maybe as a new method of EvaluationSpec.

jan-cerny · 2017-07-27T14:40:29Z

I looked into this again today. I think that EvaulationSpec doesn't care if the ID is short or long. Only subcommand in oscapd-cli that I found that takes the profile ID is "task-create -i", which is interactive, so user just presses a number, and doesn't type the ID by hand. In oscapd-evaluate, the only subcommand is "oscapd-evaluate scan", which was solved in this PR. I think we I just need the code to evaluation_spec module.

jan-cerny · 2017-07-27T15:08:07Z

I have created a method in EvaluationSpec and moved the code there.

mpreisler · 2017-07-27T15:16:17Z

openscap_daemon/evaluation_spec.py

@@ -349,6 +349,21 @@ def get_cpe_ids(self, config):

        return cpe_ids

+    def match_ssg_profile_id(self, ssg_sds, profile):


suggested name "select_profile_by_suffix". and please change "profile" variable to "profile_suffix".

jan-cerny · 2017-07-27T16:02:34Z

I have renamed the method to "select_profile_by_suffix" and changed "profile" variable to "profile_suffix".

mpreisler · 2017-07-27T16:06:29Z

openscap_daemon/evaluation_spec.py

+                break
+        else:
+            raise RuntimeError(
+                "Profile with id='%s' doesn't exist on target '%s'. "


this error is wrong if we are selecting by suffix

Should it be something like Profile with ID matching suffix '%s' doesn't ?

The whole thing doesn't make sense in the method in EvaluationSpec, I suggest to make it throw an exception and then you can make a nice message in oscapd-evaluate.

Can I catch the exception?

Yeah, throw in EvaluationSpec, then catch it in oscapd-evaluate to make a nice specific message.

Recently we implemented short profile IDs in OpenSCAP. However oscapd-evaluate checks if the given profile ID is present in SCAP Security Guide Datastream. We need to apply suffix matching on the list of SSG profiles. Otherwise we will get an error that the profile was not found.

jan-cerny · 2017-07-28T10:36:10Z

I have improved this patch . The method select_profile_by_suffix throws an exception. This exception is caught in oscapd-evaluate where an error message is produced.

jan-cerny · 2017-07-28T11:08:00Z

@openscap-ci test this please

yuumasato · 2017-07-31T09:10:04Z

openscap_daemon/evaluation_spec.py

+        for p in profiles:
+            if p.endswith(profile_suffix):
+                self.profile_id = p
+                break


We should handle multiple matches of the suffix too.

I think that we don't have multiple profiles with same ID in SSG. But it still makes sense to check for multiple matches. Thanks for suggestion. It will handle if user types some very short suffix, maybe a single character. I'll do that.

mpreisler · 2017-07-31T15:23:15Z

openscap_daemon/evaluation_spec.py

@@ -349,6 +349,16 @@ def get_cpe_ids(self, config):

        return cpe_ids

+    def select_profile_by_suffix(self, ssg_sds, profile_suffix):


This shouldn't take ssg_sds. Instead it should use whatever content is already set in the EvaluationSpec. If there is no content set yet it should throw an exception.

OK, I wouldn't expect that. I'll try to handle it.

mpreisler · 2017-07-31T15:23:25Z

openscap_daemon/evaluation_spec.py

+                self.profile_id = p
+                break
+        else:
+            raise RuntimeError("No profile with suffix %s" % profile_suffix)


make this a typed exception if you plan to catch it.

mpreisler · 2017-07-31T15:33:37Z

bin/oscapd-evaluate

-                                % (es.profile_id, target, target)
+                        try:
+                            args.profile = es.select_profile_by_suffix(ssg_sds, args.profile)
+                        except RuntimeError:


this is quite dangerous, a lot of things can throw RuntimeError, please make this exception typed

OK 👍 👍

An exception will be raised if multiple matches occur.

Instead of providing the SCAP Security Guide source datastream directly to the method select_profile_by_suffix, we will make the datastream available in EvaluationSpec and then we will obtain the datastream filepath from the EvaluationSpec.

This commit creates a new class of exception "ProfileSuffixMatchError", that is thrown by select_profile_by_suffix method in situations when a profile ID could not be matched. Then this exception is caught and then an error message is issued.

jan-cerny · 2017-08-01T12:36:13Z

I have done requested changes.

yuumasato · 2017-08-01T13:24:43Z

LGTM.
I think @mpreisler would like to review before merging.

jan-cerny · 2017-08-01T13:35:55Z

@yuumasato thank you

mpreisler · 2017-08-01T15:59:48Z

I trust you @yuumasato ! :D

jan-cerny added this to the 0.1.7 milestone Jul 3, 2017

jan-cerny force-pushed the profile_id_check_fix branch from 08f7a41 to a19f7d5 Compare July 13, 2017 09:45

jan-cerny force-pushed the profile_id_check_fix branch from a19f7d5 to 8e32083 Compare July 27, 2017 15:07

mpreisler reviewed Jul 27, 2017

View reviewed changes

jan-cerny force-pushed the profile_id_check_fix branch from 8e32083 to d762ef1 Compare July 27, 2017 16:01

mpreisler reviewed Jul 27, 2017

View reviewed changes

jan-cerny force-pushed the profile_id_check_fix branch from d762ef1 to 13802e6 Compare July 28, 2017 10:31

jan-cerny force-pushed the profile_id_check_fix branch from 13802e6 to fdf0f0a Compare July 28, 2017 10:33

yuumasato suggested changes Jul 31, 2017

View reviewed changes

mpreisler reviewed Jul 31, 2017

View reviewed changes

Check for multiple matches of the same suffix

76db9a6

An exception will be raised if multiple matches occur.

jan-cerny added 2 commits August 1, 2017 14:10

Use typed exception for profile IDs

cb93115

This commit creates a new class of exception "ProfileSuffixMatchError", that is thrown by select_profile_by_suffix method in situations when a profile ID could not be matched. Then this exception is caught and then an error message is issued.

yuumasato approved these changes Aug 1, 2017

View reviewed changes

mpreisler assigned yuumasato Aug 1, 2017

mpreisler merged commit aeb6325 into OpenSCAP:master Aug 1, 2017

		@@ -349,6 +349,21 @@ def get_cpe_ids(self, config):

		return cpe_ids

		def match_ssg_profile_id(self, ssg_sds, profile):

		@@ -349,6 +349,16 @@ def get_cpe_ids(self, config):

		return cpe_ids

		def select_profile_by_suffix(self, ssg_sds, profile_suffix):

Accept short profile IDs #107

Accept short profile IDs #107

Conversation

jan-cerny commented Jul 3, 2017

mpreisler commented Jul 6, 2017

mpreisler commented Jul 6, 2017

jan-cerny commented Jul 11, 2017

mpreisler commented Jul 11, 2017

jan-cerny commented Jul 13, 2017

jan-cerny commented Jul 13, 2017

mpreisler commented Jul 13, 2017 via email

jan-cerny commented Jul 14, 2017 • edited Loading

jan-cerny commented Jul 25, 2017

mpreisler commented Jul 25, 2017 • edited Loading

jan-cerny commented Jul 27, 2017

jan-cerny commented Jul 27, 2017

jan-cerny commented Jul 27, 2017

mpreisler Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

jan-cerny commented Jul 27, 2017

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

jan-cerny commented Jul 28, 2017

jan-cerny commented Jul 28, 2017

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

jan-cerny commented Aug 1, 2017

yuumasato commented Aug 1, 2017

jan-cerny commented Aug 1, 2017

mpreisler commented Aug 1, 2017 • edited Loading

jan-cerny commented Jul 14, 2017 •

edited

Loading

mpreisler commented Jul 25, 2017 •

edited

Loading

mpreisler Jul 27, 2017 •

edited

Loading

mpreisler commented Aug 1, 2017 •

edited

Loading