Conversation
mmarseu
left a comment
mmarseu
left a comment
There was a problem hiding this comment.
Shouldn't this check be part of the validate command? It is certainly a type of validation.
If it remains a separate command, available_commands.md should be updated.
IMO, the check that all bom-refs are unique is missing. Your code depends on this assumption but doesn't make sure it is correct. Again, this would be better suited for the validate command, because it is a requirement in the spec. It just can't be expressed in JSON schema.
| _STATUS_APP_ERROR = 2 | ||
| _STATUS_USAGE_ERROR = 3 | ||
| _STATUS_VALIDATION_ERROR = 4 | ||
| _STATUS_plausibility_ERROR = 5 |
| choices=["stdout", "warnings-ng"], | ||
| default="stdout", | ||
| ) | ||
| add_output_argument(parser) |
There was a problem hiding this comment.
Do not use this argument for the report. For one, it is confusing because other commands use the same argument for outputting an SBOM, not a log file. Secondly, the help message is incorrect for your use case.
Also, the interaction between --report-format and --output is not clear to me. What if the user specifies --report-format stdout but still passes an output file? What if it is the other way around: --report-format warnings-ng but no output?
| if args.output is None: | ||
| output = Path("./issues.json") | ||
| else: | ||
| output = args.output |
There was a problem hiding this comment.
If the user passes a path to a file in a folder that doesn't exist or a path to a folder, writing to this path will crash.
| if report_format == "warnings-ng": | ||
| warnings_ng_handler = WarningsNgReporter(file, output) | ||
| logger.addHandler(warnings_ng_handler) |
There was a problem hiding this comment.
From a design-perspective I don't find it intuitive that the command logic should interpret the command-line arguments to decide where to log it's errors. This seems better suited for __main__.py, if you can manage to make that work.
|
|
||
| # check compositions | ||
| for composition in sbom.get("compositions", []): | ||
| for reference in composition.get("assemblies", []): |
There was a problem hiding this comment.
You're missing the compositions[].dependencies array which also contains refs.
| # check if the dependency tree is connected, i.e. that the product | ||
| # decribed by the sbom depends directly or indirectly on every component. | ||
| # also checks that every component is depended on |
There was a problem hiding this comment.
This is a misinterpretation of CycloneDX. Not all components in an SBOM are required to be part of the same dependency tree.
That is why dependencies and assemblies are both a thing in CycloneDX. There are products assembled from multiple components, where there is no depends-on relationship but rather an is-part-of relationship.
Agree, though this should not be a default check when executing validate and be controlled through a flag. Regarding the uniqueness of |
CBeck-96
force-pushed
the
plausibility_test_bom_refs
branch
from
abe4be1 to
9f40972
Compare
| metavar="<plausability-check>", | ||
| choices=["yes", "y"], | ||
| help=( | ||
| "y/yes if the plausibility of the bom-refs in the" | ||
| "sbom should also be checked" | ||
| ), |
There was a problem hiding this comment.
Why this additional choice? Why not providing the flag plausibility-check means true?
|
|
||
|
|
||
| def invoke_validate(args: argparse.Namespace) -> int: | ||
| logger_validate = logging.getLogger(__name__) |
There was a problem hiding this comment.
why introduce a new logger and not use the existing?
| path_to_second_sbom = ( | ||
| path_to_folder_with_test_sboms + "sub_programm_T5.0.3.96_20220217T101458_cdx.json" | ||
| ) |
There was a problem hiding this comment.
why introduce a second SBOM, if you still make changes to it? In this case, just adjust your first sbom.
There was a problem hiding this comment.
adapting the tests for plausibility to this one is, and in doing so changing it to fit the requirements, would be work, that seemed unnecessary
| def test_get_validate( | ||
| self, mock_validate: unittest.mock.Mock, mock_read: unittest.mock.Mock | ||
| ) -> None: | ||
| error_return = {"error"} |
There was a problem hiding this comment.
introduced for using it once?
| errors.append( | ||
| create_error_orphaned_bom_ref( | ||
| affected.get("ref", ""), | ||
| "vulnerabilitie " + vulnerability.get("id", ""), |
|
Has this PR been abandoned? It's been in my todo list forever but now I'm not even sure there is a point in reviewing this anymore. |
|
Considering there are other priorities and demand seems low, i will close it for the time beeing and tackle the subject at a more oppurtune time (propably version 1.0 or higher). |
3 participants
closes #5