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

feat(sbom): add support for scanning a sbom attestation #2652

Merged
merged 24 commits into from
Aug 8, 2022

Conversation

otms61
Copy link
Collaborator

@otms61 otms61 commented Aug 2, 2022

Description

Support for scanning a sbom attestation.
We can scan a sbom attestation as the following.

$ trivy image --format cyclonedx --output sbom.cdx.json otms61/vuln-python
$ cosign attest --key cosign.key --type cyclonedx --predicate sbom.cdx.json  otms61/vuln-python --no-upload > attest.sbom.cdx.json
$ trivy sbom attest.sbom.cdx.json

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).


// When cosign creates an SBOM attestation, it stores the predicate under a "Data" key.
// https://github.com/sigstore/cosign/blob/938ad43f84aa183850014c8cc6d999f4b7ec5e8d/pkg/cosign/attestation/attestation.go#L39-L43
if _, found := st.Predicate.(map[string]interface{})["Data"]; found {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It causes panic if the type assertion fails. Can we make sure the assertion works beforehand?

Suggested change
if _, found := st.Predicate.(map[string]interface{})["Data"]; found {
if cosignPredicate, ok := st.Predicate.(map[string]interface{}); ok {
data, found := cosignPredicate["Data"]
if !found {
return Statement{}, xerrors.Errorf("unsupported predicate format")
}
st.CosignPredicateData = data

Also, we can define our own struct only with needed fields so that we will not have to go back and forth for marshaling/unmarshaling. We can pass json.RawMessage to the SBOM unmarshaler.

predicateByte, err = json.Marshal(attest.CosignPredicateData)
if err != nil {
return sbom.SBOM{}, xerrors.Errorf("failed to marshal predicate: %w", err)
}

// StatementHeader defines the common fields for all statements
type StatementHeader struct {
	PredicateType string    `json:"predicateType"`
}

// CosignPredicate specifies the format of the Custom Predicate.
type CosignPredicate struct {
	Data      json.RawMessage
}

/*
Statement binds the attestation to a particular subject and identifies the
of the predicate. This struct represents a generic statement.
*/
type Statement struct {
	StatementHeader
	// Predicate contains type speficic metadata.
	Predicate CosignPredicate `json:"predicate"`
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea! It will simplify the code!

return sbom.SBOM{}, xerrors.Errorf("failed to decode attestation: %w", err)
}

return u.predicateUnmarshaler.Unmarshal(bytes.NewReader(attest.Predicate.Data))
Copy link
Collaborator

@knqyf263 knqyf263 Aug 3, 2022

Choose a reason for hiding this comment

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

Could you wrap the error by xerrors if it is returned? It adds stack trace and helps us debug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@knqyf263
Copy link
Collaborator

knqyf263 commented Aug 4, 2022

We may want to add an integration test here.

func TestCycloneDX(t *testing.T) {

@otms61 otms61 marked this pull request as ready for review August 4, 2022 12:25
@otms61
Copy link
Collaborator Author

otms61 commented Aug 4, 2022

I have added an integration test.

@@ -32,7 +32,7 @@ $ cosign attest --key /path/to/cosign.key --type spdx --predicate sbom.spdx.json

# cyclonedx
$ trivy image --format cyclonedx -o sbom.cdx.json <IMAGE>
$ cosign attest --key /path/to/cosign.key --type https://cyclonedx.org/schema --predicate sbom.cdx.json <IMAGE>
$ cosign attest --key /path/to/cosign.key --type cyclonedx --predicate sbom.cdx.json <IMAGE>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably should put a note about cosign version as cyclonedx was added in v1.10.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a description about cosign version.

Comment on lines 220 to 222
sbom.cdx.intoto.jsonl (alpine 3.7.3)

Total: 2 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sbom.cdx.intoto.jsonl (alpine 3.7.3)
Total: 2 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 2)
sbom.cdx.intoto.jsonl (alpine 3.7.3)
====================================
Total: 2 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 2)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

"github.com/aquasecurity/trivy/pkg/attestation"
)

func TestDecode(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func TestDecode(t *testing.T) {
func TestStatement_UnmarshalJSON(t *testing.T) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@knqyf263 knqyf263 merged commit 317a026 into aquasecurity:main Aug 8, 2022
@otms61 otms61 deleted the scan_sbom_attest branch August 8, 2022 14:03
@otms61 otms61 restored the scan_sbom_attest branch August 27, 2022 09:44
@ChristianCiach
Copy link

ChristianCiach commented Feb 15, 2023

Heads up: This will probably break with the next Cosign version because this has been merged:

Especially see the comment sigstore/cosign#2718 (comment)

I still think it is the wrong approach for Trivy to directly support in-toto attestations for the reasons I've specified here: sigstore/cosign#2307 (comment)

@knqyf263
Copy link
Collaborator

Thanks for the heads-up. Yeah, we're aware of that. Cosign adds many breaking changes, which is hard for us to follow...

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.

Scan SBOM attestation
3 participants