Attention is currently required from: Nico Huber, Martin L Roth, Angel Pons, Arthur Heymans. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63639 )
Change subject: Add SBOM (Software Bill of Materials) Generation ......................................................................
Patch Set 13:
(13 comments)
This change is ready for review.
Patchset:
PS13: Now I added the 'goswid' tool (https://github.com/9elements/goswid) as static util program (not the git repo itself). That one still needs some polishing, which I will do in the near future. But for now it does what it is supposed to do, which is converting the json SWID format into a binary format which is smaller and optionally compressed.
File src/sbom/payload-BOOTBOOT.json.src:
https://review.coreboot.org/c/coreboot/+/63639/comment/a4cf7af7_d97d08fa PS12, Line 6: BootBoot
Isn't the name all-uppercase? BOOTBOOT
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/1db0efa2_7d551884 PS12, Line 12: BootBoot
ditto
Done
File src/southbridge/intel/common/firmware/Kconfig:
https://review.coreboot.org/c/coreboot/+/63639/comment/37e169e8_e3544a89 PS12, Line 67: ME_SBOM
nit: I'd call this `INCLUDE_ME_SBOM`
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/fe2b27e5_224faa2d PS12, Line 68: "Include SBOM file"
This prompt text is too generic and can confuse people, as it's the only thing immediately shown to […]
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/a795e999_b2fdd2d7 PS12, Line 70: default y
Why is this enabled by default?
Didn't mean to enable it by default. At least not for now
https://review.coreboot.org/c/coreboot/+/63639/comment/3d0914cb_94addd30 PS12, Line 74: ME (Management Engine)
I don't think it's necessary to explain what ME means in this file. […]
I tried to be more explicit, but I guess it's a bit redundant
https://review.coreboot.org/c/coreboot/+/63639/comment/5026017a_e0946679 PS12, Line 78: "Generate SBOM file"
I'd try to make it clear that this option isn't great, maybe something like this? […]
you are right I like yours better
https://review.coreboot.org/c/coreboot/+/63639/comment/ce31d067_c946c111 PS12, Line 90: "SBOM file path"
How about: […]
Done
File util/goswid/cmd/main.go:
https://review.coreboot.org/c/coreboot/+/63639/comment/840cb240_e63fad93 PS12, Line 54: //if (output_format != uswid) && (len(input_file_paths) > 1) { : // ErrorOut("multiple input files are only supported in conjunction with the .uswid output file extension\n") : //}
?
since this corresponds to a different project/repo, I will make the changes there and pull them at a later time. There is still some polishing on that one left anyway.
https://review.coreboot.org/c/coreboot/+/63639/comment/ecb40b7b_0f87e1f1 PS12, Line 68: if if_len >= 5 && input_file_path[if_len-5:if_len] == ".json" { : err = input_tag.FromJSON(input_file) : } else if if_len >= 4 && input_file_path[if_len-4:if_len] == ".xml" { : err = input_tag.FromXML(input_file) : } else if if_len >= 5 && input_file_path[if_len-5:if_len] == ".cbor" { : err = input_tag.FromCBOR(input_file) : } else if if_len >= 6 && input_file_path[if_len-6:if_len] == ".uswid" { : _, err = uswid_input_tag.FromUSWID(input_file) : isUSWID = true : } else { : fmt.Printf("input file extension not recognized, assuming USWID: %s\n", input_file_path) : _, err = uswid_input_tag.FromUSWID(input_file) : isUSWID = true : }
Hmmm, why not split the filename using `. […]
I guess yours is a bit more explicit. Like the other one, I will make the changes on that project and pull the upstream in the near future.
File util/goswid/pkg/uswid/uswid.go:
https://review.coreboot.org/c/coreboot/+/63639/comment/fe04de7b_4b2dc491 PS12, Line 14: // can't be const...
why?
to be honest I was kind of confused with this one. But it seems golang really doesn't allow a byte array to be const.
https://review.coreboot.org/c/coreboot/+/63639/comment/62c92c54_29b87e05 PS12, Line 32: 16
Would be nice to avoid having magic offsets. Maybe define constants so that they have a name? […]
in my opinion this is the most explicit and readable version of doing protocol/encoding stuff. An abstraction would only make it worse (at least in my opinion)