Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented Oct 7, 2024

This patch introduces support for a new material type called ZAP_DAST_ZIP, which represents a ZAP DAST report in ZIP format. The crafter will validate the incoming file as a valid ZIP archive and, using the extracted JSON report, ensure that the program used is ZAP.

Refs: #1359

@javirln javirln requested review from jiparis and migmartri October 7, 2024 10:21
@javirln javirln self-assigned this Oct 7, 2024
// detection. The order of the list is important as it defines the order of the
// detection process. Normally from most common one to the least common one and weaker validation method.
var CraftingMaterialInValidationOrder = []CraftingSchema_Material_MaterialType{
CraftingSchema_Material_ZAP_DAST_ZIP,
Copy link
Member

Choose a reason for hiding this comment

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

why first? Isn't it too expensive? I'd make sure we order by less expensive to more

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I don't know why I placed it there, it should be lower on the list.

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 Javi. ptal at my comment about the autodetection


// Craft will extract the ZAP JSON report from the zip file and upload it to the CAS
func (i *ZAPCrafter) Craft(ctx context.Context, filePath string) (*api.Attestation_Material, error) {
archive, err := zip.OpenReader(filePath)
Copy link
Member

Choose a reason for hiding this comment

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

could we check if it's a zip file?

Copy link
Member Author

Choose a reason for hiding this comment

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

That OpenReader checks already that the incoming file is a zip file but we can do a manual check in any case.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln merged commit 9c4d4a5 into chainloop-dev:main Oct 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.

ptal at my comment

}

// isZipFile checks if the file is a valid zip archive
func (i *ZAPCrafter) isZipFile(filePath string) error {
Copy link
Member

Choose a reason for hiding this comment

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

yeah, it seems that in the end this was not worth it and in fact this is a bad implementation since you are reading the whole file in memory. Imagine that you are providing a 500MB file... https://stackoverflow.com/a/76828828

Could you please remove this?

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.

2 participants