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: validate now accepts components with external bom #308

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mmarseu
Copy link
Collaborator

@mmarseu mmarseu commented Oct 16, 2024

The What of this PR is straight-forward: It makes the validate command accept components als valid which have an external reference of type bom, even if those components do not fulfil the requirements of our custom schema.
The program now outputs a warning to inform the user that the externally referenced BOM cannot be checked.

The How merits a little explanation, I guess, to help with the review. Because the schema diffs look much more confusing than they are.

The custom schemas now wrap all of our custom requirements on components in a big if-then-else construct, where the if tests whether the component has an external reference to a BOM.

  • If it does not, the else clause applies all requirements as before. (see note on version below)
  • If it does, the then clause applies a schema which is always expected to fail (by requiring an arbitrarily named property). I call this a marker requirement.

This marker requirement can be detected in the validation routine. When jsonschema produces an error for this particular marker, the tool only writes a warning but otherwise ignores the error.

Note on version: If the removal of the version requirements trips you up when you see it all the way at the top of the diff, don't worry. I have only moved it into the else clause, because an externally referenced BOM would be where the version is declared.

Misc changes:

  • Adds a dependency on pytest-subtests, a small pytest plugin which enables support for unittest's subtest feature when running the tests through pytest. The alternative would be to either not use subtests (but I think the feature is very useful in this case to run tests against all spec versions at once) or to rewrite the tests to native pytest which has a comparable feature.

Copy link
Contributor

github-actions bot commented Oct 16, 2024

Coverage

Coverage Report •
FileStmtsMissCoverMissing
validator
   validate.py110694%110–111, 113, 147, 163, 198
TOTAL200810494% 

Tests Skipped Failures Errors Time
367 2 💤 0 ❌ 0 🔥 12.537s ⏱️

@italvi italvi force-pushed the 295-ignore-any-requirements-in-the-custom-schema-on-components-which-have-an-external-reference-of-type-bom-however-output-a-warning-that-the-validity-of-the-component-cannot-be-checked branch from 077d948 to 484cd14 Compare November 22, 2024 12:57
Copy link
Collaborator

@italvi italvi left a comment

Choose a reason for hiding this comment

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

@mmarseu I faced two issues while testing: If e.g. version is empty, an error for the component is still thrown, because the if is after the try-catch, where you throw the warning, contradicting the statement from the warning.

Further: if the url is not a valid bom-link as described on the cdx website, respectively specification, you ignore it. So I could write some gibberish, even just an empty url and still would ignore the validation. Though this hopefully is only an issue if providing an externalReference manually.

pyproject.toml Outdated Show resolved Hide resolved
@@ -100,7 +100,24 @@ def validate_sbom(
)
for error in sorted(v.iter_errors(sbom), key=str):
try:
if len(error.absolute_path) > 3:
if error.validator == "required" and error.validator_value == [
"this_is_an_externally_described_component"
Copy link
Collaborator

Choose a reason for hiding this comment

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

From an implementation point of view: IMO it would be easier to just validate the component and then check here if the component includes an external reference pointing to a SBOM, no? Having this workaround/check in this one file here, would reduce the risk of doing wrong changes in the schema.

Copy link
Collaborator Author

@mmarseu mmarseu Nov 25, 2024

Choose a reason for hiding this comment

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

I'm not sure I fully understand the idea. You propose to not change the schema at all and add logic to the validator which - in case of errors in a component - checks whether that component contains an external reference to an SBOM. If it does, it will ignore the errors and print a warning?
From the top of my head, I can't see why that wouldn't also work. There'll be upsides and downsides to both approaches. and had we had a design discussion prior to the change, I might have favored yours. For efficiency's sake, I wouldn't change the implementation now, though. Feel free to take over, if you want 😆

@mmarseu
Copy link
Collaborator Author

mmarseu commented Nov 25, 2024

@mmarseu I faced two issues while testing: If e.g. version is empty, an error for the component is still thrown, because the if is after the try-catch, where you throw the warning, contradicting the statement from the warning.

I don't really see the problem with this particular behavior. If you have an empty version property on a component that references an external SBOM, what information is that supposed to convey? I don't see valid use-cases for this, so the error makes sense.

But your argument brought me to a larger issue I hadn't considered. Obviously, the user shouldn't be required to include all of the mandatory fields in a component which references an external SBOM. That is the entire point of this PR.
But what if they chose to include some information nevertheless (e.g., a version as in your example)? Either we prohibit this (not my preferred solution) or we ensure that any properties which are present still conform to our requirements (e.g., not be empty).

AFAIK, this applies to:

  • name (minLength 1)
  • version (minLength 1)
  • copyright (minLength 1) or licenses (minItems 1)

Further: if the url is not a valid bom-link as described on the cdx website, respectively specification, you ignore it. So I could write some gibberish, even just an empty url and still would ignore the validation. Though this hopefully is only an issue if providing an externalReference manually.

That is by design. Bom-linking isn't the only way to reference an external SBOM. As far as I am concerned, a URL is also fine. The stock CDX spec already ensures that the URL is either a valid IRI-reference or a bom-link. Unfortunately, it is actually not easy to find a string that isn't a valid IRI-reference, so it's hard to verify that this works. Try :foo as a URL. You'll see that this throws an error.
The empty string is a special case and I agree that it should be invalid. I'll make the change.

@mmarseu mmarseu force-pushed the 295-ignore-any-requirements-in-the-custom-schema-on-components-which-have-an-external-reference-of-type-bom-however-output-a-warning-that-the-validity-of-the-component-cannot-be-checked branch from 8563c66 to 63d2d3a Compare November 25, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants