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(materials): Add support for CSAF 2.0 and 2.1 schemas and remaining CSAF_* materials #749

Merged
merged 11 commits into from
May 8, 2024

Conversation

javirln
Copy link
Member

@javirln javirln commented May 6, 2024

This pull request builds upon the changes introduced in PR #751 to implement CSAF version 2.0 and 2.1 support within Chainloop. This update incorporates three new CSAF Chainloop materials, expanding upon the existing CSAF VEX profile:

This implementation adheres to the CSAF 2.0 and 2.1 JSON schema verification standards, accessible here. Furthermore, it refactors the CSAFVEXCrafter to a more generalized CSAFCrafter, aligning with the JSON specification 2.0 and 2.1.

With this update, Chainloop contracts can now process the newly introduced materials, ensuring validation against the schema. Notably, there is no requirement to specify the csaf_version within the file, as Chainloop dynamically determines the appropriate schema for validation.

schemaVersion: v1
runner:
  type: GITHUB_ACTION
materials:
  - type: CSAF_INFORMATIONAL_ADVISORY
    name: informational-advisory
  - type: CSAF_SECURITY_ADVISORY
    name: security-advisory
  - type: CSAF_SECURITY_INCIDENT_RESPONSE
    name: security-incident-response

References: #635

This update enhances the versatility and compatibility of Chainloop with the latest CSAF standards.

@javirln javirln requested review from migmartri and jiparis May 6, 2024 10:27
@javirln javirln self-assigned this May 6, 2024
@javirln javirln marked this pull request as draft May 6, 2024 10:27
@javirln javirln changed the title feat(materials): Add support for CSAF- Informational Advisory, Security Advisory, Security Incident Response WIP feat(materials): Add support for CSAF- Informational Advisory, Security Advisory, Security Incident Response May 7, 2024
@javirln javirln changed the title WIP feat(materials): Add support for CSAF- Informational Advisory, Security Advisory, Security Incident Response WIP feat(materials): Add support for CSAF 2.0 and 2.1 schemas May 7, 2024
@javirln javirln changed the title WIP feat(materials): Add support for CSAF 2.0 and 2.1 schemas WIP feat(materials): Add support for CSAF 2.0 and 2.1 schemas and remaining materials May 7, 2024
@javirln javirln marked this pull request as ready for review May 7, 2024 10:41
Signed-off-by: Javier Rodriguez <[email protected]>
Copy link
Member

@jiparis jiparis left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple of minor comments.

internal/attestation/crafter/materials/csaf.go Outdated Show resolved Hide resolved
internal/schemavalidators/schemavalidators.go Show resolved Hide resolved
Signed-off-by: Javier Rodriguez <[email protected]>
@javirln javirln changed the title WIP feat(materials): Add support for CSAF 2.0 and 2.1 schemas and remaining materials feat(materials): Add support for CSAF 2.0 and 2.1 schemas and remaining CSAF_* materials May 7, 2024
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Awesome!

I'd like to see if we can differentiate the different profiles. We could reuse a single crafter but we might need to do additional checks.

@migmartri
Copy link
Member

migmartri commented May 8, 2024

On another note, let's not forget to document these new material types in the readme and the docs before release. Probably better in another patch, thanks!

javirln added 5 commits May 8, 2024 08:10
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
Signed-off-by: Javier Rodriguez <[email protected]>
javirln added 2 commits May 8, 2024 18:06
Signed-off-by: Javier Rodriguez <[email protected]>
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Left some comments for a future refactoring. For now I'd merge it.

Thanks!

}

// Validate the CSAF file against the specified category.
document, docExists := v.(map[string]interface{})["document"]
Copy link
Member

Choose a reason for hiding this comment

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

it would have been more clear if we use a custom struct with only the document and category objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm but the custom struct won't have the remaining fields to be passed down to the validator no? Maybe I'm missing something :/

Copy link
Member

Choose a reason for hiding this comment

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

it will not, that's why I mentioned doing this validation after the other one. You might require another marshall process though.

Anyways, not a big deal, feel free to merge it :)


// Validate the CSAF file against the specified category.
document, docExists := v.(map[string]interface{})["document"]
if docExists {
Copy link
Member

Choose a reason for hiding this comment

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

if you use a custom struct I do not think you would have needed to do all these if checks and castings.

documentMap, docMapOk := document.(map[string]interface{})
category, categoryExists := documentMap["category"].(string)

if docMapOk && categoryExists && category != "" && category != i.category {
Copy link
Member

Choose a reason for hiding this comment

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

I'd have done this validation after you do the base validation, that way you can guarantee that it's a base doc, and then use a custom struct to do a custom validation.

Does it make sense?

}

// Validate the CSAF file against the specified category.
document, docExists := v.(map[string]interface{})["document"]
Copy link
Member

Choose a reason for hiding this comment

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

it will not, that's why I mentioned doing this validation after the other one. You might require another marshall process though.

Anyways, not a big deal, feel free to merge it :)

Signed-off-by: Javier Rodriguez <[email protected]>
@javirln javirln merged commit 8c03ae2 into chainloop-dev:main May 8, 2024
14 checks passed
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.

3 participants