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

vulnerability attestation: ITE-9 specification #268

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

hectorj2f
Copy link
Contributor

As a continuation of the discussion initiated in #58 and the Slack thread, I decided to propose a vulnerability scan-specific in-toto predicate.

This ITE-9 document describes a vulnerability predicate type to represent vulnerability reports from the scanners in an "exportable" manner and independently of the format chosen to output the scanning results.

Note that, the fields proposed for this new predicate type are inspired by the cosign specification for the vulnerability attestations.

Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f hectorj2f requested a review from a team as a code owner July 23, 2023 22:57

> The timestamp of when the vulnerability DB was updated last time.

**scanner.result** object
Copy link
Member

@pxp928 pxp928 Jul 24, 2023

Choose a reason for hiding this comment

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

As this is the most important, we should have a set struct that people should follow. For example, a field to specify vulnerabilityIDs and such.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

I'd probably expect a list of vulnerability identifiers coupled with severity. If you'd like to allow scanners to add additional custom information perhaps let them add annotations?

E.g.

scanner.result = [{"id": "CVE-123", "severity": "medium", "annotations": { "cvss_score": 5.2 }}, {...}]

I could see having different lists for CVEs vs OSV results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds good to me 👍🏻 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomHennen @pxp928 This suggestion makes sense to me. But I wanted to stay away from the defined format to represent the results at least for now. Users may prefer to use SARIF,or any other custom JSON format to set the results of the scan. I'd say a second iteration should define the format of this result struct.

wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the real value from predicates is having the predicates be well defined so that consumers know how to use it. Without result being well defined I don't think there's much value. Perhaps I'm missing something?

Copy link
Contributor Author

@hectorj2f hectorj2f Jul 27, 2023

Choose a reason for hiding this comment

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

@TomHennen Sure! I highlighted it on the Purpose section. My initial goal was to define the metadata around the results which could help to exchange these attestations without having to solve the problem of which format to use when sharing the results of a vulnerability scan by any scanner.

However I am happy to move on defining a structure for the result's section if we feel we are ready to come to an agreement. cc @znewman01

Copy link
Member

Choose a reason for hiding this comment

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

Hey @hectorj2f, yes we should define a starting structure for the results for an initial release that we can iterate on if needed. For example, having a list of vulnerabilityIDs might be a start. Not sure if we want to include this but we could also have severity:

{
	"severity": [ {
		"type": string,
		"score": string
	} ]
}

similar to the osv implementation: https://ossf.github.io/osv-schema/#severity-field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sgtm! I'll add these suggestions.

spec/predicates/vuln.md Outdated Show resolved Hide resolved

The in-toto [attestation] framework and a [Vulnerability scanner tool].

## Use Cases
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for some reason Markdown doesn't seem to identify this as a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having a look at this!

## Use Cases

When sharing the results of a vulnerability scan using an attestation, there is certain metadata that is crucial to trust and reuse this information.
Information about the scanner used during the scanning is relevant to trust these resuls. The state of the vulnerability database used to search for vulnerabilities defines the accuracy of the results. Other metadata information such as the timestamp when the scan finished could define the reusability of these results.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: resuls -> results

spec/predicates/vuln.md Show resolved Hide resolved

> The timestamp of when the vulnerability DB was updated last time.

**scanner.result** object
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

I'd probably expect a list of vulnerability identifiers coupled with severity. If you'd like to allow scanners to add additional custom information perhaps let them add annotations?

E.g.

scanner.result = [{"id": "CVE-123", "severity": "medium", "annotations": { "cvss_score": 5.2 }}, {...}]

I could see having different lists for CVEs vs OSV results?

@colek42
Copy link
Member

colek42 commented Jul 24, 2023

@hectorj2f can you talk about what the benefits of this attestation format vs wrapping SARIF in an in-toto statement?

@hectorj2f
Copy link
Contributor Author

@colek42 Without going into details of defining a specific output format here (e.g. Sarif) for the scanner results, the main purpose consists on defining a set of metadata fields to exchange these attestations without having to worry about which output format was used by the scanner to share the results here. These metadata fields will help anyone consuming these attestations to know things about the used scanner, the state of the database and other values. It is more of a standarization of a set of fields that could help to exchange and trust the results from these attestations. For instance, if I get a vulnerability attestation but I know the vulnerability database used by this scanner hasn't been updated since months. I'll discard these results. Other aspects, such as the version/source of the scanner can represent for certain customers and reason to ignore these attestations.

However if I now focus on the benefits of a different format versus Sarif. I'd say Sarif is a language conceived to render data in a GUI and consequently it ignores additional data that you can only get from custom output formats (e.g. grype json or trivy json output formats). In addition to that, the sarif specification is massive because it is used to render all kind of scanning results (not just vulnerability scanning).

@colek42
Copy link
Member

colek42 commented Jul 28, 2023

@hectorj2f thank you. That makes complete sense.

Signed-off-by: Hector Fernandez <[email protected]>
Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f
Copy link
Contributor Author

hectorj2f commented Aug 17, 2023

@marcelamelara Thanks for the review. I've addressed your comments. Could you have another look :) ?

Copy link
Member

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

LGTM! This looks good for an initial release. Thanks @hectorj2f!

spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
@adityasaky
Copy link
Member

@hectorj2f do you think you could add a proto definition for this predicate in https://github.com/in-toto/attestation/tree/main/protos/in_toto_attestation/predicates?

@hectorj2f
Copy link
Contributor Author

@adityasaky Sure. Would it be okay to add it as part of a separate PR ?

@adityasaky
Copy link
Member

Sure, I think that's fine.

@hectorj2f
Copy link
Contributor Author

@TomHennen Could you have another look at this PR ?

Copy link
Contributor

@TomHennen TomHennen 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 making this changes, this is looking good!

spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
@hectorj2f
Copy link
Contributor Author

@TomHennen Thanks for the review, I addressed your comments.

Copy link
Contributor

@TomHennen TomHennen 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 changes, just a couple minor things to cleanup.

spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
spec/predicates/vuln.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TomHennen TomHennen 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 one minor comment.

spec/predicates/vuln.md Show resolved Hide resolved
Signed-off-by: Hector Fernandez <[email protected]>
@TomHennen TomHennen merged commit cefdc8c into in-toto:main Sep 8, 2023
1 check passed
@TomHennen
Copy link
Contributor

Thanks for working through this Hector!

@hectorj2f
Copy link
Contributor Author

Thanks to everyone for the reviews 🎉 !

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.

6 participants