search

OpenSCAP / openscap-daemon

Manages continuous scans of your infrastructure
https://www.open-scap.org/tools/openscap-daemon
GNU Lesser General Public License v2.1
106 stars 32 forks source link

Accept short profile IDs #107

Closed jan-cerny closed 7 years ago

jan-cerny commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

mpreisler commented 7 years ago

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ý" notifications@github.com 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 https://github.com/OpenSCAP/openscap-daemon/pull/107#issuecomment-315029438, or mute the thread https://github.com/notifications/unsubscribe-auth/AAt-ARgHa_fsx_o1xKjSEmRU6P6sFXcEks5sNeexgaJpZM4OMLwX .

jan-cerny commented 7 years ago

@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 commented 7 years ago

@mpreisler Any idea?

mpreisler commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

jan-cerny commented 7 years ago

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

jan-cerny commented 7 years ago

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 commented 7 years ago

@openscap-ci test this please

jan-cerny commented 7 years ago

I have done requested changes.

yuumasato commented 7 years ago

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

jan-cerny commented 7 years ago

@yuumasato thank you

mpreisler commented 7 years ago

I trust you @yuumasato ! :D