Skip to content

Conversation

migmartri
Copy link
Member

@migmartri migmartri commented Sep 28, 2023

Fixes #365

The issue was that we had a hardcoded list of material types to normalize and it was outside of the testing loop.

The issue was consistent when an user tried to use any new material type, such as the new sarif or vex types.

This code makes two things

  • Change the normalization code to be less brittle and dynamic based on the structure of the material type
  • Adds missing error handling

Example of successful attestation

┌────────────────────────────────────────────────────────────────────────────────────┐
│ Materials                                                                          │
├──────────┬─────────────────────────────────────────────────────────────────────────┤
│ Name     │ image                                                                   │
│ Type     │ CONTAINER_IMAGE                                                         │
│ Digest   │ sha256:39d48595c363755c9e00240ea74e0b6be0b5bdfb63ef2e58b9ec469e8828787d │
├──────────┼─────────────────────────────────────────────────────────────────────────┤
│ Name     │ rootfs                                                                  │
│ Type     │ ARTIFACT                                                                │
│ Filename │ sbom.cyclonedx.json                                                     │
│ Value    │                                                                         │
│ Digest   │ sha256:16159bb881eb4ab7eb5d8afc5350b0feeed1e31c0a268e355e74f9ccbe885e0c │
├──────────┼─────────────────────────────────────────────────────────────────────────┤
│ Name     │ sarif                                                                   │
│ Type     │ SARIF                                                                   │
│ Filename │ report.sarif                                                            │
│ Value    │                                                                         │
│ Digest   │ sha256:c4a63494f9289dd9fd44f841efb4f5b52765c2de6332f2d86e5f6c0340b40a95 │
├──────────┼─────────────────────────────────────────────────────────────────────────┤
│ Name     │ sbom                                                                    │
│ Type     │ SBOM_CYCLONEDX_JSON                                                     │
│ Filename │ sbom.cyclonedx.json                                                     │
│ Value    │                                                                         │
│ Digest   │ sha256:16159bb881eb4ab7eb5d8afc5350b0feeed1e31c0a268e355e74f9ccbe885e0c │
├──────────┼─────────────────────────────────────────────────────────────────────────┤
│ Name     │ vex                                                                     │
│ Type     │ OPENVEX                                                                 │
│ Filename │ openvex_v0.2.0.json                                                     │
│ Value    │                                                                         │
│ Digest   │ sha256:b4bd86d5855f94bcac0a92d3100ae7b85d050bd2e5fb9037a200e5f5f0b073a2 │
└──────────┴─────────────────────────────────────────────────────────────────────────┘

Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
@migmartri migmartri requested a review from danlishka September 28, 2023 09:45
Copy link
Member

@danlishka danlishka left a comment

Choose a reason for hiding this comment

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

LGTM

artifactType := mdef.MaterialType
nMaterial := mdef.NormalizedOutput()
nMaterial, err := mdef.NormalizedOutput()
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

why do we continue on error, can you add a comment here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because v01 is actually deprecated and in fact we should probably remove it. Just to not to add more code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have added a comment on the reasoning, sorry.

@migmartri migmartri merged commit f5ede4c into chainloop-dev:main Sep 28, 2023
@migmartri migmartri deleted the fix-download-issue branch September 28, 2023 11:01
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.

panic at attestation push
2 participants