Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to include severity in table output #409

Merged
merged 6 commits into from
Jun 23, 2023
Merged

Add option to include severity in table output #409

merged 6 commits into from
Jun 23, 2023

Conversation

giovanni-bozzano
Copy link
Contributor

Add an optional --include-severity flag to include a severity column when using table or markdown output, when severity is present in the OSV schema.

Add an optional --include-severity flag to include a severity column in table and markdown output formats.
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add tests

@giovanni-bozzano
Copy link
Contributor Author

Sure, test added.

From an end user perspective, it would be useful to also have the numeric score computed in the same column, for a quicker overview. Let me know if this sounds good.

@another-rex
Copy link
Collaborator

Thanks for working on this @giovanni-bozzano!

Unfortunately we want to be very careful with what we add to the table output as we want to keep the horizontal width down, and also keep the number of CLI flags limited.

Converting CVSS:3.1/AV:L/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:L into a single numeric score will help, but it also loses a decent amount of information.

A more long term solution is to create a markdown/text based output format, which is more suitable for adding additional information like severity info, fixed versions...etc, We are currently developing this as part of #57, currently estimated to be completed this quarter.

For now a workaround would be to parse the json output to get the severity value.

@another-rex another-rex reopened this Jun 15, 2023
@another-rex
Copy link
Collaborator

Discussed this a bit more with the team, something similar to this could work showing only the computed scores in the table, and only showing the full string (or a subset of it) in the text output I was describing above so if the user wants more info they can get it.

We can use https://github.com/goark/go-cvss to compute the score relatively easily, if you can replace the current full severity string with just the score it would be great!

@giovanni-bozzano
Copy link
Contributor Author

I changed the table output to be numeric as you suggested. This will show the computed base score in case of CVSS v2 or v3, and it will keep the OSV schema score string in case of quantitative type.

It would probably be good to point out that this is the base score. Maybe this information can be included in the docs to keep the table narrow.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change! Agreed with adding to the documentation to clarify that this is the base severity score.

I think now that it's reasonably short, we can remove the include-severity flag and just always show the severity column. (Does mean a couple of the tests in main_test.go will need to be updated)

internal/output/table.go Outdated Show resolved Hide resolved
@oliverchang
Copy link
Collaborator

Thanks for the change! Agreed with adding to the documentation to clarify that this is the base severity score.

I think now that it's reasonably short, we can remove the include-severity flag and just always show the severity column. (Does mean a couple of the tests in main_test.go will need to be updated)

CC @hayleycd to see where we can fit this in our current docs.

- Remove "--include-severity" flag
- Rename severity column to "CVSS"
@giovanni-bozzano
Copy link
Contributor Author

I removed the flag and renamed the column to "CVSS". Tests have been changed accordingly.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just check if the final comments are correct then we should be good to merge!

internal/output/table.go Outdated Show resolved Hide resolved
internal/output/table.go Outdated Show resolved Hide resolved
@hayleycd
Copy link
Collaborator

So what determines if the severity score is CVSS v2 or CVSS v3? (Clarifying so I can update the docs)

@another-rex
Copy link
Collaborator

Inside each OSV entry the severity looks like this:

  "severity": [
    {
      "type": "CVSS_V3",
      "score": "CVSS:3.1/AV:N/AC:H/PR:L/UI:R/S:U/C:H/I:H/A:H"
    }
  ],

The type field shows what version is used, currently the schema defines only two options, v3 and v2: https://ossf.github.io/osv-schema/#severity-field

As for why you would choose one over the other I think it's just newer advisories use cvss v3, and older ones use v2.

@hayleycd
Copy link
Collaborator

hayleycd commented Jun 21, 2023

@another-rex
So there is nothing specific to this code, it will just pull the cvss (whether version 2 or 3) from the OSV entry?

@another-rex
Copy link
Collaborator

Yep

@another-rex another-rex merged commit 24be1d4 into google:main Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants